-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add support for inherit key on cacheControl. #1424
Conversation
@lunks: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
This adds support for skipping using defaultMaxAge on types if inherited is set to true. This allows to more easily cache a whole node if you do not want or cannot to set an specific maxAge.
60be79b
to
288693a
Compare
@lunks Thank you for the PR! Can you elaborate on the situations when you're not able to set a specific @martijnwalraven definitely has some thoughts on this. We'd love to hear them. #1295 is also relevant |
@@ -1,5 +1,10 @@ | |||
# Changelog | |||
|
|||
### Unreleased | |||
|
|||
* Add support for `inherited` key to skip setting a `maxAge` to 0 if a |
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.
@lunks I wonder if it would provide more utility for inherit to be recursive, such that subfields will also inherit their cacheability as well from grandparents, etc. Basically, should inherit
itself be inherited?
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.
If I understand correctly what you're proposing, I don't think that's a good idea, because it would mark an entire tree as cacheable. And I think objects should always have to opt-in to caching (even if that's through inherit
).
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.
For a bit of background on why I think inheritance should be opt-in, see here:
https://github.com/apollographql/apollo-cache-control-js/issues/14#issuecomment-37315847.6
Sure, but first, thanks for the great project! So, my issue is: I currently have is a very slow list of images I'd like to cache. Unfortunately on other parts of the system, I need other images to not be cached because they might change a lot. So I'm left with either having a short I'm not sure how Lastly, I'll write the docs after we have agreed on the implementation if that's ok.
It sure would. Ideally, I could do something like |
You may already know this, but |
In my mind, the primary use case for something like |
One thing I wonder about is whether we want all cache hints to be inherited through For the former, I'm not sure what the semantics should be of combining The problem with |
I think we can re-open and re-use this code in the future if this takes root, but at this point, I believe it's mostly stalled at this point. I've tagged this for cache-control so we can reference it in future considerations. |
I haven't used Apollo in a while. I'll keep the branch in case anyone wants to take over. |
@abernix I share the pain point as described by lunks above: #1424 (comment). Some sort of inheritance mechanism would def. be ideal. |
This adds support for skipping using defaultMaxAge on types if inherited is
set to true. This allows to more easily cache a whole node if you do not
want or cannot set an specific maxAge.
TODO: