-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
An initial implementation of the cache API #428
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 😃 Added some comments, but looks really good, and especially like the README
for the cache plugin.
} | ||
} | ||
|
||
export class CacheError extends Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do this for R2Error
too, but this should probably extend MiniflareError
(or HttpError
?), and use code
instead of status
. MiniflareError
will automatically set the name
correctly too based off new.target
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this over from R2Error
, but the reason it uses status
rather than code
is because it is the http status code. R2Error
has another property v4Code
(or v4code
depending on context, the API is maddeningly inconsistent), so using code there would be a bit confusing, I think. For consistency, it might make sense to keep this as status
. I'll have a look at extending from MiniflareError
though
import { Clock } from "../../shared"; | ||
import crypto from "crypto"; | ||
import { AddressInfo } from "net"; | ||
import http from "node:http"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit-picky, but given we're not using node:
prefixes throughout the rest of the codebase (we probably should tbh), could we change this to just http
? Whatever we end up deciding, probably best to add an eslint rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding an eslint rule to enforce it? (In a separate PR, so as not to mess up the diff of this one?) I'll remove it for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that sounds good, I'd make the switch to node:
in that PR too. 👍
@mrbbot is this good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! 😊 LGTM!
To test this, build
workerd
on theexpose-cache-api
branch, copy the binary into the right directory withinnode_modules
for your system, and run miniflare withcache: true
. The following test worker should demonstrate the cache behaviour:test-cache.worker.js