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

Preserve cache-and-network fetchPolicy when refetching. #4840

Merged

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented May 20, 2019

Should help with the problem reported by @supercranky in this comment.

The apparent reasoning for preserving the no-cache and network-only fetch policies is that refetching requires a "network" fetch policy, but I believe cache-and-network should also count as a "network" fetch policy, since it always attempts to fetch from the network in addition to fetching from the cache. Even if that network request fails, it may be useful (e.g. for offline usage) to return partial data from the cache, which is currently not possible when refetching, given the refetch method's behavior of overriding non-network/cache-friendly fetchPolicys.

@benjamn benjamn added this to the Release 2.6.0 milestone May 20, 2019
@benjamn benjamn requested a review from hwillson May 20, 2019 17:53
@benjamn benjamn self-assigned this May 20, 2019
@benjamn
Copy link
Member Author

benjamn commented May 20, 2019

@hwillson If this feels like too much of a behavioral change before the final 2.6.0 release, we can postpone it for 2.6.1 or 2.7.0, especially since the original behavior has been in place since long before Apollo Client 2.6 or 2.5 (since back when forceFetch was a thing, in fact: 4fc0b64).

@@ -1070,7 +1071,7 @@ describe('ObservableQuery', () => {
const observable = queryManager.watchQuery({
query: firstRequest.query,
variables: firstRequest.variables,
fetchPolicy: 'cache-and-network',
fetchPolicy: 'cache-first',
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was literally just checking that cache-and-network gets overridden with network-only when refetching, so I think it's safe to check cache-first instead (if we accept the premise of this pull request).

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Looks great @benjamn, and I'm 👍 on getting this into 2.6.0. Thanks!

packages/apollo-client/src/core/ObservableQuery.ts Outdated Show resolved Hide resolved
Should help with the problem reported by @supercranky in this comment:
#4636 (comment)

The apparent reasoning for preserving the no-cache and network-only fetch
policies is that refetching requires a "network" fetch policy, but I
believe cache-and-network should also count as a "network" fetch policy,
since it always attempts to fetch from the network in addition to fetching
from the cache. Even if that network request fails, it may be useful
(e.g. for offline usage) to return data from the cache.
@benjamn benjamn force-pushed the preserve-cache-and-network-fetchPolicy-when-refetching branch from 05c58f3 to 467146b Compare May 21, 2019 16:21
@benjamn benjamn merged commit bcd3aff into release-2.6.0 May 21, 2019
@benjamn benjamn deleted the preserve-cache-and-network-fetchPolicy-when-refetching branch May 21, 2019 16:26
@slorber
Copy link
Contributor

slorber commented Nov 20, 2019

Hi @benjamn

This change seems to be annoying for some of us (apollographql/react-apollo#3457 (comment))

Do you think it could be possible to pass a custom fetch policy to the "refetch" call? I want my query to have cache-and-network, but I want my refetch() promise to only resolve after the actual network refetch has been done, and this seems impossible currently

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants