Don't call onError if errors are thrown in onCompleted (#12174)

apollographql · Dec 6, 2024 · ba5cc33 · ba5cc33
1 parent 8dfc90d
commit ba5cc33
Show file tree

Hide file tree

Showing 5 changed files with 137 additions and 67 deletions.
diff --git a/.changeset/fluffy-worms-fail.md b/.changeset/fluffy-worms-fail.md
@@ -0,0 +1,5 @@
+---
+"@apollo/client": minor
+---
+
+Ensure errors thrown in the `onCompleted` callback from `useMutation` don't call `onError`.
diff --git a/.changeset/sharp-windows-switch.md b/.changeset/sharp-windows-switch.md
@@ -0,0 +1,5 @@
+---
+"@apollo/client": minor
+---
+
+Reject the mutation promise if errors are thrown in the `onCompleted` callback of `useMutation`.
diff --git a/.size-limits.json b/.size-limits.json
@@ -1,4 +1,4 @@
 {
-  "dist/apollo-client.min.cjs": 41615,
+  "dist/apollo-client.min.cjs": 41613,
   "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 34349
 }
diff --git a/src/react/hooks/__tests__/useMutation.test.tsx b/src/react/hooks/__tests__/useMutation.test.tsx
@@ -1207,6 +1207,61 @@ describe("useMutation Hook", () => {
         expect.objectContaining({ variables })
       );
     });
+
+    // https://github.com/apollographql/apollo-client/issues/12008
+    it("does not call onError if errors are thrown in the onCompleted callback", async () => {
+      const CREATE_TODO_DATA = {
+        createTodo: {
+          id: 1,
+          priority: "Low",
+          description: "Get milk!",
+          __typename: "Todo",
+        },
+      };
+
+      const variables = {
+        priority: "Low",
+        description: "Get milk2.",
+      };
+
+      const mocks = [
+        {
+          request: {
+            query: CREATE_TODO_MUTATION,
+            variables,
+          },
+          result: {
+            data: CREATE_TODO_DATA,
+          },
+        },
+      ];
+
+      const onError = jest.fn();
+
+      using _disabledAct = disableActEnvironment();
+      const { takeSnapshot } = await renderHookToSnapshotStream(
+        () =>
+          useMutation(CREATE_TODO_MUTATION, {
+            onCompleted: () => {
+              throw new Error("Oops");
+            },
+            onError,
+          }),
+        {
+          wrapper: ({ children }) => (
+            <MockedProvider mocks={mocks}>{children}</MockedProvider>
+          ),
+        }
+      );
+
+      const [createTodo] = await takeSnapshot();
+
+      await expect(createTodo({ variables })).rejects.toEqual(
+        new Error("Oops")
+      );
+
+      expect(onError).not.toHaveBeenCalled();
+    });
   });
 
   describe("ROOT_MUTATION cache data", () => {

diff --git a/src/react/hooks/useMutation.ts b/src/react/hooks/useMutation.ts
@@ -138,82 +138,87 @@ export function useMutation<
 
       return client
         .mutate(clientOptions as MutationOptions<TData, OperationVariables>)
-        .then((response) => {
-          const { data, errors } = response;
-          const error =
-            errors && errors.length > 0 ?
-              new ApolloError({ graphQLErrors: errors })
-            : void 0;
-
-          const onError =
-            executeOptions.onError || ref.current.options?.onError;
-
-          if (error && onError) {
-            onError(
-              error,
-              clientOptions as MutationOptions<TData, OperationVariables>
-            );
-          }
+        .then(
+          (response) => {
+            const { data, errors } = response;
+            const error =
+              errors && errors.length > 0 ?
+                new ApolloError({ graphQLErrors: errors })
+              : void 0;
+
+            const onError =
+              executeOptions.onError || ref.current.options?.onError;
+
+            if (error && onError) {
+              onError(
+                error,
+                clientOptions as MutationOptions<TData, OperationVariables>
+              );
+            }
 
-          if (
-            mutationId === ref.current.mutationId &&
-            !clientOptions.ignoreResults
-          ) {
-            const result = {
-              called: true,
-              loading: false,
-              data,
-              error,
-              client,
-            };
-
-            if (ref.current.isMounted && !equal(ref.current.result, result)) {
-              setResult((ref.current.result = result));
+            if (
+              mutationId === ref.current.mutationId &&
+              !clientOptions.ignoreResults
+            ) {
+              const result = {
+                called: true,
+                loading: false,
+                data,
+                error,
+                client,
+              };
+
+              if (ref.current.isMounted && !equal(ref.current.result, result)) {
+                setResult((ref.current.result = result));
+              }
             }
-          }
 
-          const onCompleted =
-            executeOptions.onCompleted || ref.current.options?.onCompleted;
+            const onCompleted =
+              executeOptions.onCompleted || ref.current.options?.onCompleted;
 
-          if (!error) {
-            onCompleted?.(
-              response.data!,
-              clientOptions as MutationOptions<TData, OperationVariables>
-            );
-          }
+            if (!error) {
+              onCompleted?.(
+                response.data!,
+                clientOptions as MutationOptions<TData, OperationVariables>
+              );
+            }
 
-          return response;
-        })
-        .catch((error) => {
-          if (mutationId === ref.current.mutationId && ref.current.isMounted) {
-            const result = {
-              loading: false,
-              error,
-              data: void 0,
-              called: true,
-              client,
-            };
-
-            if (!equal(ref.current.result, result)) {
-              setResult((ref.current.result = result));
+            return response;
+          },
+          (error) => {
+            if (
+              mutationId === ref.current.mutationId &&
+              ref.current.isMounted
+            ) {
+              const result = {
+                loading: false,
+                error,
+                data: void 0,
+                called: true,
+                client,
+              };
+
+              if (!equal(ref.current.result, result)) {
+                setResult((ref.current.result = result));
+              }
             }
-          }
 
-          const onError =
-            executeOptions.onError || ref.current.options?.onError;
+            const onError =
+              executeOptions.onError || ref.current.options?.onError;
 
-          if (onError) {
-            onError(
-              error,
-              clientOptions as MutationOptions<TData, OperationVariables>
-            );
+            if (onError) {
+              onError(
+                error,
+                clientOptions as MutationOptions<TData, OperationVariables>
+              );
 
-            // TODO(brian): why are we returning this here???
-            return { data: void 0, errors: error };
-          }
+              // TODO(brian): why are we returning this here???
+              return { data: void 0, errors: error };
+            }
 
-          throw error;
-        });
+            throw error;
+          }
+        );
     },
     []
   );