Fixes issue where refetching an error after an error was returned wou…

…ld hang on loading state (#11180) When an error is thrown by `useReadQuery` for a query kicked off by `useBackgroundQuery`, you can try fetching the query again using the `refetch` function. The network request would be properly kicked off, but if the result of the refetch contained another error, the hook would get stuck in the loading state. This was due to the subscription to `ObservableQuery` from `QueryReference` getting cleaned up when the `observer.error` function was run. The only reason the hook works today when refetching after an error with a successful result is because we have some [code](https://github.com/apollographql/apollo-client/blob/1c74ed4e3dc5feb5537b2aac30ab9d730fca0342/src/react/cache/QueryReference.ts#L285-L289) that catches cases where successful results could still be reported back to the hook --------- Co-authored-by: Lenz Weber-Tronic <[email protected]>
apollographql · Aug 31, 2023 · 7d9c481 · 7d9c481
1 parent 6b81981
commit 7d9c481
Show file tree

Hide file tree

Showing 7 changed files with 299 additions and 16 deletions.
diff --git a/.changeset/wise-clouds-approve.md b/.changeset/wise-clouds-approve.md
@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Fixes an issue where refetching from `useBackgroundQuery` via `refetch` with an error after an error was already fetched would get stuck in a loading state.
diff --git a/.size-limit.cjs b/.size-limit.cjs
@@ -1,7 +1,7 @@
 const checks = [
   {
     path: "dist/apollo-client.min.cjs",
-    limit: "38107",
+    limit: "38190",
   },
   {
     path: "dist/main.cjs",
@@ -10,7 +10,7 @@ const checks = [
   {
     path: "dist/index.js",
     import: "{ ApolloClient, InMemoryCache, HttpLink }",
-    limit: "31980",
+    limit: "32044",
   },
   ...[
     "ApolloProvider",

diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts
@@ -931,6 +931,32 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`,
     return this.reobserveAsConcast(newOptions, newNetworkStatus).promise;
   }
 
+  public resubscribeAfterError(
+    onNext: (value: ApolloQueryResult<TData>) => void,
+    onError?: (error: any) => void,
+    onComplete?: () => void
+  ): ObservableSubscription;
+
+  public resubscribeAfterError(
+    observer: Observer<ApolloQueryResult<TData>>
+  ): ObservableSubscription;
+
+  public resubscribeAfterError(...args: [any, any?, any?]) {
+    // If `lastError` is set in the current when the subscription is re-created,
+    // the subscription will immediately receive the error, which will
+    // cause it to terminate again. To avoid this, we first clear
+    // the last error/result from the `observableQuery` before re-starting
+    // the subscription, and restore the last value afterwards so that the
+    // subscription has a chance to stay open.
+    const last = this.last;
+    this.resetLastResults();
+
+    const subscription = this.subscribe(...args);
+    this.last = last;
+
+    return subscription;
+  }
+
   // (Re)deliver the current result to this.observers without applying fetch
   // policies or making network requests.
   private observe() {

diff --git a/src/react/cache/QueryReference.ts b/src/react/cache/QueryReference.ts
@@ -248,6 +248,12 @@ export class InternalQueryReference<TData = unknown> {
   }
 
   private handleError(error: ApolloError) {
+    this.subscription.unsubscribe();
+    this.subscription = this.observable.resubscribeAfterError(
+      this.handleNext,
+      this.handleError
+    );
+
     switch (this.status) {
       case "loading": {
         this.status = "idle";

diff --git a/src/react/hooks/__tests__/useBackgroundQuery.test.tsx b/src/react/hooks/__tests__/useBackgroundQuery.test.tsx
@@ -3503,6 +3503,258 @@ describe("useBackgroundQuery", () => {
         },
       ]);
     });
+
+    it("can refetch after error is encountered", async () => {
+      type Variables = {
+        id: string;
+      };
+
+      interface Data {
+        todo: {
+          id: string;
+          name: string;
+          completed: boolean;
+        };
+      }
+      const user = userEvent.setup();
+
+      const query: TypedDocumentNode<Data, Variables> = gql`
+        query TodoItemQuery($id: ID!) {
+          todo(id: $id) {
+            id
+            name
+            completed
+          }
+        }
+      `;
+
+      const mocks = [
+        {
+          request: { query, variables: { id: "1" } },
+          result: {
+            data: null,
+            errors: [new GraphQLError("Oops couldn't fetch")],
+          },
+          delay: 10,
+        },
+        {
+          request: { query, variables: { id: "1" } },
+          result: {
+            data: { todo: { id: "1", name: "Clean room", completed: true } },
+          },
+          delay: 10,
+        },
+      ];
+
+      const client = new ApolloClient({
+        link: new MockLink(mocks),
+        cache: new InMemoryCache(),
+      });
+
+      function App() {
+        return (
+          <ApolloProvider client={client}>
+            <Parent />
+          </ApolloProvider>
+        );
+      }
+
+      function SuspenseFallback() {
+        return <p>Loading</p>;
+      }
+
+      function Parent() {
+        const [queryRef, { refetch }] = useBackgroundQuery(query, {
+          variables: { id: "1" },
+        });
+
+        return (
+          <Suspense fallback={<SuspenseFallback />}>
+            <ErrorBoundary
+              onReset={() => refetch()}
+              fallbackRender={({ error, resetErrorBoundary }) => (
+                <>
+                  <button onClick={resetErrorBoundary}>Retry</button>
+                  <div>{error.message}</div>
+                </>
+              )}
+            >
+              <Todo queryRef={queryRef} />
+            </ErrorBoundary>
+          </Suspense>
+        );
+      }
+
+      function Todo({ queryRef }: { queryRef: QueryReference<Data> }) {
+        const {
+          data: { todo },
+        } = useReadQuery(queryRef);
+
+        return (
+          <div data-testid="todo">
+            {todo.name}
+            {todo.completed && " (completed)"}
+          </div>
+        );
+      }
+
+      render(<App />);
+
+      // Disable error message shown in the console due to an uncaught error.
+      // TODO: need to determine why the error message is logged to the console
+      // as an uncaught error since other tests do not require this.
+      const consoleSpy = jest
+        .spyOn(console, "error")
+        .mockImplementation(() => {});
+
+      expect(screen.getByText("Loading")).toBeInTheDocument();
+
+      expect(
+        await screen.findByText("Oops couldn't fetch")
+      ).toBeInTheDocument();
+
+      consoleSpy.mockRestore();
+
+      const button = screen.getByText("Retry");
+
+      await act(() => user.click(button));
+
+      expect(screen.getByText("Loading")).toBeInTheDocument();
+
+      await waitFor(() => {
+        expect(screen.getByTestId("todo")).toHaveTextContent(
+          "Clean room (completed)"
+        );
+      });
+    });
+
+    it("throws errors on refetch after error is encountered after first fetch with error", async () => {
+      type Variables = {
+        id: string;
+      };
+
+      interface Data {
+        todo: {
+          id: string;
+          name: string;
+          completed: boolean;
+        };
+      }
+      const user = userEvent.setup();
+
+      const query: TypedDocumentNode<Data, Variables> = gql`
+        query TodoItemQuery($id: ID!) {
+          todo(id: $id) {
+            id
+            name
+            completed
+          }
+        }
+      `;
+
+      const mocks = [
+        {
+          request: { query, variables: { id: "1" } },
+          result: {
+            data: null,
+            errors: [new GraphQLError("Oops couldn't fetch")],
+          },
+          delay: 10,
+        },
+        {
+          request: { query, variables: { id: "1" } },
+          result: {
+            data: null,
+            errors: [new GraphQLError("Oops couldn't fetch again")],
+          },
+          delay: 10,
+        },
+      ];
+
+      const client = new ApolloClient({
+        link: new MockLink(mocks),
+        cache: new InMemoryCache(),
+      });
+
+      function App() {
+        return (
+          <ApolloProvider client={client}>
+            <Parent />
+          </ApolloProvider>
+        );
+      }
+
+      function SuspenseFallback() {
+        return <p>Loading</p>;
+      }
+
+      function Parent() {
+        const [queryRef, { refetch }] = useBackgroundQuery(query, {
+          variables: { id: "1" },
+        });
+
+        return (
+          <Suspense fallback={<SuspenseFallback />}>
+            <ErrorBoundary
+              onReset={() => refetch()}
+              fallbackRender={({ error, resetErrorBoundary }) => (
+                <>
+                  <button onClick={resetErrorBoundary}>Retry</button>
+                  <div>{error.message}</div>
+                </>
+              )}
+            >
+              <Todo queryRef={queryRef} />
+            </ErrorBoundary>
+          </Suspense>
+        );
+      }
+
+      function Todo({ queryRef }: { queryRef: QueryReference<Data> }) {
+        const {
+          data: { todo },
+        } = useReadQuery(queryRef);
+
+        return (
+          <div data-testid="todo">
+            {todo.name}
+            {todo.completed && " (completed)"}
+          </div>
+        );
+      }
+
+      render(<App />);
+
+      // Disable error message shown in the console due to an uncaught error.
+      // TODO: need to determine why the error message is logged to the console
+      // as an uncaught error since other tests do not require this.
+      const consoleSpy = jest
+        .spyOn(console, "error")
+        .mockImplementation(() => {});
+
+      expect(screen.getByText("Loading")).toBeInTheDocument();
+
+      expect(
+        await screen.findByText("Oops couldn't fetch")
+      ).toBeInTheDocument();
+
+      const button = screen.getByText("Retry");
+
+      await act(() => user.click(button));
+
+      expect(screen.getByText("Loading")).toBeInTheDocument();
+
+      await waitFor(() => {
+        expect(
+          screen.getByText("Oops couldn't fetch again")
+        ).toBeInTheDocument();
+      });
+
+      expect(screen.queryByText("Loading")).not.toBeInTheDocument();
+
+      consoleSpy.mockRestore();
+    });
+
     it("`refetch` works with startTransition to allow React to show stale UI until finished suspending", async () => {
       type Variables = {
         id: string;

diff --git a/src/react/hooks/__tests__/useSuspenseQuery.test.tsx b/src/react/hooks/__tests__/useSuspenseQuery.test.tsx
@@ -3506,6 +3506,7 @@ describe("useSuspenseQuery", () => {
   });
 
   it("tears down subscription when throwing an error", async () => {
+    jest.useFakeTimers();
     const consoleSpy = jest.spyOn(console, "error").mockImplementation();
 
     const { query, mocks } = useErrorCase({
@@ -3523,8 +3524,14 @@ describe("useSuspenseQuery", () => {
 
     await waitFor(() => expect(renders.errorCount).toBe(1));
 
+    // The query was never retained since the error was thrown before the
+    // useEffect coud run. We need to wait for the auto dispose timeout to kick
+    // in before we check whether the observable was cleaned up
+    jest.advanceTimersByTime(30_000);
+
     expect(client.getObservableQueries().size).toBe(0);
 
+    jest.useRealTimers();
     consoleSpy.mockRestore();
   });
 

diff --git a/src/react/hooks/useQuery.ts b/src/react/hooks/useQuery.ts
@@ -209,21 +209,8 @@ class InternalState<TData, TVariables extends OperationVariables> {
           };
 
           const onError = (error: Error) => {
-            const last = obsQuery["last"];
             subscription.unsubscribe();
-            // Unfortunately, if `lastError` is set in the current
-            // `observableQuery` when the subscription is re-created,
-            // the subscription will immediately receive the error, which will
-            // cause it to terminate again. To avoid this, we first clear
-            // the last error/result from the `observableQuery` before re-starting
-            // the subscription, and restore it afterwards (so the subscription
-            // has a chance to stay open).
-            try {
-              obsQuery.resetLastResults();
-              subscription = obsQuery.subscribe(onNext, onError);
-            } finally {
-              obsQuery["last"] = last;
-            }
+            subscription = obsQuery.resubscribeAfterError(onNext, onError);
 
             if (!hasOwnProperty.call(error, "graphQLErrors")) {
               // The error is not a GraphQL error