Query call twice with fetchPolicy: no-cache

[clementloridan]

Issue Description

On a simple page, my useQuery or useLazyQuery will be fetch twice with fetchPolicy: 'no-cache'.
Maybe because of ssrForceFetchDelay.

Should I miss something about fetchPolicy: no-cache and ssrForceFetchDelay?

Why no-cache? Because I don't manage a lot of data with apollo/client, performance issue.

Thk to helping me :)

Link to Reproduction

https://github.com/clementloridan/react-apollo-error-template

Reproduction Steps

const [fetch] = useLazyQuery(QUERY, {
        fetchPolicy: 'no-cache',
        nextFetchPolicy: 'no-cache',
 });

React.useEffect(() => {
        fetch();
}, []);

My apollo config:

ssrMode: typeof window === 'undefined',
ssrForceFetchDelay: 100,
defaultOptions: {
    watchQuery: {
        fetchPolicy: 'cache-and-network',
        nextFetchPolicy: 'cache-first',
        errorPolicy: 'all',
    },
    query: {
        fetchPolicy: 'cache-and-network',
        nextFetchPolicy: 'cache-first',
        errorPolicy: 'all',
    },
    mutate: {
        errorPolicy: 'all',
    },
},

[jerelmiller]

Hey @clementloridan 👋 !

Are you by chance using React 18 and React's StrictMode to wrap your app? React runs effects twice in development when using strict mode. This is a likely possibility given your code snippet above.

If you are not running strict mode, a reproduction of the problem would be great so we can narrow this down. You can use our error template to provide some boilerplate to demonstrate the issue. Oops, somehow I missed your link to reproduction when I posted this. We'll take a look when we get a chance!

[clementloridan]

Hi @jerelmiller, I have the same issue on the react-apollo-error-template. And this template is based already on React 18.
I sync my repo on codesandbox: https://codesandbox.io/s/github/clementloridan/react-apollo-error-template for check quickly.

(And thk @jerelmiller for your quick answer)

[jerelmiller]

@clementloridan I'm so sorry, I totally missed your reproduction link when I posted that earlier comment. I could have answered my own question 🤣 . We will take a look at this when we get a chance!

[clementloridan]

More info
Working well on @apollo/client@3.5.0.beta-2

loading stay to true between: @apollo/client@3.5.0.beta-3 to @apollo/client@3.5.0.beta-9

Issue appear from @apollo/client@3.5.0.beta-10

https://codesandbox.io/s/tender-golick-q9ru50

[phryneas]

This gets stranger and stranger - with this change, the double-fetch doesn't occur as well:

+setTimeout(() => {
  const container = document.getElementById("root");
  const root = createRoot(container);
  root.render(
    <ApolloProvider client={client}>
      <App />
    </ApolloProvider>
  );
+}, 500);

=> if the Apollo client is created at the same time as the first render runs, we see a double-fetch. Otherwise not.

PS: because client.disableNetworkFetches switches from true to false after creation.

And that now plays a role because of 4a0e8dd - which happened between those two betas that you told us about.

[phryneas]

Which results to a fix for this in userland (for now, this still gives us plenty of stuff to evaluate & talk about internally):

const client = new ApolloClient({
  cache: new InMemoryCache(),
  uri: "https://swapi-graphql.netlify.app/.netlify/functions/index",
  ssrMode: typeof window === "undefined",
-  ssrForceFetchDelay: 100,
+  ssrForceFetchDelay: typeof window === "undefined" ? 100 : 0,
  defaultOptions: {
    watchQuery: {
      fetchPolicy: "cache-and-network",
      nextFetchPolicy: "cache-first",
      errorPolicy: "all",
    },
    query: {
      fetchPolicy: "cache-and-network",
      nextFetchPolicy: "cache-first",
      errorPolicy: "all",
    },
    mutate: {
      errorPolicy: "all",
    },
  },
});

In a non-SSR-scenario, both ssrMode should be false and ssrForceFetchDelay should be 0.

[phryneas]

Issue Writeup #10540

Reproduction

Description:
An ApolloClient is rendered with a positive ssrForceFetchDelay on the client. That means, the client wil stay in disableNetworkFetches mode for 100ms.
After that, disableNetworkFetches switches over to false.
Generally, a positive ssrForceFetchDelay essentially implies ssrMode for a certain time, with the purpose of skipping network-only and cache-and-network fetches within that first time window. (See docs)
In this case, a query is made with useQuery and fetchPolicy: "no-cache". That will cause a network request to occur, even if disableNetworkFetches is true.
As disableNetworkFetches is taken into account for the dependency array used in useQuerys useSyncExternalStore effect, switching disableNetworkFetches after 100ms triggers that effect a second time.
Since the value is first unsubscribed and then resubscribed and not stored in the cache, it is discarded and a second request is made.

Questions

• should no-cache queries happen in the disableNetworkFetches time-window?
  Probably yes?
• should switching from disableNetworkFetches: true to disableNetworkFetches: false ever re-execute a query?
  Probably not if data is already present? On the other hand, what about cache-and-network?
• As this was caused by 4a0e8dd: is that change a bug, or did it just surface a bug that should be fixed elsewhere?
  I would argue that this is an underlying bug that was just surfaced by that change.

Follow-up questions (may go off into other tangents)

• should changing disableNetworkFetches from true to false rerender components? Right now, it seems pretty accidental: if there is no rerender after disableNetworkFetches was set to true, a request is potentially never made, even if it should have only been skipped temporarily by that option. According to 8644, this is not only used in SSR scenarios on the server, but also in rehydration scenarios on the client, where it is not impossible that necessary data is not rehydrated, so a fetch call might still be necessary.

Potential fix

Delaying the "unsubscribe" for a tick gives the "next subscription" an option to latch onto an existing non-cache in-memory value. That resolves this specific scenario, but I am not sure if there are other scenarios, where we might need another fix.

- return () => subscription.unsubscribe();
+ return () => setTimeout(() => subscription.unsubscribe());

Coincidentally, this fixes also a bug with polling and StrictMode: #9819

@jerelmiller @alessbell @benjamn I would need some feedback on this. Is my analysis here okay - is that fix enough, or do we need more? What about the follow-up-question?

[bignimbus]

Hi all, the fix for this was merged and should be released with the next patch, probably sometime this week 🚀

[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.