Fixed useQuery under StrictMode creating 2 observable query

.changeset/plenty-wolves-fetch.md

@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Fixed issue under StrictMode causing queries created with useQuery to be potentially refetched after unmounting.

.vscode/settings.json

@@ -5,6 +5,9 @@
   "files.trimTrailingWhitespace": true,
   "files.insertFinalNewline": true,
   "typescript.tsdk": "node_modules/typescript/lib",
+  "editor.codeActionsOnSave": {
+    "source.organizeImports": "never"
+  },
   "cSpell.enableFiletypes": ["mdx"],
   "jest.jestCommandLine": "node_modules/.bin/jest --config ./config/jest.config.js --ignoreProjects 'ReactDOM 17' --runInBand"
 }

src/core/ApolloClient.ts

@@ -415,6 +415,17 @@ export class ApolloClient<TCacheShape> implements DataProxy {
     T = any,
     TVariables extends OperationVariables = OperationVariables,
   >(options: WatchQueryOptions<TVariables, T>): ObservableQuery<T, TVariables> {
+    const { observable, register } = this.createQuery(options);
+    register();
+    return observable;
+  }
+
+  public createQuery<
+    T = any,
+    TVariables extends OperationVariables = OperationVariables,
+  >(
+    options: WatchQueryOptions<TVariables, T>
+  ): { observable: ObservableQuery<T, TVariables>; register: () => void } {
     if (this.defaultOptions.watchQuery) {
       options = mergeOptions(this.defaultOptions.watchQuery, options);
     }
@@ -428,7 +439,16 @@ export class ApolloClient<TCacheShape> implements DataProxy {
       options = { ...options, fetchPolicy: "cache-first" };
     }
 
-    return this.queryManager.watchQuery<T, TVariables>(options);
+    const { observable, queryInfo } = this.queryManager.createQuery<
+      T,
+      TVariables
+    >(options);
+    return {
+      observable,
+      register: () => {
+        this.queryManager.registerObservableQuery(observable, queryInfo);
+      },
+    };
   }
 
   /**

src/core/QueryInfo.ts

@@ -287,11 +287,17 @@ export class QueryInfo {
   }
 
   private shouldNotify() {
-    if (!this.dirty || !this.listeners.size) {
+    if (
+      !this.dirty ||
+      !this.listeners.size ||
+      // It's possible that the query is no longer being watched, but the
+      // ObservableQuery is still active/pending cleanup. In this case, we should not notify.
+      !this.observableQuery?.hasObservers()
+    ) {
       return false;
     }
 
-    if (isNetworkRequestInFlight(this.networkStatus) && this.observableQuery) {
+    if (isNetworkRequestInFlight(this.networkStatus)) {
       const { fetchPolicy } = this.observableQuery.options;
       if (fetchPolicy !== "cache-only" && fetchPolicy !== "cache-and-network") {
         return false;

src/core/QueryManager.ts

@@ -720,10 +720,16 @@ export class QueryManager<TStore> {
     };
   }
 
-  public watchQuery<
+  /**
+   * Create a query, but do not associate it with the QueryManager.
+   * This allows to throw away the query if it ends up not being needed
+   */
+  public createQuery<
     T,
     TVariables extends OperationVariables = OperationVariables,
-  >(options: WatchQueryOptions<TVariables, T>): ObservableQuery<T, TVariables> {
+  >(
+    options: WatchQueryOptions<TVariables, T>
+  ): { observable: ObservableQuery<T, TVariables>; queryInfo: QueryInfo } {
     const query = this.transform(options.query);
 
     // assign variable default values if supplied
@@ -746,8 +752,6 @@ export class QueryManager<TStore> {
     });
     observable["lastQuery"] = query;
 
-    this.queries.set(observable.queryId, queryInfo);
-
     // We give queryInfo the transformed query to ensure the first cache diff
     // uses the transformed query instead of the raw query
     queryInfo.init({
@@ -756,6 +760,25 @@ export class QueryManager<TStore> {
       variables: observable.variables,
     });
 
+    return { observable, queryInfo };
+  }
+
+  /**
+   * Register an ObservableQuery with the QueryManager created previously with createQuery.
+   */
+  public registerObservableQuery(
+    observable: ObservableQuery<any, any>,
+    queryInfo: QueryInfo
+  ): void {
+    this.queries.set(observable.queryId, queryInfo);
+  }
+
+  public watchQuery<
+    T,
+    TVariables extends OperationVariables = OperationVariables,
+  >(options: WatchQueryOptions<TVariables, T>): ObservableQuery<T, TVariables> {
+    const { observable, queryInfo } = this.createQuery(options);
+    this.registerObservableQuery(observable, queryInfo);
     return observable;
   }
 

src/core/__tests__/QueryManager/index.ts

@@ -4712,6 +4712,51 @@ describe("QueryManager", () => {
           .then(resolve, reject);
       }
     );
+
+    itAsync(
+      "will not update inactive query on `resetStore` nor on `refetchQueries`",
+      (resolve, reject) => {
+        const testQuery = gql`
+          query TestQuery {
+            author {
+              firstName
+              lastName
+            }
+          }
+        `;
+        const link = new (class extends ApolloLink {
+          public request() {
+            reject(new Error("Query was not supposed to be called"));
+            return null;
+          }
+        })();
+
+        const queryManager = new QueryManager(
+          getDefaultOptionsForQueryManagerTests({
+            link,
+            cache: new InMemoryCache({ addTypename: false }),
+          })
+        );
+        const oq = queryManager.watchQuery({
+          query: testQuery,
+          fetchPolicy: "cache-and-network",
+        });
+        // Recreate state where an observable query is dirty but has no observers in the query manager
+        // @ts-expect-error -- Accessing private field for testing
+        oq.queryInfo.dirty = true;
+
+        resetStore(queryManager).then((q) => {
+          expect(q).toHaveLength(0);
+          expect(oq.hasObservers()).toBe(false);
+          resolve();
+        });
+
+        const refetched = queryManager.refetchQueries({
+          include: ["TestQuery"],
+        });
+        expect(refetched.size).toBe(0);
+      }
+    );
   });
 
   describe("loading state", () => {

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

@@ -1,7 +1,7 @@
 import React, { Fragment, ReactNode, useEffect, useRef, useState } from "react";
 import { DocumentNode, GraphQLError, GraphQLFormattedError } from "graphql";
 import gql from "graphql-tag";
-import { act } from "@testing-library/react";
+import { act, configure } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 import { render, screen, waitFor, renderHook } from "@testing-library/react";
 import {
@@ -1542,7 +1542,7 @@ describe("useQuery Hook", () => {
 
       function checkObservableQueries(expectedLinkCount: number) {
         const obsQueries = client.getObservableQueries("all");
-        expect(obsQueries.size).toBe(2);
+        expect(obsQueries.size).toBe(1);
 
         const activeSet = new Set<typeof result.current.observable>();
         const inactiveSet = new Set<typeof result.current.observable>();
@@ -1597,6 +1597,72 @@ describe("useQuery Hook", () => {
 
       checkObservableQueries(2);
     });
+
+    it("only refetch active queries", async () => {
+      const query1 = gql`
+        {
+          user {
+            id
+            name
+            count
+          }
+        }
+      `;
+      const query2 = gql`
+        {
+          user {
+            id
+            name
+          }
+        }
+      `;
+      let count = 0;
+      let replyData = (): object => ({
+        data: { user: { id: "1", name: "Alice", count: ++count } },
+      });
+      const cache = new InMemoryCache();
+      const client = new ApolloClient({
+        link: new ApolloLink(() => Observable.of(replyData())),
+        cache,
+      });
+
+      function Query({ query }: { query: DocumentNode }) {
+        const { data } = useQuery(query, { fetchPolicy: "cache-and-network" });
+        return data?.user ?
+            <div>
+              {data.user.name}: {data.user.count}
+            </div>
+          : null;
+      }
+
+      function Component({ query }: { query: DocumentNode }) {
+        return (
+          <React.StrictMode>
+            <ApolloProvider client={client}>
+              <Query query={query} />
+            </ApolloProvider>
+          </React.StrictMode>
+        );
+      }
+
+      const { unmount } = render(<Component query={query1} />);
+      await screen.findAllByText("Alice: 1");
+      expect(client.getObservableQueries("all").size).toBe(1);
+      unmount();
+
+      await new Promise((resolve) => setTimeout(resolve));
+      expect(client.getObservableQueries("all").size).toBe(0);
+
+      replyData = () => ({ data: { user: { id: "1", name: "Alice" } } });
+      render(<Component query={query2} />, {});
+
+      expect(client.getObservableQueries("all").size).toBe(1);
+      await act(async () => {
+        const refetched = await client.reFetchObservableQueries();
+        expect(refetched).toHaveLength(1);
+        expect(refetched[0].data).toEqual({ user: { id: "1", name: "Alice" } });
+      });
+    });
   });
 
   describe("polling", () => {

src/react/hooks/useQuery.ts

@@ -85,6 +85,7 @@ interface InternalState<TData, TVariables extends OperationVariables> {
   client: ReturnType<typeof useApolloClient>;
   query: DocumentNode | TypedDocumentNode<TData, TVariables>;
   observable: ObsQueryWithMeta<TData, TVariables>;
+  registerObservableQuery: () => void;
   resultData: InternalResult<TData, TVariables>;
 }
 
@@ -179,18 +180,29 @@ function useInternalState<
   function createInternalState(previous?: InternalState<TData, TVariables>) {
     verifyDocumentType(query, DocumentType.Query);
 
+    // See if there is an existing observable that was used to fetch the same
+    // data and if so, use it instead since it will contain the proper queryId
+    // to fetch the result set. This is used during SSR.
+    const ssrObservable = renderPromises?.getSSRObservable(
+      makeWatchQueryOptions()
+    );
+    let { observable, register } =
+      ssrObservable ?
+        { observable: ssrObservable, register: () => {} }
+      : client.createQuery(
+          getObsQueryOptions(void 0, client, options, makeWatchQueryOptions())
+        );
+    if (renderPromises && !ssrObservable) {
+      // In SSR, useEffects are not called, so we need to register the observable now
+      register();
+      register = () => {};
+    }
+
     const internalState: InternalState<TData, TVariables> = {
       client,
       query,
-      observable:
-        // See if there is an existing observable that was used to fetch the same
-        // data and if so, use it instead since it will contain the proper queryId
-        // to fetch the result set. This is used during SSR.
-        (renderPromises &&
-          renderPromises.getSSRObservable(makeWatchQueryOptions())) ||
-        client.watchQuery(
-          getObsQueryOptions(void 0, client, options, makeWatchQueryOptions())
-        ),
+      observable,
+      registerObservableQuery: register,
       resultData: {
         // Reuse previousData from previous InternalState (if any) to provide
         // continuity of previousData even if/when the query or client changes.
@@ -204,6 +216,13 @@ function useInternalState<
   let [internalState, updateInternalState] =
     React.useState(createInternalState);
 
+  // Creating a watcher/listener in a `useState` breaks the rules of React
+  // useState initializers should be pure, aka, the result should be throwable and recreatable
+  // Instead of registering the observable with the QueryManager in the `useState` initializer, we create it in a `useEffect` hook
+  // The actual subscription of the query is handled in the `useObservableSubscriptionResult` hook
+  const registerObservableQuery = internalState.registerObservableQuery;
+  React.useEffect(registerObservableQuery, [registerObservableQuery]);
+
   /**
    * Used by `useLazyQuery` when a new query is executed.
    * We keep this logic here since it needs to update things in unsafe