Repository: apollographql/apollo-client

Problem statement:
[4.0] Remove `setOnError` and `onError` from `ApolloLink`
These APIs are only used by `MockLink` and appear to only be there to rewrite errors. This code is not called anywhere else. We'd prefer to remove this API.


Hints:

Patch:
diff --git a/src/link/core/ApolloLink.ts b/src/link/core/ApolloLink.ts
--- a/src/link/core/ApolloLink.ts
+++ b/src/link/core/ApolloLink.ts
@@ -1,4 +1,4 @@
-import type { Observable, Subscriber } from "rxjs";
+import type { Observable } from "rxjs";
 import { EMPTY } from "rxjs";
 
 import {
@@ -138,29 +138,6 @@ export class ApolloLink {
     throw newInvariantError("request is not implemented");
   }
 
-  protected onError(
-    error: any,
-    observer?: Subscriber<FetchResult>
-  ): false | void {
-    if (observer && observer.error) {
-      observer.error(error);
-      // Returning false indicates that observer.error does not need to be
-      // called again, since it was already called (on the previous line).
-      // Calling observer.error again would not cause any real problems,
-      // since only the first call matters, but custom onError functions
-      // might have other reasons for wanting to prevent the default
-      // behavior by returning false.
-      return false;
-    }
-    // Throw errors will be passed to observer.error.
-    throw error;
-  }
-
-  public setOnError(fn: ApolloLink["onError"]): this {
-    this.onError = fn;
-    return this;
-  }
-
   /**
    * @internal
    * Used to iterate through all links that are concatenations or `split` links.


Test patch:
diff --git a/src/__tests__/mutationResults.ts b/src/__tests__/mutationResults.ts
--- a/src/__tests__/mutationResults.ts
+++ b/src/__tests__/mutationResults.ts
@@ -164,9 +164,7 @@ describe("mutation results", () => {
           delay,
         },
         ...mockedResponses
-      ).setOnError((error) => {
-        throw error;
-      }),
+      ),
       cache: new InMemoryCache({
         dataIdFromObject: (obj: any) => {
           if (obj.id && obj.__typename) {
diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts
--- a/src/cache/inmemory/__tests__/policies.ts
+++ b/src/cache/inmemory/__tests__/policies.ts
@@ -3654,9 +3654,7 @@ describe("type policies", function () {
             },
           },
         },
-      ]).setOnError((error) => {
-        throw new Error(error);
-      });
+      ]);
 
       const client = new ApolloClient({ link, cache });
 
@@ -4095,9 +4093,7 @@ describe("type policies", function () {
             },
           },
         },
-      ]).setOnError((error) => {
-        throw new Error(error);
-      });
+      ]);
 
       const client = new ApolloClient({ link, cache });
 
diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx
--- a/src/react/hooks/__tests__/useQuery.test.tsx
+++ b/src/react/hooks/__tests__/useQuery.test.tsx
@@ -823,7 +823,11 @@ describe("useQuery Hook", () => {
           },
           {
             wrapper: ({ children }) => (
-              <MockedProvider mocks={mocks} cache={cache}>
+              <MockedProvider
+                mocks={mocks}
+                cache={cache}
+                mockLinkDefaultOptions={{ delay: 0 }}
+              >
                 {children}
               </MockedProvider>
             ),
@@ -2106,8 +2110,6 @@ describe("useQuery Hook", () => {
 
       const link = new MockLink(mocks);
       const requestSpy = jest.spyOn(link, "request");
-      const onErrorFn = jest.fn();
-      link.setOnError(onErrorFn);
       const wrapper = ({ children }: any) => (
         <MockedProvider link={link} cache={cache}>
           {children}
@@ -2162,10 +2164,6 @@ describe("useQuery Hook", () => {
         )
       ).rejects.toThrow();
 
-      await waitFor(() => {
-        expect(onErrorFn).toHaveBeenCalledTimes(0);
-      });
-
       requestSpy.mockRestore();
     });
 
@@ -2201,8 +2199,6 @@ describe("useQuery Hook", () => {
 
       const link = new MockLink(mocks);
       const requestSpy = jest.spyOn(link, "request");
-      const onErrorFn = jest.fn();
-      link.setOnError(onErrorFn);
 
       const client = new ApolloClient({
         queryDeduplication: false,
@@ -2275,7 +2271,6 @@ describe("useQuery Hook", () => {
       jest.advanceTimersByTime(200);
 
       expect(requestSpy).toHaveBeenCalledTimes(2);
-      expect(onErrorFn).toHaveBeenCalledTimes(0);
 
       jest.useRealTimers();
     });
@@ -2307,8 +2302,6 @@ describe("useQuery Hook", () => {
       const cache = new InMemoryCache();
       const link = new MockLink(mocks);
       const requestSpy = jest.spyOn(link, "request");
-      const onErrorFn = jest.fn();
-      link.setOnError(onErrorFn);
       const wrapper = ({ children }: any) => (
         <React.StrictMode>
           <MockedProvider link={link} cache={cache}>
@@ -2362,7 +2355,6 @@ describe("useQuery Hook", () => {
         )
       ).rejects.toThrow();
       expect(requestSpy).toHaveBeenCalledTimes(requestSpyCallCount);
-      expect(onErrorFn).toHaveBeenCalledTimes(0);
 
       requestSpy.mockRestore();
     });
@@ -2398,8 +2390,6 @@ describe("useQuery Hook", () => {
 
       const link = new MockLink(mocks);
       const requestSpy = jest.spyOn(link, "request");
-      const onErrorFn = jest.fn();
-      link.setOnError(onErrorFn);
 
       const client = new ApolloClient({ link, cache });
 
@@ -2464,7 +2454,6 @@ describe("useQuery Hook", () => {
       await expect(takeSnapshot).not.toRerender({ timeout: 50 });
       // TODO rarely seeing 3 here investigate further
       expect(requestSpy).toHaveBeenCalledTimes(2);
-      expect(onErrorFn).toHaveBeenCalledTimes(0);
     });
 
     it("should start and stop polling in Strict Mode", async () => {
@@ -2495,8 +2484,6 @@ describe("useQuery Hook", () => {
       const cache = new InMemoryCache();
       const link = new MockLink(mocks);
       const requestSpy = jest.spyOn(link, "request");
-      const onErrorFn = jest.fn();
-      link.setOnError(onErrorFn);
       const wrapper = ({ children }: any) => (
         <React.StrictMode>
           <MockedProvider link={link} cache={cache}>
@@ -2555,7 +2542,6 @@ describe("useQuery Hook", () => {
       getCurrentSnapshot().startPolling(20);
 
       expect(requestSpy).toHaveBeenCalledTimes(2);
-      expect(onErrorFn).toHaveBeenCalledTimes(0);
 
       {
         const result = await takeSnapshot();
@@ -2582,7 +2568,6 @@ describe("useQuery Hook", () => {
       }
 
       expect(requestSpy).toHaveBeenCalledTimes(4);
-      expect(onErrorFn).toHaveBeenCalledTimes(0);
       requestSpy.mockRestore();
     });
 
diff --git a/src/testing/core/mocking/mockClient.ts b/src/testing/core/mocking/mockClient.ts
--- a/src/testing/core/mocking/mockClient.ts
+++ b/src/testing/core/mocking/mockClient.ts
@@ -5,6 +5,7 @@ import { InMemoryCache } from "@apollo/client/cache";
 
 import { mockSingleLink } from "./mockLink.js";
 
+// TODO: Deprecate this function
 export function createMockClient<TData>(
   data: TData,
   query: DocumentNode,
@@ -14,8 +15,6 @@ export function createMockClient<TData>(
     link: mockSingleLink({
       request: { query, variables },
       result: { data },
-    }).setOnError((error) => {
-      throw error;
     }),
     cache: new InMemoryCache(),
   });
diff --git a/src/testing/core/mocking/mockLink.ts b/src/testing/core/mocking/mockLink.ts
--- a/src/testing/core/mocking/mockLink.ts
+++ b/src/testing/core/mocking/mockLink.ts
@@ -172,14 +172,7 @@ export class MockLink extends ApolloLink {
         );
       }
 
-      return throwError(() => {
-        const error = new Error(message);
-
-        // TODO: Remove this once `onError` and `setOnError` is removed.
-        if (this.onError(error) !== false) {
-          return error;
-        }
-      });
+      return throwError(() => new Error(message));
     }
 
     if (matched.maxUsageCount > 1) {
diff --git a/src/testing/react/__tests__/MockedProvider.test.tsx b/src/testing/react/__tests__/MockedProvider.test.tsx
--- a/src/testing/react/__tests__/MockedProvider.test.tsx
+++ b/src/testing/react/__tests__/MockedProvider.test.tsx
@@ -998,61 +998,6 @@ describe("General use", () => {
     consoleSpy.mockRestore();
   });
 
-  it("should support custom error handling using setOnError", async () => {
-    let finished = false;
-    function Component({ ...variables }: Variables) {
-      useQuery<Data, Variables>(query, { variables });
-      return null;
-    }
-
-    const mockLink = new MockLink([], { showWarnings: false });
-    mockLink.setOnError((error) => {
-      expect(error).toMatchSnapshot();
-      finished = true;
-    });
-    const link = ApolloLink.from([errorLink, mockLink]);
-
-    render(
-      <MockedProvider link={link}>
-        <Component {...variables} />
-      </MockedProvider>
-    );
-
-    await waitFor(() => {
-      expect(finished).toBe(true);
-    });
-  });
-
-  it("should pipe exceptions thrown in custom onError functions through the link chain", async () => {
-    let finished = false;
-    function Component({ ...variables }: Variables) {
-      const { loading, error } = useQuery<Data, Variables>(query, {
-        variables,
-      });
-      if (!loading) {
-        expect(error).toMatchSnapshot();
-        finished = true;
-      }
-      return null;
-    }
-
-    const mockLink = new MockLink([], { showWarnings: false });
-    mockLink.setOnError(() => {
-      throw new Error("oh no!");
-    });
-    const link = ApolloLink.from([errorLink, mockLink]);
-
-    render(
-      <MockedProvider link={link}>
-        <Component {...variables} />
-      </MockedProvider>
-    );
-
-    await waitFor(() => {
-      expect(finished).toBe(true);
-    });
-  });
-
   it("should support loading state testing with delay", async () => {
     jest.useFakeTimers();
 
diff --git a/src/testing/react/__tests__/__snapshots__/MockedProvider.test.tsx.snap b/src/testing/react/__tests__/__snapshots__/MockedProvider.test.tsx.snap
--- a/src/testing/react/__tests__/__snapshots__/MockedProvider.test.tsx.snap
+++ b/src/testing/react/__tests__/__snapshots__/MockedProvider.test.tsx.snap
@@ -84,8 +84,6 @@ Object {
 }
 `;
 
-exports[`General use should pipe exceptions thrown in custom onError functions through the link chain 1`] = `[Error: oh no!]`;
-
 exports[`General use should return "Mocked response should contain" errors in response 1`] = `
 [Error: Mocked response should contain either \`result\`, \`error\` or a \`delay\` of \`Infinity\`:
 {
@@ -111,32 +109,6 @@ Request variables: {}
 ]
 `;
 
-exports[`General use should support custom error handling using setOnError 1`] = `
-[Error: No more mocked responses for the query:
-query GetUser($username: String!) {
-  user(username: $username) {
-    id
-    __typename
-  }
-}
-
-Request variables: {"username":"mock_username"}
-]
-`;
-
-exports[`General use should support custom error handling using setOnError 2`] = `
-[Error: No more mocked responses for the query:
-query GetUser($username: String!) {
-  user(username: $username) {
-    id
-    __typename
-  }
-}
-
-Request variables: {"username":"mock_username"}
-]
-`;
-
 exports[`General use should use the mock if the \`variables\` callback function returns true 1`] = `
 Object {
   "__typename": "User",