Allow queryRefs to be disposed of synchronously (#11738)

.api-reports/api-report-react_internal.md

@@ -1731,6 +1731,8 @@ interface SubscriptionOptions<TVariables = OperationVariables, TData = any> {
 class SuspenseCache {
     constructor(options?: SuspenseCacheOptions);
     // (undocumented)
+    add(cacheKey: CacheKey, queryRef: InternalQueryReference<unknown>): void;
+    // (undocumented)
     getQueryRef<TData = any>(cacheKey: CacheKey, createObservable: () => ObservableQuery<TData>): InternalQueryReference<TData>;
 }
 

.changeset/fuzzy-grapes-pump.md

@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Fix an issue where rerendering `useBackgroundQuery` after the `queryRef` had been disposed, either via the auto dispose timeout or by unmounting `useReadQuery`, would cause the `queryRef` to be recreated potentially resulting in another network request.

.changeset/lovely-mayflies-grab.md

@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Allow queryRefs to be disposed of synchronously when a suspense hook unmounts. This prevents some situations where using a suspense hook with the same query/variables as the disposed queryRef accidentally used the disposed queryRef rather than creating a new instance.

.size-limits.json

@@ -1,4 +1,4 @@
 {
-  "dist/apollo-client.min.cjs": 39325,
+  "dist/apollo-client.min.cjs": 39368,
   "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32634
 }

src/react/hooks/__tests__/useBackgroundQuery.test.tsx

@@ -410,7 +410,10 @@ it("auto resubscribes when mounting useReadQuery after naturally disposed by use
   await wait(0);
 
   expect(client.getObservableQueries().size).toBe(0);
-  expect(client).not.toHaveSuspenseCacheEntryUsing(query);
+  // We retain the cache entry in useBackgroundQuery to avoid recreating the
+  // queryRef if useBackgroundQuery rerenders before useReadQuery is mounted
+  // again.
+  expect(client).toHaveSuspenseCacheEntryUsing(query);
 
   await act(() => user.click(toggleButton));
 
@@ -447,6 +450,92 @@ it("auto resubscribes when mounting useReadQuery after naturally disposed by use
   await expect(Profiler).not.toRerender({ timeout: 50 });
 });
 
+it("does not recreate queryRef and execute a network request when rerendering useBackgroundQuery after queryRef is disposed", 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 = createDefaultProfiler<SimpleCaseData>();
+  const { SuspenseFallback, ReadQueryHook } =
+    createDefaultTrackedComponents(Profiler);
+
+  function App() {
+    useTrackRenders();
+    const [show, setShow] = React.useState(true);
+    // 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" });
+
+    return (
+      <>
+        <button onClick={() => setShow((show) => !show)}>Toggle</button>
+        <Suspense fallback={<SuspenseFallback />}>
+          {show && <ReadQueryHook queryRef={queryRef} />}
+        </Suspense>
+      </>
+    );
+  }
+
+  const { rerender } = renderWithClient(<App />, { client, wrapper: Profiler });
+
+  const toggleButton = screen.getByText("Toggle");
+
+  {
+    const { renderedComponents } = await Profiler.takeRender();
+
+    expect(renderedComponents).toStrictEqual([App, SuspenseFallback]);
+  }
+
+  {
+    const { snapshot } = await Profiler.takeRender();
+
+    expect(snapshot.result).toEqual({
+      data: { greeting: "Hello" },
+      error: undefined,
+      networkStatus: NetworkStatus.ready,
+    });
+  }
+
+  await act(() => user.click(toggleButton));
+  await Profiler.takeRender();
+  await wait(0);
+
+  rerender(<App />);
+  await Profiler.takeRender();
+
+  expect(fetchCount).toBe(1);
+
+  await act(() => user.click(toggleButton));
+
+  {
+    const { snapshot, renderedComponents } = await Profiler.takeRender();
+
+    expect(renderedComponents).toStrictEqual([App, ReadQueryHook]);
+    expect(snapshot.result).toEqual({
+      data: { greeting: "Hello" },
+      error: undefined,
+      networkStatus: NetworkStatus.ready,
+    });
+  }
+
+  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({
@@ -3672,6 +3761,79 @@ it('does not suspend deferred queries with partial data in the cache and using a
   await expect(Profiler).not.toRerender({ timeout: 50 });
 });
 
+it.each<SuspenseQueryHookFetchPolicy>([
+  "cache-first",
+  "network-only",
+  "cache-and-network",
+])(
+  'responds to cache updates in strict mode while using a "%s" fetch policy',
+  async (fetchPolicy) => {
+    const { query, mocks } = setupSimpleCase();
+
+    const client = new ApolloClient({
+      cache: new InMemoryCache(),
+      link: new MockLink(mocks),
+    });
+
+    const Profiler = createDefaultProfiler<SimpleCaseData>();
+    const { SuspenseFallback, ReadQueryHook } =
+      createDefaultTrackedComponents(Profiler);
+
+    function App() {
+      useTrackRenders();
+      const [queryRef] = useBackgroundQuery(query, { fetchPolicy });
+
+      return (
+        <Suspense fallback={<SuspenseFallback />}>
+          <ReadQueryHook queryRef={queryRef} />
+        </Suspense>
+      );
+    }
+
+    renderWithClient(<App />, {
+      client,
+      wrapper: ({ children }) => (
+        <React.StrictMode>
+          <Profiler>{children}</Profiler>
+        </React.StrictMode>
+      ),
+    });
+
+    {
+      const { renderedComponents } = await Profiler.takeRender();
+
+      expect(renderedComponents).toStrictEqual([App, SuspenseFallback]);
+    }
+
+    {
+      const { snapshot } = await Profiler.takeRender();
+
+      expect(snapshot.result).toEqual({
+        data: { greeting: "Hello" },
+        error: undefined,
+        networkStatus: NetworkStatus.ready,
+      });
+    }
+
+    client.writeQuery({
+      query,
+      data: { greeting: "Updated hello" },
+    });
+
+    {
+      const { snapshot } = await Profiler.takeRender();
+
+      expect(snapshot.result).toEqual({
+        data: { greeting: "Updated hello" },
+        error: undefined,
+        networkStatus: NetworkStatus.ready,
+      });
+    }
+
+    await expect(Profiler).not.toRerender({ timeout: 50 });
+  }
+);
+
 describe("refetch", () => {
   it("re-suspends when calling `refetch`", async () => {
     const { query, mocks: defaultMocks } = setupVariablesCase();

src/react/hooks/useBackgroundQuery.ts

@@ -239,6 +239,20 @@ 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.
+  React.useEffect(() => {
+    if (queryRef.disposed) {
+      suspenseCache.add(cacheKey, queryRef);
+    }
+    // Omitting the deps is intentional. This avoids stale closures and the
+    // conditional ensures we aren't running the logic on each render.
+  });
+
   const fetchMore: FetchMoreFunction<TData, TVariables> = React.useCallback(
     (options) => {
       const promise = queryRef.fetchMore(options as FetchMoreQueryOptions<any>);

src/react/hooks/useReadQuery.ts

@@ -64,7 +64,17 @@ function _useReadQuery<TData>(
     updateWrappedQueryRef(queryRef, internalQueryRef.promise);
   }
 
-  React.useEffect(() => internalQueryRef.retain(), [internalQueryRef]);
+  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]);
 
   const promise = useSyncExternalStore(
     React.useCallback(

src/react/hooks/useSuspenseQuery.ts

@@ -239,6 +239,34 @@ 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

@@ -242,12 +242,9 @@ export class InternalQueryReference<TData = unknown> {
       disposed = true;
       this.references--;
 
-      // Wait before fully disposing in case the app is running in strict mode.
-      setTimeout(() => {
-        if (!this.references) {
-          this.dispose();
-        }
-      });
+      if (!this.references) {
+        this.dispose();
+      }
     };
   }
 

src/react/internal/cache/SuspenseCache.ts

@@ -47,4 +47,9 @@ export class SuspenseCache {
 
     return ref.current;
   }
+
+  add(cacheKey: CacheKey, queryRef: InternalQueryReference<unknown>) {
+    const ref = this.queryRefs.lookupArray(cacheKey);
+    ref.current = queryRef;
+  }
 }