-
Notifications
You must be signed in to change notification settings - Fork 45
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
Dont cache graphql errors #1219
Conversation
861ec67
to
8d4d0f4
Compare
FYI @sjahl |
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.
Nice sleuthing on this one, this will be an excellent and crucial patch to have pre-v4.
Only comment is that per Steve's slack thread (linked here again for convenience), the header should be no-store
, rather than no-cache
.
Other than that lookin' good.
// Also, as written, this will no-cache not-found errors, which is | ||
// undesirable, but something we can put off fixing until later. | ||
|
||
response.set('Cache-Control', 'no-cache') |
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.
Per this Slack thread, I believe this should be no-store
rather than no-cache
, to have our desired effect
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.
Yes thank you nice catch
Note that one subtlety of this is that it also no-caches not-found errors, which is undesirable. If there's no, e.g., gene X now, that's going to also be the case later.
8d4d0f4
to
9ccc331
Compare
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.
LGTM!
No description provided.