Unexpected Behavior with fetchPolicy "network-only" and nextFetchPolicy "cache-first" In 3.7.x

[dairyisscary]

I'm in the middle of doing an upgrade of AC 3.6.8 to 3.7.x (currently tested and template made with 3.7.1) and I found something that I think is a bug, but I'm not sure.

Intended outcome:
I have useQuery inside a component that is conditionally shown that uses the fetchPolicy of "network-only" and nextFetchPolicy of "cache-first". In the demo below, I've made the querying component conditionally mount with a toggle button. I expect that every time the component is mounted fresh, it should always fetch from the network and ignore the cache for that first load.

Actual outcome:
It appears that, instead, the component comes to life, fetches from the network, fills the cache, then when I hide and fully unmount the component, then when I toggle it back to mounted, it does in fact start fetching from the network but seems to "apply" the "cache-first" policy too quickly and displays the cached data before the data comes back from the network. In the demo below, I've raised the delay on the network to 1.5s so it's more obvious and made the length of the people array a count of the number of times the server data is used.

In other words, I think I expect:

• click to show the querying component, I see Loading... for 1.5s, then 1 as the count.
• click to hide the querying component
• click to show the querying component, I see Loading... for 1.5s, then 2 as the count.

but instead, what actually happens on that last bullet is I never see Loading..., I see 1 as the count, then 1.5s later it updates to 2.

How to reproduce the issue:
https://github.com/dairyisscary/react-apollo-error-template

Versions
3.7.x

[jerelmiller]

@dairyisscary I played around with this a bit and can indeed confirm the issue you are seeing is a regression. It looks however that this regression was introduced in v3.6.9. I am able to toggle between v3.6.8 to see it work and v3.6.9 to see it break.

Looking at the changelog, it looks like this PR meant to fix an issue with toggling skip also introduced this regression.

[jerelmiller]

I've gone ahead and written a failing test that demonstrates this issue.

[levrik]

I'm unsure if I'm hit by the same issue but it might be.
Basically I have a custom hook which uses useQuery inside. I render the either data or previousData if data is unavailable, so the previous data stays while the new data is loaded. The fetch policy is set to network-only.
If now variables change to something else and then change back to what they original were, cache-first is used, breaking the behavior of rendering the previously fetched data as data is not undefined.
With Apollo Client 3.5.10 which I used before, data was undefined and previousData was correctly displayed while fetching with the changed variables.
I seem to can work around this by manually setting nextFetchPolicy to network-only but I'm unsure if this might have other unintended side-effects. No re-mounting involved here.

[jerelmiller]

@levrik would you be able to provide a bit more detail on your issue? To make sure I understand, are you saying that you have something like the following?

useQuery(QUERY, {
  fetchPolicy: 'network-only',
  nextFetchPolicy: 'cache-first'
})

Some code or a failing test would be tremendously helpful to understand if its a similar issue or not.

One of the changes made in #9823 is that once a request is kicked off, the nextFetchPolicy is then updated synchronously (see more details on this comment for the reasons why). It's possible you're hitting this case. If not, some code samples would help us understand what you're seeing.

[levrik]

@jerelmiller Exactly. I'll try to assemble an example highlighting the issue latest next week.

[skrivle]

Hi All 👋

We are also experiencing this issue it seems. We were able to reproduce this both in a test case and in browser here: https://github.com/skrivle/apollo-next-fetch

This is a regular CRA project. You can run npm test and you'll see the test failing. To make the test pass you can comment out the nextFetchPolicy config item in App.tsx

For the in browser reproduction please follow these steps:

• npm start
• click contact 1
• click contact 2
• click unload (which will unmount the app component)
• click contact 1
• observe that "loading" is shown very briefly and then it incorrectly shows the cached data for contact 2 until data for contact 1 is loaded.

Note: the conversation items have the same id but different content across the two requests. This is to illustrate the scenario where the properties of an object with a consistent id change over time.

[JWo1F]

I have the same error. When will it be fixed?

The problem looks even more frightening in light of the fact that these settings can be made as default settings. In my project we have the default settings as fetchPolicy: network-only and nextFetchPolicy: cache-first. As a result this bug affects almost the whole service.

1. As a solution I tried to roll back to 3.6.10, but the same problem is reproduced on 3.6.10 too.
2. I've also tried version 3.6.9, the bug also plays on it.
3. I then rolled back to 3.6.8 and the bug disappeared.

From this I conclude that the bug appeared in version 3.6.9.

[jerelmiller]

Hey @JWo1F 👋 !

That is was I concluded as well. The bug was introduced in 3.6.9 (see my links in that comment for more references to the underlying issue). I have a failing test that demonstrates this issue. In fact I attempted to fix it but so far have been unsuccessful. I'm hoping to come back to this bug sometime soon to get a proper solution in place (if you're curious about more specifics, see my comment on the challenges I've faced thus far).

[JWo1F]

Hey @jerelmiller!

Could you tell me, you said you were going to do the task "sometime soon".
When will you have time to fight the bug?

[phryneas]

I've picked this up and given it a shot over in #10631

[JWo1F]

@phryneas, hi! Do you have any progress on this task?

[phryneas]

@JWo1F I am not quite content with the PR yet, and have most of my work directed elsewhere at the moment. I hope that I can return to this within the next few weeks, I'm sorry for the delay!

[JWo1F]

@phryneas, hi! Any progress on the task? It's been 3 weeks and we still can't update to the new version... Sorry to keep bugging you, this is a very hot topic for us.

[phryneas]

@JWo1F I'm very sorry that it has been yet another 3 weeks - we have been working hard to get the Next.js support package and the 3.8 beta out, so this has been a bit on the back burner.

I will be on conference for a week now, but already have a reminder set for this issue once I'm back. I'm really sorry that I can't give you better news than that, but I thought I'd at least keep you in the loop.

Would it help you if I rebase the current PR onto the current main branch and make a PR-release that you can use in the meantime?

[JWo1F]

@phryneas, yes, it works for us! Thank you!

[phryneas]

@JWo1F I've made a PR release for you - you can install it with npm i @apollo/client@0.0.0-pr-10631-20230531131910

[phryneas]

@JWo1F I've picked this up again - the PR is in a ready-for-review state now and in a moment, there will be another build available to you over there in a comment :)

[jerelmiller]

Fixed by #10631 and released with v3.7.17.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.