[v3.4 Regression] Changing variables uses nextFetchPolicy instead of fetchPolicy

[dylanwulf]

Intended outcome:
I have a query with fetchPolicy: 'network-only' and nextFetchPolicy: 'cache-first'. When I change the variables, it should fetch new results from the server regardless of what's in the cache.

Actual outcome:
Changing the variables appears to use the nextFetchPolicy and does not fetch new data from the server.

How to reproduce the issue:
https://github.com/dylanwulf/react-apollo-error-template/tree/changing-variables-nextFetchPolicy
Please use branch changing-variables-nextFetchPolicy.
Reproduction instructions: Open dev tools and watch the console. Click any of the unselected radio buttons, see that a request to the server is made (this is printed to the console instead of the network tab due to the apollo link). Click "All", see that a request to the server is NOT made.

It looks like this problem first showed up in v3.4.0-rc.5 and still exists in v3.4.0-rc.14

[brainkim]

Related:

• nextFetchPolicy should reset to fetchPolicy when variables changes #7437
• Queries in a re-mounted component revert to using "fetchPolicy" instead of "nextFetchPolicy" #7311
• watchQuery defaultOptions are unset when query variables change (specifically nextFetchPolicy) #6839

[dylanwulf]

@brainkim I thought it was interesting that #7437 is happening in v3.3 whereas I only saw my issue in v3.4. I played around a little and I was able to get the issue to happen in v3.3 if fetchPolicy and nextFetchPolicy are set in defaultOptions instead of directly in useQuery options. Possibly related to #6839? (which is another issue I would love to be fixed)

[benjamn]

@dylanwulf The operative change in 3.4.0-rc.5 was the addition of this line in PR #8346:

apollo-client/src/react/data/QueryData.ts

Line 203 in f11a163

applyNextFetchPolicy(options);

Calling applyNextFetchPolicy here advances immediately to nextFetchPolicy without ever using fetchPolicy, which seems incorrect to me. Removing this line fixes the reproduction you provided, though you might want to remove notifyOnNetworkStatusChange: true from your useQuery options to avoid receiving intermediate loading: true results with cache data.

On a more general note, since you mentioned #6839, I have to admit I'm not comfortable with the role useQuery plays in resetting fetchPolicy and nextFetchPolicy, which happens simply because useQuery is called again with (very likely) the same initial options object, every time the component rerenders. I happen to think fetchPolicy should be reset when variables change, but I don't think useQuery is the right/best mechanism to accomplish that.

[dylanwulf]

@benjamn thanks for looking into this! awesome to hear you found the root cause

though you might want to remove notifyOnNetworkStatusChange: true from your useQuery options to avoid receiving intermediate loading: true results with cache data.

When I change the variables, I seem to be receiving intermediate loading: true results with cache data whether notifyOnNetworkStatusChange is true or false (both in v3.3, and v3.4 with that change to QueryData.ts you described). But I can always add in some simple logic to hide results whenever loading is true.

which happens simply because useQuery is called again with (very likely) the same initial options object, every time the component rerenders. I happen to think fetchPolicy should be reset when variables change, but I don't think useQuery is the right/best mechanism to accomplish that.

I agree, I think that when the variables change it should use the fetchPolicy that's being passed into the options (whether that's in the useQuery options or the defaultOptions)

Between #6839 and #8426 (comment), defaultOptions seem pretty much unusable since I have to explicitly specify fetchPolicy and nextFetchPolicy in every single useQuery anyway. But at least that's a problem with a workaround.

[benjamn]

When I change the variables, I seem to be receiving intermediate loading: true results with cache data whether notifyOnNetworkStatusChange is true or false (both in v3.3, and v3.4 with that change to QueryData.ts you described).

@dylanwulf I looked into this, and it seems to be a consequence of using React useState to store/modify the gender variable. Changing the gender using setGender forces React to rerender the component immediately, even though we might want the update to take place in the background for network-only fetches. Since useQuery can't return the final data yet, it returns a loading: true result, regardless of notifyOnNetworkStatusChange.

When using client.watchQuery and the ObservableQuery API directly, changing the variables with notifyOnNetworkStatusChange: false should skip the loading: true result, so you only see the final update. I'm not sure there's much we can do about the React situation right now, other than reporting the truth about the loading: true state of the results useQuery is forced to render.

For what it's worth, we are actively rethinking these issues with useQuery in the context of the a new useFragment hook (see #8236).

[dylanwulf]

forces React to rerender the component immediately

Oooh ok that makes sense, thank you for the explanation

[benjamn]

Alright, except for the weirdness about notifyOnNetworkStatusChange: false not working with React and useQuery, I believe this is fixed in @apollo/client@3.4.0-rc.18. Thanks for pushing this issue (and #6839) forward @dylanwulf!