From 2675d3c97e6c47c6e298382004c7c9c2d3ffed0c Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Fri, 3 May 2024 09:56:27 -0600 Subject: [PATCH] Fix issue where `queryRef` is recreated unnecessarily in strict mode (#11821) --- .changeset/lazy-parents-thank.md | 5 ++ .changeset/seven-forks-own.md | 6 ++ .size-limits.json | 2 +- .../__tests__/useBackgroundQuery.test.tsx | 82 +++++++++++++++++++ src/react/hooks/useBackgroundQuery.ts | 23 ++++-- src/react/hooks/useReadQuery.ts | 12 +-- src/react/hooks/useSuspenseQuery.ts | 28 ------- src/react/internal/cache/QueryReference.ts | 8 +- 8 files changed, 114 insertions(+), 52 deletions(-) create mode 100644 .changeset/lazy-parents-thank.md create mode 100644 .changeset/seven-forks-own.md diff --git a/.changeset/lazy-parents-thank.md b/.changeset/lazy-parents-thank.md new file mode 100644 index 00000000000..7e35d83112a --- /dev/null +++ b/.changeset/lazy-parents-thank.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix a regression where rerendering a component with `useBackgroundQuery` would recreate the `queryRef` instance when used with React's strict mode. diff --git a/.changeset/seven-forks-own.md b/.changeset/seven-forks-own.md new file mode 100644 index 00000000000..a8c779efa57 --- /dev/null +++ b/.changeset/seven-forks-own.md @@ -0,0 +1,6 @@ +--- +"@apollo/client": patch +--- + +Revert the change introduced in +[3.9.10](https://github.com/apollographql/apollo-client/releases/tag/v3.9.10) via #11738 that disposed of queryRefs synchronously. This change caused too many issues with strict mode. diff --git a/.size-limits.json b/.size-limits.json index b18103fdaec..855bf336313 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39551, + "dist/apollo-client.min.cjs": 39540, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32826 } diff --git a/src/react/hooks/__tests__/useBackgroundQuery.test.tsx b/src/react/hooks/__tests__/useBackgroundQuery.test.tsx index 605c5ed42c1..36fa009ba73 100644 --- a/src/react/hooks/__tests__/useBackgroundQuery.test.tsx +++ b/src/react/hooks/__tests__/useBackgroundQuery.test.tsx @@ -536,6 +536,88 @@ it("does not recreate queryRef and execute a network request when rerendering us await expect(Profiler).not.toRerender({ timeout: 50 }); }); +// https://github.com/apollographql/apollo-client/issues/11815 +it("does not recreate queryRef or execute a network request when rerendering useBackgroundQuery in strict mode", async () => { + const { query } = setupSimpleCase(); + const user = userEvent.setup(); + let fetchCount = 0; + const client = new ApolloClient({ + link: new ApolloLink(() => { + fetchCount++; + + return new Observable((observer) => { + setTimeout(() => { + observer.next({ data: { greeting: "Hello" } }); + observer.complete(); + }, 20); + }); + }), + cache: new InMemoryCache(), + }); + + const Profiler = createProfiler({ + initialSnapshot: { + queryRef: null as QueryReference | null, + result: null as UseReadQueryResult | null, + }, + }); + const { SuspenseFallback, ReadQueryHook } = + createDefaultTrackedComponents(Profiler); + + function App() { + useTrackRenders(); + const [, setCount] = React.useState(0); + // Use a fetchPolicy of no-cache to ensure we can more easily track if + // another network request was made + const [queryRef] = useBackgroundQuery(query, { fetchPolicy: "no-cache" }); + + Profiler.mergeSnapshot({ queryRef }); + + return ( + <> + + }> + + + + ); + } + + renderWithClient(, { + client, + wrapper: ({ children }) => ( + + {children} + + ), + }); + + const incrementButton = screen.getByText("Increment"); + + { + const { renderedComponents } = await Profiler.takeRender(); + + expect(renderedComponents).toStrictEqual([App, SuspenseFallback]); + } + + // eslint-disable-next-line testing-library/render-result-naming-convention + const firstRender = await Profiler.takeRender(); + const initialQueryRef = firstRender.snapshot.queryRef; + + await act(() => user.click(incrementButton)); + + { + const { snapshot } = await Profiler.takeRender(); + + expect(snapshot.queryRef).toBe(initialQueryRef); + expect(fetchCount).toBe(1); + } + + await expect(Profiler).not.toRerender({ timeout: 50 }); +}); + it("disposes of the queryRef when unmounting before it is used by useReadQuery", async () => { const { query, mocks } = setupSimpleCase(); const client = new ApolloClient({ diff --git a/src/react/hooks/useBackgroundQuery.ts b/src/react/hooks/useBackgroundQuery.ts index 0b060b2476f..601273ce627 100644 --- a/src/react/hooks/useBackgroundQuery.ts +++ b/src/react/hooks/useBackgroundQuery.ts @@ -239,16 +239,21 @@ function _useBackgroundQuery< updateWrappedQueryRef(wrappedQueryRef, promise); } - // Handle strict mode where the query ref might be disposed when useEffect - // runs twice. We add the queryRef back in the suspense cache so that the next - // render will reuse this queryRef rather than initializing a new instance. - // This also prevents issues where rerendering useBackgroundQuery after the - // queryRef has been disposed, either automatically or by unmounting - // useReadQuery will ensure the same queryRef is maintained. + // This prevents issues where rerendering useBackgroundQuery after the + // queryRef has been disposed would cause the hook to return a new queryRef + // instance since disposal also removes it from the suspense cache. We add + // the queryRef back in the suspense cache so that the next render will reuse + // this queryRef rather than initializing a new instance. React.useEffect(() => { - if (queryRef.disposed) { - suspenseCache.add(cacheKey, queryRef); - } + // Since the queryRef is disposed async via `setTimeout`, we have to wait a + // tick before checking it and adding back to the suspense cache. + const id = setTimeout(() => { + if (queryRef.disposed) { + suspenseCache.add(cacheKey, queryRef); + } + }); + + return () => clearTimeout(id); // Omitting the deps is intentional. This avoids stale closures and the // conditional ensures we aren't running the logic on each render. }); diff --git a/src/react/hooks/useReadQuery.ts b/src/react/hooks/useReadQuery.ts index 6add98a094b..0b600c59f34 100644 --- a/src/react/hooks/useReadQuery.ts +++ b/src/react/hooks/useReadQuery.ts @@ -83,17 +83,7 @@ function _useReadQuery( updateWrappedQueryRef(queryRef, internalQueryRef.promise); } - React.useEffect(() => { - // It may seem odd that we are trying to reinitialize the queryRef even - // though we reinitialize in render above, but this is necessary to - // handle strict mode where this useEffect will be run twice resulting in a - // disposed queryRef before the next render. - if (internalQueryRef.disposed) { - internalQueryRef.reinitialize(); - } - - return internalQueryRef.retain(); - }, [internalQueryRef]); + React.useEffect(() => internalQueryRef.retain(), [internalQueryRef]); const promise = useSyncExternalStore( React.useCallback( diff --git a/src/react/hooks/useSuspenseQuery.ts b/src/react/hooks/useSuspenseQuery.ts index 0459aba5087..77159eb31f2 100644 --- a/src/react/hooks/useSuspenseQuery.ts +++ b/src/react/hooks/useSuspenseQuery.ts @@ -239,34 +239,6 @@ function _useSuspenseQuery< }; }, [queryRef]); - // This effect handles the case where strict mode causes the queryRef to get - // disposed early. Previously this was done by using a `setTimeout` inside the - // dispose function, but this could cause some issues in cases where someone - // might expect the queryRef to be disposed immediately. For example, when - // using the same client instance across multiple tests in a test suite, the - // `setTimeout` has the possibility of retaining the suspense cache entry for - // too long, which means that two tests might accidentally share the same - // `queryRef` instance. By immediately disposing, we can avoid this situation. - // - // Instead we can leverage the work done to allow the queryRef to "resume" - // after it has been disposed without executing an additional network request. - // This is done by calling the `initialize` function below. - React.useEffect(() => { - if (queryRef.disposed) { - // Calling the `dispose` function removes it from the suspense cache, so - // when the component rerenders, it instantiates a fresh query ref. - // We address this by adding the queryRef back to the suspense cache - // so that the lookup on the next render uses the existing queryRef rather - // than instantiating a new one. - suspenseCache.add(cacheKey, queryRef); - queryRef.reinitialize(); - } - // We can omit the deps here to get a fresh closure each render since the - // conditional will prevent the logic from running in most cases. This - // should also be a touch faster since it should be faster to check the `if` - // statement than for React to compare deps on this effect. - }); - const skipResult = React.useMemo(() => { const error = toApolloError(queryRef.result); diff --git a/src/react/internal/cache/QueryReference.ts b/src/react/internal/cache/QueryReference.ts index f3bd6395b8c..16a014d0d89 100644 --- a/src/react/internal/cache/QueryReference.ts +++ b/src/react/internal/cache/QueryReference.ts @@ -244,9 +244,11 @@ export class InternalQueryReference { disposed = true; this.references--; - if (!this.references) { - this.dispose(); - } + setTimeout(() => { + if (!this.references) { + this.dispose(); + } + }); }; }