Fix issue where queryRef is recreated unnecessarily in strict mode (#11821)

Author: jerelmiller

.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.

.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.

.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
 }

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<SimpleCaseData> | null,
+      result: null as UseReadQueryResult<SimpleCaseData> | 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 (
+      <>
+        <button onClick={() => setCount((count) => count + 1)}>
+          Increment
+        </button>
+        <Suspense fallback={<SuspenseFallback />}>
+          <ReadQueryHook queryRef={queryRef} />
+        </Suspense>
+      </>
+    );
+  }
+
+  renderWithClient(<App />, {
+    client,
+    wrapper: ({ children }) => (
+      <Profiler>
+        <React.StrictMode>{children}</React.StrictMode>
+      </Profiler>
+    ),
+  });
+
+  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({

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.
   });

src/react/hooks/useReadQuery.ts

@@ -83,17 +83,7 @@ function _useReadQuery<TData>(
     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(

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);
 

src/react/internal/cache/QueryReference.ts

@@ -244,9 +244,11 @@ export class InternalQueryReference<TData = unknown> {
       disposed = true;
       this.references--;
 
-      if (!this.references) {
-        this.dispose();
-      }
+      setTimeout(() => {
+        if (!this.references) {
+          this.dispose();
+        }
+      });
     };
   }