Remove double initialization and unneeded useLazyRef from useFragment to avoid write to ref in render

[jerelmiller]

While working on data masking with fragments, I stumbled upon a small optimization that we can make in useFragment. This hook uses cache.diff directly in order to get an initial value returned from the first execution of the hook. This initial value was set using useLazyRef. We also had a bit of code within a useMemo that also set the ref's value to handle when options change to ensure we were able to get a synchronous value that way. This useMemo means we don't actually need that useLazyRef since the useMemo will also initialize the ref value if its not already set.

By replacing useLazyRef with useRef, we were able to remove the useLazyRef hook entirely and inline the creation of diff options in the same useMemo call. This reduces a bit of bundle size as a result.

[changeset-bot]

🦋 Changeset detected

Latest commit: 49dcf2e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	39.31 KB (-0.05% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	47.96 KB (-0.08% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	45.54 KB (-0.07% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	34.4 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	32.28 KB (0%)
import { ApolloProvider } from "dist/react/index.js"	1.26 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.24 KB (0%)
import { useQuery } from "dist/react/index.js"	5.21 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.3 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	5.69 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.77 KB (0%)
import { useMutation } from "dist/react/index.js"	3.62 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.84 KB (0%)
import { useSubscription } from "dist/react/index.js"	4.41 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	3.46 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	5.49 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.15 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	4.99 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.64 KB (-0.03% 🔽)
import { useLoadableQuery } from "dist/react/index.js"	5.07 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.72 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.39 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	3.33 KB (0%)
import { useFragment } from "dist/react/index.js"	2.3 KB (-0.93% 🔽)
import { useFragment } from "dist/react/index.js" (production)	2.24 KB (-1% 🔽)

[netlify]

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	49dcf2e
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/66cddee5c32f530008ce8b23
😎 Deploy Preview	https://deploy-preview-12020--apollo-client-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[phryneas]

What do you think about this?

Suggested change

	// Since .next is async, we need to make sure that we
	// get the correct diff on the next render given new diffOptions
	React.useMemo(() => {
	const {
	fragment,
	fragmentName,
	from,
	optimistic = true,
	...rest
	} = options;
	return {
	...rest,
	returnPartialData: true,
	id: typeof from === "string" ? from : cache.identify(from),
	query: cache["getFragmentDoc"](fragment, fragmentName),
	optimistic,
	};
	}, [options]);
	} = stableOptions;
	const resultRef = useLazyRef<UseFragmentResult<TData>>(() =>
	diffToResult(cache.diff<TData>(diffOptions))
	);
	const stableOptions = useDeepMemo(() => options, [options]);
	// Since .next is async, we need to make sure that we
	// get the correct diff on the next render given new diffOptions
	React.useMemo(() => {
	resultRef.current = diffToResult(cache.diff<TData>(diffOptions));
	}, [diffOptions, cache]);
	resultRef.current = diffToResult(
	cache.diff<TData>({
	...rest,
	returnPartialData: true,
	id: typeof from === "string" ? from : cache.identify(from),
	query: cache["getFragmentDoc"](fragment, fragmentName),
	optimistic,
	})
	);
	}, [stableOptions, cache]);
	// Used for both getSnapshot and getServerSnapshot
	const getSnapshot = React.useCallback(() => resultRef.current, []);
	const getSnapshot = React.useCallback(() => resultRef.current!, []);
	return useSyncExternalStore(
	React.useCallback(
	// Since .next is async, we need to make sure that we
	// get the correct diff on the next render given new diffOptions
	const currentDiff = React.useMemo(() => {
	const {
	fragment,
	fragmentName,
	from,
	optimistic = true,
	...rest
	} = stableOptions;
	return diffToResult(
	cache.diff<TData>({
	...rest,
	returnPartialData: true,
	id: typeof from === "string" ? from : cache.identify(from),
	query: cache["getFragmentDoc"](fragment, fragmentName),
	optimistic,
	})
	);
	}, [stableOptions, cache]);
	// Used for both getSnapshot and getServerSnapshot
	const getSnapshot = React.useCallback(
	() => resultRef.current || currentDiff,
	[currentDiff]
	);
	return useSyncExternalStore(
	React.useCallback(

and then in the cleanup

        return () => {
          resultRef.current = undefined;
          subscription.unsubscribe();
          clearTimeout(lastTimeout);
        };

[jerelmiller]

Looks like that causes some tests failures. I'll dig in a bit more after planning to try and understand why this change causes it because I like this change a lot.

[jerelmiller]

Ok seems this fails so many tests because the check to determine if the current result matches the last one fails on the initial render:

if (equal(result, resultRef.current)) return;

With this change in place, resultRef.current is undefined, so this check will always fail, therefore we get an extra render since the hook thinks the result is different.

This check is really only used for the first result emitted from watchFragment, specifically for this case of initialization since watchFragment emits its initial value asynchronously and we want to avoid an extra render for the initial value.

Let me see if comparing against currentResult in uSES (which means adding it to the dependency array) causes any failures here.

[phryneas]

That looks good to me now :)