From 82d8cb4255be497748829f12eb25ac87c11ee5e4 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Tue, 27 Aug 2024 09:20:10 -0600 Subject: [PATCH] Remove double initialization and unneeded useLazyRef from useFragment to avoid write to ref in render (#12020) --- .changeset/tough-geese-sing.md | 5 ++ .size-limits.json | 2 +- src/react/hooks/internal/index.ts | 1 - src/react/hooks/internal/useLazyRef.ts | 13 ------ src/react/hooks/useFragment.ts | 63 ++++++++++++++------------ 5 files changed, 41 insertions(+), 43 deletions(-) create mode 100644 .changeset/tough-geese-sing.md delete mode 100644 src/react/hooks/internal/useLazyRef.ts diff --git a/.changeset/tough-geese-sing.md b/.changeset/tough-geese-sing.md new file mode 100644 index 00000000000..50420a30cda --- /dev/null +++ b/.changeset/tough-geese-sing.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Better conform to Rules of React by avoiding write of ref in render for `useFragment`. diff --git a/.size-limits.json b/.size-limits.json index e8bbc744738..8965193f2fe 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 40271, + "dist/apollo-client.min.cjs": 40252, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 33059 } diff --git a/src/react/hooks/internal/index.ts b/src/react/hooks/internal/index.ts index ce58b546f69..85aad7cb932 100644 --- a/src/react/hooks/internal/index.ts +++ b/src/react/hooks/internal/index.ts @@ -2,6 +2,5 @@ export { useDeepMemo } from "./useDeepMemo.js"; export { useIsomorphicLayoutEffect } from "./useIsomorphicLayoutEffect.js"; export { useRenderGuard } from "./useRenderGuard.js"; -export { useLazyRef } from "./useLazyRef.js"; export { __use } from "./__use.js"; export { wrapHook } from "./wrapHook.js"; diff --git a/src/react/hooks/internal/useLazyRef.ts b/src/react/hooks/internal/useLazyRef.ts deleted file mode 100644 index 2656a2773a4..00000000000 --- a/src/react/hooks/internal/useLazyRef.ts +++ /dev/null @@ -1,13 +0,0 @@ -import * as React from "rehackt"; - -const INIT = {}; - -export function useLazyRef(getInitialValue: () => T) { - const ref = React.useRef(INIT as unknown as T); - - if (ref.current === INIT) { - ref.current = getInitialValue(); - } - - return ref; -} diff --git a/src/react/hooks/useFragment.ts b/src/react/hooks/useFragment.ts index 6f94e8edaac..9cc56f05908 100644 --- a/src/react/hooks/useFragment.ts +++ b/src/react/hooks/useFragment.ts @@ -12,7 +12,7 @@ import { useApolloClient } from "./useApolloClient.js"; import { useSyncExternalStore } from "./useSyncExternalStore.js"; import type { ApolloClient, OperationVariables } from "../../core/index.js"; import type { NoInfer } from "../types/types.js"; -import { useDeepMemo, useLazyRef, wrapHook } from "./internal/index.js"; +import { useDeepMemo, wrapHook } from "./internal/index.js"; import equal from "@wry/equality"; export interface UseFragmentOptions @@ -64,39 +64,41 @@ function _useFragment( options: UseFragmentOptions ): UseFragmentResult { const { cache } = useApolloClient(options.client); + const { from, ...rest } = options; - const diffOptions = useDeepMemo>(() => { - 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]); - - const resultRef = useLazyRef>(() => - diffToResult(cache.diff(diffOptions)) + // We calculate the cache id seperately from `stableOptions` because we don't + // want changes to non key fields in the `from` property to affect + // `stableOptions` and retrigger our subscription. If the cache identifier + // stays the same between renders, we want to reuse the existing subscription. + const id = React.useMemo( + () => (typeof from === "string" ? from : cache.identify(from)), + [cache, from] ); - const stableOptions = useDeepMemo(() => options, [options]); + const resultRef = React.useRef>(); + const stableOptions = useDeepMemo(() => ({ ...rest, from: id! }), [rest, id]); // 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(diffOptions)); - }, [diffOptions, cache]); + const currentDiff = React.useMemo(() => { + const { fragment, fragmentName, from, optimistic = true } = stableOptions; + + return diffToResult( + cache.diff({ + ...stableOptions, + returnPartialData: true, + id: 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 || currentDiff, + [currentDiff] + ); return useSyncExternalStore( React.useCallback( @@ -104,7 +106,11 @@ function _useFragment( let lastTimeout = 0; const subscription = cache.watchFragment(stableOptions).subscribe({ next: (result) => { - if (equal(result, resultRef.current)) return; + // Since `next` is called async by zen-observable, we want to avoid + // unnecessarily rerendering this hook for the initial result + // emitted from watchFragment which should be equal to + // `currentDiff`. + if (equal(result, currentDiff)) return; resultRef.current = result; // If we get another update before we've re-rendered, bail out of // the update and try again. This ensures that the relative timing @@ -115,11 +121,12 @@ function _useFragment( }, }); return () => { + resultRef.current = void 0; subscription.unsubscribe(); clearTimeout(lastTimeout); }; }, - [cache, stableOptions] + [cache, stableOptions, currentDiff] ), getSnapshot, getSnapshot