defaultOptions fetch policy 'cache-and-network' is not used

[Arquanite]

Intended outcome:
useQuery (and probably others) should use default fetch policy (in this case 'cache-and-network')

Actual outcome:
It does request only once, just like 'cache-first' fetch policy. It seems like it ignores default setting.

How to reproduce the issue:
I edited example from docs to show this bug. Press "Change" button to switch currency. It should make request everytime, but it does only once per currency.
https://codesandbox.io/s/default-options-u7jut

Versions
Apollo-Client: 3.0.2
React: 16.13.0

[maapteh]

you are using a graphql endpoint which spins down on no activity, so you first have to open https://48p1r2roz4.sse.codesandbox.io/ in order to see it working, but indeed this is a bug :)

[Titozzz]

I can confirm this issue, I'm investigating with a teammate to see if we can find why 🤷

[adrienharnay]

@Titozzz and I are pretty sure it's caused by this:

apollo-client/src/core/QueryManager.ts

Lines 836 to 852 in 50fa0b8

	if (fetchPolicy === "cache-and-network" ||
	fetchPolicy === "network-only") {
	// 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, so we
	// modify options.fetchPolicy here 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.
	options.fetchPolicy = "cache-first";
	}

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

It would mean that when using apollo-persist-cache and for, say, a paginated component:

• first page load ever: I load the 1st page and navigate to the 2nd and 3rd (through the same query, but variables change)
• some new element gets created (which impacts 1st, 2nd and 3rd pages)
• I reload the first page, the cache gets updated (for the 1st page)
• I navigate to the 2nd and 3rd pages (still through the same query, but variables change), my cache is hit, and never gets refreshed (the query is never performed again at all)

That doesn't seem right, or maybe I misunderstood something? 🤔

[adrienharnay]

@benjamn @hwillson sorry to summon you, do you have an opinion on the matter?

[adrienharnay]

Some (somewhat organized) thoughts on this issue, after thinking about it this week-end. Please note I do not have full knowledge of the codebase, and while making assumptions I know I might not grasp the whole complexity of the code involved.

1. I've tried commenting options.fetchPolicy = "cache-first"; and see what happens, since I believe it's not the best behavior and a "dirty fix" for other problems at other places. The next comments will be based on this assumption. Also, I'm testing using apollo-persist-cache.

2. First problem: at page load, some queries are performed twice, while that was not the case before. Logging here:

   apollo-client/src/react/hooks/utils/useDeepMemo.ts

   Line 18 in 50fa0b8

   ref.current = { key, value: memoFn() };

   makes me realize the tick gets updated

   apollo-client/src/react/hooks/utils/useBaseQuery.ts

   Lines 31 to 40 in 50fa0b8

   	if (!queryData.ssrInitiated()) {
   	// When new data is received from the `QueryData` object, we want to
   	// force a re-render to make sure the new data is displayed. We can't
   	// force that re-render if we're already rendering however so to be
   	// safe we'll trigger the re-render in a microtask.
   	Promise.resolve().then(forceUpdate);
   	} else {
   	// If we're rendering on the server side we can force an update at
   	// any point.
   	forceUpdate();

   because onNewData

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

   Line 281 in 50fa0b8

   onNewData();

   is called.
   That makes the key for useDeepMemo update, and execute is called again.

Yes, the data has changed since the last call (which was the first call, in a loading state, that returned null), but not compared to what I have in the cache (the objects are the same). Also, the condition is based on the loading state, so the tick will get updated anyway.

=> Right now, updating the tick re-renders AND triggers execute(). The condition in onNewData is the only thing that prevents the tick from getting updated -> triggering the call -> tick getting updated -> infinite loop. Maybe re-rendering and re-fetching should be decoupled?

3. Second problem: the current implementation (with options.fetchPolicy = "cache-first"; commented or not is not optimal with queries that use dynamic variables.

Not commented: as stated in previous comments, the queries won't fire again when the variables change and the cache can be hit (cache-first after first call).

Commented: useDeepMemo only tracks the last call to execute

apollo-client/src/react/hooks/utils/useDeepMemo.ts

Lines 11 to 22 in 50fa0b8

	export function useDeepMemo<TKey, TValue>(
	memoFn: () => TValue,
	key: TKey
	): TValue {
	const ref = useRef<{ key: TKey; value: TValue }>();
	if (!ref.current || !equal(key, ref.current.key)) {
	ref.current = { key, value: memoFn() };
	}
	return ref.current.value;
	}

, so doing variables A then variables B then variables A, useDeepMemo's cache will always be missed.

I would suggest using hashing the key and doing ref.current = { ...ref.current, [key]: value } instead, so that all performed calls can be cached and hit at the same time. That would also get rid of equalify.equal, and I would suggest something like that util https://github.com/Brigad/redux-rest-easy/blob/master/src/internals/utils/hash.js which is super fast. I believe the perfs would be the same, but would need to measure in order to be sure.

I think making these 3 changes would:

1. avoid queries that don't re-render when changing variables, that can be confusing especially when you don't know the internals of apollo-client. You specified cache-and-network in your app as a default, yet the internals override it
2. avoid having the same logic for re-rendering and re-fetching, which I believe now only works because of the options.fetchPolicy = "cache-first"; patch
3. improve useDeepMemo for queries with dynamic variables

Please let me know what you think, and if you agree to at least part of what I think, let me know how I could help 🙂

[benjamn]

@adrienharnay Thanks for investigating and sharing your reasoning!

We are strongly considering reverting #6353, with an easy way to restore that behavior if you really want it: #6712. Curious to hear if you have any thoughts about that.

As for useDeepMemo, we'd really prefer to avoid using it altogether, and rely instead on other layers of caching. It seems self-defeating to put such a simplistic caching mechanism in front of the InMemoryCache, and useDeepMemo has definitely caused more than its share of problems in the past (as @hwillson can attest).

[adrienharnay]

Thank you for your answer, #6712 seems greats!

As for useDeepMemo, did you already start replacing it with another layer of caching? I'd like to hear more about it :)

[martinjlowm]

I think it would be a wise decision to document the nextFetchPolicy-trick (at least explain what problem it can solve). I've spent quite some time figuring out why we were seeing infinite queries in multiple places. Currently, I don't see how cache-and-network would work on its own, since the cache is updated on every response with a new reference, causing a re-render through the updated tick and a refetch as Adrien points out. I'd gladly contribute some additional docs :)

[hwillson]

It doesn't sound like there is an outstanding issue here, so closing. Thanks!

[adrienharnay]

Well I guess it is still related to this unresolved issue: #7437