Skip to content
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 opt-in to cache responses with errors #2363

Merged
merged 1 commit into from Jun 16, 2020
Merged

Add opt-in to cache responses with errors #2363

merged 1 commit into from Jun 16, 2020

Conversation

lwasyl
Copy link
Contributor

@lwasyl lwasyl commented Jun 15, 2020

Most straightforward way to fix #2358 by implementing #2281 (comment).

I could make STORE_PARTIAL_RESPONSES deprecated right away, if there's another suggestion to enable old behavior. Overall I consider this a quick workaround for people affected by the breaking change introduced in #2281

Copy link
Contributor

@tasomaniac tasomaniac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I actually think that this is the right way to do it but not a work-around. The behavior change in 2.1.0 was also discussed with apollo-ios folks. Storing partial when errors are present brings some other edge case issues that are hard to resolve. Enabling that via header is a good solution for people need it.

My question is that how can we make this more obvious in the documentation?

@martinbonnin
Copy link
Contributor

I too like the flexibility of this solution. I also like that the default is to not cache partial responses.

I'm not sure we want to want to advertise the new header too much though. It's an advanced use case and I'd prefer users to not use errors for normal operation. Having to dig a bit for the header would ensure that users are aware of what they are doing ?

@tasomaniac
Copy link
Contributor

If they're aware of the custom header support, then these docs on the objects are all good. I'm just afraid that not a lot of people know that this even exists.

@martinbonnin
Copy link
Contributor

@tasomaniac I can write some doc about this. I think we need to elaborate on the CacheKeyResolver documentation. I was thinking about doing a separate Normalized Cache page and there could be a section about partial responses. Would that work?

@sav007 any objection to merging this?

@tasomaniac
Copy link
Contributor

@martinbonnin up to you. You know it the best. I just wanted to bring it up. If we don't have the right context to document this particular behavior we can just mention the fact that we have options to control caching behavior.

@martinbonnin martinbonnin merged commit ba9d69f into apollographql:master Jun 16, 2020
@lwasyl lwasyl deleted the store-partial-responses-opt-in branch June 16, 2020 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow caching partial responses with error
4 participants