Error cached on suspenseQuery

[mehdiSlam]

Issue Description

Hello,

When an error is throw by a suspenseQuery, the result of the error call is cached even if i use 'no-cache' as fetch-policy.
I checked in the code of useSuspenseQuery and a promise containing the error call is cached in the queryRef in the suspense cache and reuse every time.

Example :

• i go to the page with my suspenseQuery who failed
• i change page
• i go back to the page with the suspenseQuery who failed => no call ( cache hit for no reason )

Link to Reproduction

https://codesandbox.io/p/devbox/dawn-brook-wl8883

Reproduction Steps

• Open console
• click on Suspense error link
• The server log Throw fake error server side and throw error => Error display on the page
• click on Home link
• click on Suspense error link
• no new log from the server => server not recall

@apollo/client version

3.12.2

[jerelmiller]

Hey @mehdiSlam 👋

Appreciate the reproduction! The no-cache fetch policy only handles data cached in InMemoryCache, but has nothing to do with the suspense cache that you're referring to here.

That said, it looks like what's happening is that the queryRef is retained in the SuspenseCache for 30 seconds after the error is thrown until it gets cleaned up, hence why navigating back to the page within that 30s window will throw the error immediately instead of making the request. If you navigate to the home page, wait 30s, then navigate back to the suspense page, you'll see that the request is made.

Incidentally, this is something we've actually tested for. You can see how the test waits for the autoDisposeTimeoutMs (default 30s) before the query ref gets disposed:

apollo-client/src/react/hooks/__tests__/useSuspenseQuery.test.tsx

Lines 3669 to 3699 in 10c8755

	it("tears down subscription when throwing an error", async () => {
	jest.useFakeTimers();
	using _consoleSpy = spyOnConsole("error");
	const { query, mocks } = useErrorCase({
	networkError: new Error("Could not fetch"),
	});
	const client = new ApolloClient({
	cache: new InMemoryCache(),
	link: new MockLink(mocks),
	});
	const { renders } = await renderSuspenseHook(
	() => useSuspenseQuery(query),
	{
	client,
	}
	);
	await waitFor(() => expect(renders.errorCount).toBe(1));
	// The query was never retained since the error was thrown before the
	// useEffect coud run. We need to wait for the auto dispose timeout to kick
	// in before we check whether the observable was cleaned up
	jest.advanceTimersByTime(30_000);
	expect(client.getObservableQueries().size).toBe(0);
	jest.useRealTimers();
	});

That said, I would agree that this isn't super great DX as-is. I'll see if I can play around with this a bit and figure out how to dispose of the queryRef in the case that an error would be thrown. Unfortunately the mechanics of suspense make this a bit tricky 🙁

[mehdiSlam]

Thanks for the explanation @jerelmiller . Indeed we found this "problem" in a test where two unit tests shared the same Apollo client but tested two different errors. The second test failed because of this delay.
The hack I found to fix the tests was to remove the client cache suspense between each tests (but it's not very pretty) :

const suspenseCacheSymbol = Symbol.for(‘apollo.suspenseCache’);
if (myApolloClient[suspenseCacheSymbol]) {
        delete myApolloClient[suspenseCacheSymbol];
   }

Using the fake timeout is more appropriate

There are very few real cases where this delay could be a problem, and at worst it would be resolved after 30 seconds.

[jerelmiller]

Hey @mehdiSlam 👋

So sorry for my delayed response. This does work, but I'm curious why you share a client between multiple tests? We'd recommend against that mostly to avoid carrying internal state/caches around between tests which might add implicit dependencies between them (like you're seeing here). If you're able to, I'd highly recommend recreating your client for each test so that each test is isolated.

That said, if you want something a little more "official", there is a timeout that controls when those cache entries get cleaned up automatically. It defaults to 30s, but you can tweak it by modifying autoDisposeTimeoutMs option:

new ApolloClient({
  defaultOptions: {
    react: {
      // Make sure this is long enough to allow the operation to finish fetching
      suspense: { autoDisposeTimeoutMs: 1000 }
    }
  }
})

This will at least reduce that 30s timeout so that its less likely that cache kicks in between tests.

[mehdiSlam]

Hello,
We have one client per test file, in general most of the mocks in a test file are the same hence the fact of having one Apollo client per test file (we reset/clear the cache before each execution). It is a old pattern that is reproduced a little everywhere that we have not yet had the time to change it.

Thanks for the tips 👍