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

Cache-and-network should not make a network call when watched cached items change via subscription/mutation #6305

Closed
jebonfig opened this issue May 18, 2020 · 3 comments · Fixed by #6353
Assignees
Milestone

Comments

@jebonfig
Copy link

This is new behavior as of beta .46 and has prevented us from moving past .45 (most likely a result of #6221).

Intended outcome:
A query with fetchPolicy: cache-and-network should not make a network call when a cached item that it's watching changes. It should just re-render containing the data from the cache.

Actual outcome:
When an individual cached item changes (via subscription or mutation), if a query with fetchPolicy: cache-and-network contains that cached item, the query is making a network call to refetch.

How to reproduce the issue:
As instructed, I've created a full reproduction here: https://github.com/jebonfig/react-apollo-error-template

Notes
cache-and-network is an important fetchPolicy in our App, as it gives us the benefit of quick renders from cache on subsequent component mounts, and also immediately follows up with the server for consistency.

A common example use case is:

  • a cache-and-network list of cars query
  • a cache-only query which pulls an individual car from the cars query data via typePolicy
    • quick side note here is I've also noticed new warnings: "Missing cache result fields: person". If I've specified a cache-only fetch policy on the query, and a type policy to pull it from the list, then my item not being in the cache is a perfectly valid scenario. I'd argue a warning here is unnecessary noise
  • a useSubscription watching the list of cars

When we receive a subscription update for an individual car, the car in the cache is updated with the new data as expected. However, the new, undesired behavior is that the list query now unnecessarily refetches from the network instead of just re-rendering based on the cache updates that have already been made.

So you might say, this is intended based on the .46 changes mentioned above. If that's the case, then how can I retain the initial onComponentMount query behavior of cache-and-network, AND the updating behavior when responding to mutations and subscriptions of cache-first?
In other words, I want my query to be cache-and-network on initial component mount, but cache-first when the cached data my query is watching has been changed by a mutation or subscription.

Versions

  System:
    OS: macOS High Sierra 10.13.6
  Binaries:
    Node: 11.2.0 - /usr/local/bin/node
    npm: 6.4.1 - /usr/local/bin/npm
  Browsers:
    Chrome: 81.0.4044.138
    Safari: 13.1
  npmPackages:
    @apollo/client: 3.0.0-beta.48 => 3.0.0-beta.48 
    @apollo/link-error: ^2.0.0-beta.3 => 2.0.0-beta.3 
    @apollo/link-ws: ^2.0.0-beta.3 => 2.0.0-beta.3 
@klaussner
Copy link

The problem also occurs with the "network-only" fetch policy. Whenever the cache is updated (either manually or by a mutation), the query is refetched.

benjamn added a commit that referenced this issue May 28, 2020
When someone chooses cache-and-network or network-only as their initial
FetchPolicy, they almost certainly do not want future cache updates to
trigger unconditional network requests, which is what repeatedly applying
the cache-and-network or network-only policies would seem to require.

Instead, when the cache reports an update after the initial network
request, subsequent network requests should be triggered only if the cache
result is incomplete.

This behavior corresponds exactly to switching to a cache-first
FetchPolicy, which we can achieve by modifying options.fetchPolicy in
QueryManager#fetchQueryObservable for the next fetchQueryObservable call,
using the same options object that the Reobserver always passes to
fetchQueryObservable.

Note: if these FetchPolicy transitions get much more complicated, we might
consider using some sort of state machine to capture the transition rules.

Should fix #6305, and a few other related issues (TBD).
benjamn added a commit that referenced this issue May 29, 2020
)

When someone chooses cache-and-network or network-only as their initial
FetchPolicy, they almost certainly do not want future cache updates to
trigger unconditional network requests, which is what repeatedly applying
the cache-and-network or network-only policies would seem to require.

Instead, when the cache reports an update after the initial network
request, subsequent network requests should be triggered only if the cache
result is incomplete.

This behavior corresponds exactly to switching to a cache-first
FetchPolicy, which we can achieve by modifying options.fetchPolicy in
QueryManager#fetchQueryObservable for the next fetchQueryObservable call,
using the same options object that the Reobserver always passes to
fetchQueryObservable.

Note: if these FetchPolicy transitions get much more complicated, we might
consider using some sort of state machine to capture the transition rules.

Should fix #6305, and a few other related issues (TBD).
@benjamn
Copy link
Member

benjamn commented May 29, 2020

@jebonfig Can you try @apollo/[email protected], which includes #6353?

@jebonfig
Copy link
Author

@jebonfig Can you try @apollo/[email protected], which includes #6353?

thanks a lot @benjamn - this is fixed on .52 🎉

benjamn added a commit that referenced this issue Jul 27, 2020
PR #6353 seemed like a clever zero-configuration way to improve the
default behavior of the cache-and-network and network-only fetch policies.
Even though the word "network" is right there in the policy strings, it
can be surprising (see #6305) to see network requests happening when you
didn't expect them.

However, the wisdom of #6353 was contingent on this new behavior of
falling back to cache-first not creating problems for anyone, and
unfortunately that hope appears to have been overly optimistic/naive:
#6677 (comment)

To keep everyone happy, I think we need to revert #6353 while providing an
easy way to achieve the same effect, when that's what you want. The new
options.nextFetchPolicy option allows updating options.fetchPolicy after
the intial network request, without calling observableQuery.setOptions.

This could be considered a breaking change, but it's also a regression
from Apollo Client 2.x that needs fixing. We are confident
options.nextFetchPolicy will restore the #6353 functionality where
desired, and we are much more comfortable requiring the explicit use of
options.nextFetchPolicy in the future.
benjamn added a commit that referenced this issue Jul 27, 2020
PR #6353 seemed like a clever zero-configuration way to improve the default
behavior of the cache-and-network and network-only fetch policies. Even
though the word "network" is right there in the policy strings, it can be
surprising (see #6305) to see network requests happening when you didn't
expect them.

However, the wisdom of #6353 was contingent on this new behavior of falling
back to cache-first not creating problems for anyone, and unfortunately that
hope appears to have been overly optimistic/naive: #6677 (comment)

To keep everyone happy, I think we need to revert #6353 while providing an
easy way to achieve the same effect, when that's what you want. The new
options.nextFetchPolicy option allows updating options.fetchPolicy after the
initial network request, without having to call observableQuery.setOptions.
Specifically, passing { nextFetchPolicy: "cache-first" } for network-only or
cache-and-network queries should restore the behavior of #6353.

This could be considered a breaking change, but it's also a regression from
Apollo Client 2.x that needs fixing. We are confident options.nextFetchPolicy
will enable the #6353 functionality where desired, and we are much more
comfortable requiring the explicit use of options.nextFetchPolicy going forward.
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants