-
Notifications
You must be signed in to change notification settings - Fork 294
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
Gate prototypes behind compatability flag for cache: no-store #2429
Gate prototypes behind compatability flag for cache: no-store #2429
Conversation
const req = new Request('https://example.org', { cache: 'no-store' }); | ||
assert.strictEqual(req.cache, 'no-store'); | ||
} else { | ||
assert.throws(() => { |
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.
Shouldn't we also be putting this new behaviour (throwing when cache
is present) behind the compatibility flag too?
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.
When the compat flag is off, we need to be using the old jsg::Unimplemented
definition, which would make this code irrelevant.
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 can't exactly be using that dynamically it's part of the type right? Should we just be throwing the same 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.
Sure you can make it dynamic. Rename jsg::Unimplemented getCache();
to jsg::Unimplemented getUnimplementedCache();
, then do something like this in the JSG_RESOURCE_TYPE
block:
if (flags.getCacheOptionEnabled()) {
JSG_READONLY_PROTOTYPE_PROPERTY(cache, getCache);
} else {
JSG_READONLY_PROTOTYPE_PROPERTY(cache, getUnimplementedCache);
}
So the exported cache
property in JS hooks up to different implementations depending on the flag.
6257929
to
cc84143
Compare
Can we please completely revert the previous change, and then re-land? I want to be able to see a diff from the known-good state to the final state, so I can verify that everything new has been properly gated. |
Small bug fix, where prototype for
cache
was not gated behind the compatibility flag in #2409.