skip: true and skipToken behave differently with useSuspenseQuery

[epmatsw]

Issue Description

I'm pretty sure this is a bug with skip: true, or at least skipToken behaves like I'd expect.

If you have a useSuspenseQuery that uses skip: boolean and toggles between true and false for that, it seems like sometimes the switch from true to false causes the query to skip the cache and suspend.

In the demo, you can quickly hit the button to toggle skipping on or off. Every once in a while, you can see the top one (skip: boolean) pop a suspend for a few frames, while the bottom one (skipToken) correctly re-reads from the cache.

Link to Reproduction

https://codesandbox.io/p/sandbox/lz67k5?file=%2Fsrc%2FApp.tsx%3A34%2C67

Reproduction Steps

No response

@apollo/client version

3.11.8

[jerelmiller]

This is such an interesting one. Both skip: true and skipToken have almost the same code paths. It looks like the only difference is that we set some extra options for skip: true (not sure why to be honest). See how skipToken returns early while skip: true adds the extra options:

apollo-client/src/react/hooks/useSuspenseQuery.ts

Lines 357 to 382 in 1f0333e

	if (options === skipToken) {
	return { query, fetchPolicy: "standby" };
	}
	const fetchPolicy =
	options.fetchPolicy ||
	client.defaultOptions.watchQuery?.fetchPolicy ||
	"cache-first";
	const watchQueryOptions = {
	...options,
	fetchPolicy,
	query,
	notifyOnNetworkStatusChange: false,
	nextFetchPolicy: void 0,
	};
	if (__DEV__) {
	validateOptions(watchQueryOptions);
	}
	// Assign the updated fetch policy after our validation since `standby` is
	// not a supported fetch policy on its own without the use of `skip`.
	if (options.skip) {
	watchQueryOptions.fetchPolicy = "standby";
	}

Other than this, both of these code paths rely on checking fetchPolicy: 'standby' in the downstream code which should avoid suspending (the __use hook is the thing that causes it to suspend):

apollo-client/src/react/hooks/useSuspenseQuery.ts

Line 253 in 1f0333e

const result = fetchPolicy === "standby" ? skipResult : __use(promise);

I'll have to dig into this further to understand where the difference is, but unfortunately won't have time to do so for a couple days. Thanks for raising the issue!

[naman1608]

Not sure but I think the options.skip check should also be moved with the skipToken check. It would help with the following -

1. Consistency between both options.skip and skipToken
2. There is no need to add notifyOnNetworkStatusChange and 'nextFetchPolicyas the query isn't being executed in the standby mode, so those lines seem redundant in the case ofoptions.skipastrue` also.

@jerelmiller I can put a PR if it makes sense. Although I see in the docs that options.skip is intended to be removed, so not sure if it be of help.

[jerelmiller]

@naman1608 you're welcome to try a fix! I think moving the check for options.skip up to the same conditional as options === skipToken makes sense as a starting point. The one difference here is that variables is set in options with options.skip, so we might want to add a test case to make sure that changing variables while skip is true, then changing skip to false properly suspends and fetches with the right variables (does that make sense?).

We are preparing for our upcoming 3.12 release, so if you end up getting a fix in place, we'll look at adding that to a 3.12 patch release. No obligation though!

[naman1608]

Yes, that makes sense. It would cause problem when using a reactive value as skip. Like -

const [skip, setSkip] = useState(true);

const { data } = useSuspenseQuery(query, {
  skip,
  variables: { id: '123' },
  fetchPolicy: 'network-only'
});

// Later...
setSkip(false); // Problem: We'd lose variables and other options if we moved options.skip check to the top

I think the main thing is how skipToken and options.skip are perceived. As in, skipToken seems to have a purpose of completely bypassing the query execution, when we don't have the variables yet.
Whereas with options.skip, it seems like it is used to temporarily disable the execution, with the variables may or may not be set. Not really able to think how the flicker would be solved then :(

[jerelmiller]

Whereas with options.skip, it seems like it is used to temporarily disable the execution

skip: true and skipToken should behave identically as an end-user of the APIs. There really should be no difference in behavior between the two. skipToken is just more type-safe because you won't find yourself having to do something like this:

useSuspenseQuery(QUERY, {
  // type-cast to prevent TS from complaining when `id` is required
  variables: { id: id as string },
  skip: !id
})

For the test case that I'm talking about, I'm thinking something more like this:

const [skip, setSkip] = useState(true)
const [id, setId] = useState(1)

useSuspenseQuery(QUERY, {
  variables: { id },
  skip,
})

// change `id` for `variables` before we unskip
setId(2);

// then we start our fetch
setSkip(false);

We just want to make sure this case is handled where variables change before skip is set to false to ensure we aren't fetching the old variables. I don't believe we have a test case for this right now (with either skip: true or skipToken).

FWIW we do already have tests with skip as a dynamic value, so this shouldn't be the issue. Here is an example:

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

Lines 5298 to 5337 in 1e0d47d

	it("suspends when `skip` becomes `false` after it was `true`", async () => {
	const { query, mocks } = useSimpleQueryCase();
	const cache = new InMemoryCache();
	const { result, renders, rerender } = renderSuspenseHook(
	({ skip }) => useSuspenseQuery(query, { skip }),
	{ cache, mocks, initialProps: { skip: true } }
	);
	expect(renders.suspenseCount).toBe(0);
	expect(result.current).toMatchObject({
	data: undefined,
	networkStatus: NetworkStatus.ready,
	error: undefined,
	});
	rerender({ skip: false });
	expect(renders.suspenseCount).toBe(1);
	await waitFor(() => {
	expect(result.current).toMatchObject({
	...mocks[0].result,
	networkStatus: NetworkStatus.ready,
	error: undefined,
	});
	});
	expect(renders.count).toBe(3);
	expect(renders.suspenseCount).toBe(1);
	expect(renders.frames).toMatchObject([
	{ data: undefined, networkStatus: NetworkStatus.ready, error: undefined },
	{
	...mocks[0].result,
	networkStatus: NetworkStatus.ready,
	error: undefined,
	},
	]);
	});

Hope this clarifies what I'm talking about 🙂

[jerelmiller]

@naman1608 by the way, I don't want you to feel obligated to fixing this. By all means, if you want to contribute a fix, absolutely go for it! Otherwise we'll try and tackle this sometime after we release 3.12 🙂.