Repository: apollographql/apollo-client

Problem statement:
useLazyQuery promise .catch not catching errors, when useMutation does
I should be able to add a `.catch` to the `useLazyQuery` promise, which should be called when the response is a graphql _or_ network error. Right now, the `.catch` does not fire in either case, and the error is present in the result variable inside the `.then`.

This is inconsistent with the `useMutation` which does fire the `.catch` on errors.

----------------

**Intended outcome:**
```js
someQuery()
    .then((r) => console.log('query done!', r))
    .catch((e) => console.log('query error!', e))
```
I should get `query error!` when there is an error.

**Actual outcome:**
```js
someQuery()
    .then((r) => console.log('query done!', r))
    .catch((e) => console.log('query error!', e))
```
I get `query done!` when there is an error.

----------------

**How to reproduce the issue:**
<details><summary>Click to open code example</summary>

The following example uses the `MockedProvider` but has the exact same result when using the normal `ApolloProvider`

```js
import React from 'react';
import ReactDOM from 'react-dom';
import './index.css';
import {MockedProvider} from "@apollo/client/testing";
import {gql, InMemoryCache, useLazyQuery, useMutation} from "@apollo/client";

const cache = new InMemoryCache();
const SOME_MUTATION = gql`
    mutation SomeMutation {
        test
    }
`;
const SOME_QUERY = gql`
    query SomeQuery {
        test
    }
`;
const mocks = [
    {
        request: {
            query: SOME_MUTATION,
        },
        result: {
            errors: [{message: 'ERROR'}]
        }
    },
    {
        request: {
            query: SOME_QUERY,
        },
        result: {
            errors: [{message: 'ERROR'}]
        }
    }
]

function App() {
    const [someMutation] = useMutation(SOME_MUTATION);
    const [someQuery] = useLazyQuery(SOME_QUERY);

    const startMutation = () => {
        someMutation()
            .then((r) => console.log('mutation done!', r))
            .catch((e) => console.log('mutation error!', e))
    }

    const startUseLazyQuery = () => {
        someQuery()
            .then((r) => console.log('query done!', r))
            .catch((e) => console.log('query error!', e))
    }

    return (
        <div>
            <button onClick={startMutation}>Click me for mutation</button>
            <br/>
            <button onClick={startUseLazyQuery}>Click me for lazy query</button>
        </div>
    )
}

ReactDOM.render(
  <React.StrictMode>
      <div className="App">
          <MockedProvider cache={cache} mocks={mocks}>
              <App />
          </MockedProvider>
      </div>
  </React.StrictMode>,
  document.getElementById('root')
);
```

</details>

----------------

**Versions**
```
  System:
    OS: Linux 4.15 Ubuntu 18.04.6 LTS (Bionic Beaver)
  Binaries:
    Node: 12.22.7 - /usr/bin/node
    Yarn: 1.21.1 - /usr/bin/yarn
    npm: 6.14.15 - /usr/bin/npm
  Browsers:
    Chrome: 95.0.4638.54
    Firefox: 94.0
  npmPackages:
    @apollo/client: ^3.5.5 => 3.5.5
```


Hints:
@MattBred Thanks for reporting this!

Seems like [this `Promise`](https://github.com/apollographql/apollo-client/blob/5d0c881a4c20c5c48842d9a8d246d26a917eccf9/src/react/hooks/useLazyQuery.ts#L44) needs to be rejected on errors, but we're currently only using its `resolve` function. Can you take a look @brainkim?
I object to the bug label because this is new 3.5 functionality but yeah this seem right to do. Though, I don’t understand why people don’t just call `ApolloClient.query()` directly at some point.

Patch:
diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts
--- a/src/core/ObservableQuery.ts
+++ b/src/core/ObservableQuery.ts
@@ -326,7 +326,7 @@ export class ObservableQuery<
     // (no-cache, network-only, or cache-and-network), override it with
     // network-only to force the refetch for this fetchQuery call.
     const { fetchPolicy } = this.options;
-    if (fetchPolicy === 'standby' || fetchPolicy === 'cache-and-network') {
+    if (fetchPolicy === 'cache-and-network') {
       reobserveOptions.fetchPolicy = fetchPolicy;
     } else if (fetchPolicy === 'no-cache') {
       reobserveOptions.fetchPolicy = 'no-cache';
diff --git a/src/react/hooks/useLazyQuery.ts b/src/react/hooks/useLazyQuery.ts
--- a/src/react/hooks/useLazyQuery.ts
+++ b/src/react/hooks/useLazyQuery.ts
@@ -1,10 +1,9 @@
 import { DocumentNode } from 'graphql';
 import { TypedDocumentNode } from '@graphql-typed-document-node/core';
-import { useCallback, useEffect, useState } from 'react';
+import { useCallback, useMemo, useState } from 'react';
 
 import {
   LazyQueryHookOptions,
-  LazyQueryResult,
   QueryLazyOptions,
   QueryTuple,
 } from '../types/types';
@@ -25,54 +24,21 @@ export function useLazyQuery<TData = any, TVariables = OperationVariables>(
   query: DocumentNode | TypedDocumentNode<TData, TVariables>,
   options?: LazyQueryHookOptions<TData, TVariables>
 ): QueryTuple<TData, TVariables> {
-  const [execution, setExecution] = useState<
-    {
-      called: boolean,
-      options?: QueryLazyOptions<TVariables>,
-      resolves: Array<(result: LazyQueryResult<TData, TVariables>) => void>,
-    }
-  >({
+  const [execution, setExecution] = useState<{
+    called: boolean,
+    options?: QueryLazyOptions<TVariables>,
+  }>({
     called: false,
-    resolves: [],
   });
 
-  const execute = useCallback<
-    QueryTuple<TData, TVariables>[0]
-  >((executeOptions?: QueryLazyOptions<TVariables>) => {
-    let resolve!: (result: LazyQueryResult<TData, TVariables>) => void;
-    const promise = new Promise<LazyQueryResult<TData, TVariables>>(
-      (resolve1) => (resolve = resolve1),
-    );
-    setExecution((execution) => {
-      if (execution.called) {
-        result && result.refetch(executeOptions?.variables);
-      }
-
-      return {
-        called: true,
-        resolves: [...execution.resolves, resolve],
-        options: executeOptions,
-      };
-    });
-
-    return promise;
-  }, []);
-
   let result = useQuery<TData, TVariables>(query, {
     ...options,
     ...execution.options,
-    // We don’t set skip to execution.called, because we need useQuery to call
-    // addQueryPromise, so that ssr calls waits for execute to be called.
+    // We don’t set skip to execution.called, because some useQuery SSR code
+    // checks skip for some reason.
     fetchPolicy: execution.called ? options?.fetchPolicy : 'standby',
     skip: undefined,
   });
-  useEffect(() => {
-    const { resolves } = execution;
-    if (!result.loading && resolves.length) {
-      setExecution((execution) => ({ ...execution, resolves: [] }));
-      resolves.forEach((resolve) => resolve(result));
-    }
-  }, [result, execution]);
 
   if (!execution.called) {
     result = {
@@ -82,16 +48,42 @@ export function useLazyQuery<TData = any, TVariables = OperationVariables>(
       error: void 0,
       called: false,
     };
+  }
 
-
+  // We use useMemo here to make sure the eager methods have a stable identity.
+  const eagerMethods = useMemo(() => {
+    const eagerMethods: Record<string, any> = {};
     for (const key of EAGER_METHODS) {
       const method = result[key];
-      result[key] = (...args: any) => {
+      eagerMethods[key] = (...args: any) => {
         setExecution((execution) => ({ ...execution, called: true }));
         return (method as any)(...args);
       };
     }
-  }
+
+    return eagerMethods;
+  }, []);
+
+  result.error = result.error || void 0;
+  Object.assign(result, eagerMethods);
+
+  const execute = useCallback<
+    QueryTuple<TData, TVariables>[0]
+  >((executeOptions?: QueryLazyOptions<TVariables>) => {
+    setExecution({ called: true, options: executeOptions });
+    return result.refetch(executeOptions?.variables).then((result1) => {
+      const result2 = {
+        ...result,
+        data: result1.data,
+        error: result1.error,
+        called: true,
+        loading: false,
+      };
+
+      Object.assign(result2, eagerMethods);
+      return result2;
+    });
+  }, []);
 
   return [execute, result];
 }


Test patch:
diff --git a/src/react/hooks/__tests__/useLazyQuery.test.tsx b/src/react/hooks/__tests__/useLazyQuery.test.tsx
--- a/src/react/hooks/__tests__/useLazyQuery.test.tsx
+++ b/src/react/hooks/__tests__/useLazyQuery.test.tsx
@@ -1,4 +1,5 @@
 import React from 'react';
+import { GraphQLError } from 'graphql';
 import gql from 'graphql-tag';
 import { renderHook } from '@testing-library/react-hooks';
 
@@ -450,6 +451,7 @@ describe('useLazyQuery Hook', () => {
     expect(result.current[1].previousData).toBe(undefined);
 
     setTimeout(() => execute({ variables: { id: 2 }}));
+
     await waitForNextUpdate();
     expect(result.current[1].loading).toBe(true);
     expect(result.current[1].data).toBe(undefined);
@@ -530,8 +532,10 @@ describe('useLazyQuery Hook', () => {
     expect(result.current[1].loading).toBe(false);
     expect(result.current[1].data).toBe(undefined);
     const execute = result.current[0];
-    const mock = jest.fn();
-    setTimeout(() => mock(execute()));
+    let executeResult: any;
+    setTimeout(() => {
+      executeResult = execute();
+    });
 
     await waitForNextUpdate();
     expect(result.current[1].loading).toBe(true);
@@ -539,9 +543,186 @@ describe('useLazyQuery Hook', () => {
     await waitForNextUpdate();
     expect(result.current[1].loading).toBe(false);
     expect(result.current[1].data).toEqual({ hello: 'world' });
+    await expect(executeResult).resolves.toEqual(result.current[1]);
+  });
+
+  it('should have matching results from execution function and hook', async () => {
+    const query = gql`
+      query GetCountries($filter: String) {
+        countries(filter: $filter) {
+          code
+          name
+        }
+      }
+    `;
+
+    const mocks = [
+      {
+        request: {
+          query,
+          variables: {
+            filter: "PA",
+          },
+        },
+        result: {
+          data: {
+            countries: {
+              code: "PA",
+              name: "Panama",
+            },
+          },
+        },
+        delay: 20,
+      },
+      {
+        request: {
+          query,
+          variables: {
+            filter: "BA",
+          },
+        },
+        result: {
+          data: {
+            countries: {
+              code: "BA",
+              name: "Bahamas",
+            },
+          },
+        },
+        delay: 20,
+      },
+    ];
+
+    const { result, waitForNextUpdate } = renderHook(
+      () => useLazyQuery(query),
+      {
+        wrapper: ({ children }) => (
+          <MockedProvider mocks={mocks}>
+            {children}
+          </MockedProvider>
+        ),
+      },
+    );
+
+    expect(result.current[1].loading).toBe(false);
+    expect(result.current[1].data).toBe(undefined);
+    const execute = result.current[0];
+    let executeResult: any;
+    setTimeout(() => {
+      executeResult = execute({ variables: { filter: "PA" } });
+    });
+
+    await waitForNextUpdate();
+    expect(result.current[1].loading).toBe(true);
+
+    await waitForNextUpdate();
+    expect(result.current[1].loading).toBe(false);
+    expect(result.current[1].data).toEqual({
+      countries: {
+        code: "PA",
+        name: "Panama",
+      },
+    });
+
+    expect(executeResult).toBeInstanceOf(Promise);
+    expect((await executeResult).data).toEqual({
+      countries: {
+        code: "PA",
+        name: "Panama",
+      },
+    });
+
+    setTimeout(() => {
+      executeResult = execute({ variables: { filter: "BA" } });
+    });
+
+    await waitForNextUpdate();
+    // TODO: Get rid of this render.
+
+    await waitForNextUpdate();
+    expect(result.current[1].loading).toBe(false);
+    expect(result.current[1].data).toEqual({
+      countries: {
+        code: "BA",
+        name: "Bahamas",
+      },
+    });
+
+    expect(executeResult).toBeInstanceOf(Promise);
+    expect((await executeResult).data).toEqual({
+      countries: {
+        code: "BA",
+        name: "Bahamas",
+      },
+    });
+  });
+
+  it('the promise should reject with errors the “way useMutation does”', async () => {
+    const query = gql`{ hello }`;
+    const mocks = [
+      {
+        request: { query },
+        result: {
+          errors: [new GraphQLError('error 1')],
+        },
+        delay: 20,
+      },
+      {
+        request: { query },
+        result: {
+          errors: [new GraphQLError('error 2')],
+        },
+        delay: 20,
+      },
+    ];
+
+    const { result, waitForNextUpdate } = renderHook(
+      () => useLazyQuery(query),
+      {
+        wrapper: ({ children }) => (
+          <MockedProvider mocks={mocks}>
+            {children}
+          </MockedProvider>
+        ),
+      },
+    );
+
+    const execute = result.current[0];
+    let executeResult: any;
+    expect(result.current[1].loading).toBe(false);
+    expect(result.current[1].data).toBe(undefined);
+    setTimeout(() => {
+      executeResult = execute();
+      executeResult.catch(() => {});
+    });
+
+    await waitForNextUpdate();
+    expect(result.current[1].loading).toBe(true);
+    expect(result.current[1].data).toBe(undefined);
+    expect(result.current[1].error).toBe(undefined);
+
+    await waitForNextUpdate();
+    expect(result.current[1].loading).toBe(false);
+    expect(result.current[1].data).toBe(undefined);
+    expect(result.current[1].error).toEqual(new Error('error 1'));
+
+    await expect(executeResult).rejects.toEqual(new Error('error 1'));
+
+    setTimeout(() => {
+      executeResult = execute();
+      executeResult.catch(() => {});
+    });
+
+    await waitForNextUpdate();
+    expect(result.current[1].loading).toBe(false);
+    expect(result.current[1].data).toBe(undefined);
+    expect(result.current[1].error).toEqual(new Error('error 1'));
+
+    await waitForNextUpdate();
+    expect(result.current[1].loading).toBe(false);
+    expect(result.current[1].data).toBe(undefined);
+    expect(result.current[1].error).toEqual(new Error('error 2'));
 
-    expect(mock).toHaveBeenCalledTimes(1);
-    expect(mock.mock.calls[0][0]).toBeInstanceOf(Promise);
-    expect(await mock.mock.calls[0][0]).toEqual(result.current[1]);
+    await expect(executeResult).rejects.toEqual(new Error('error 2'));
   });
 });
diff --git a/src/react/hooks/__tests__/useMutation.test.tsx b/src/react/hooks/__tests__/useMutation.test.tsx
--- a/src/react/hooks/__tests__/useMutation.test.tsx
+++ b/src/react/hooks/__tests__/useMutation.test.tsx
@@ -272,6 +272,49 @@ describe('useMutation Hook', () => {
         expect(onError.mock.calls[0][0].message).toBe(CREATE_TODO_ERROR);
       });
 
+      it('should reject when there’s only an error and no error policy is set', async () => {
+        const variables = {
+          description: 'Get milk!'
+        };
+
+        const mocks = [
+          {
+            request: {
+              query: CREATE_TODO_MUTATION,
+              variables,
+            },
+            result: {
+              errors: [new GraphQLError(CREATE_TODO_ERROR)],
+            },
+          }
+        ];
+
+        const { result } = renderHook(
+          () => useMutation(CREATE_TODO_MUTATION),
+          { wrapper: ({ children }) => (
+            <MockedProvider mocks={mocks}>
+              {children}
+            </MockedProvider>
+          )},
+        );
+
+        const createTodo = result.current[0];
+        let fetchError: any;
+        await act(async () => {
+          // need to call createTodo this way to get “act” warnings to go away.
+          try {
+            await createTodo({ variables });
+          } catch (err) {
+            fetchError = err;
+            return;
+          }
+
+          throw new Error("function did not error");
+        });
+
+        expect(fetchError).toEqual(new GraphQLError(CREATE_TODO_ERROR));
+      });
+
       it(`should reject when errorPolicy is 'none'`, async () => {
         const variables = {
           description: 'Get milk!'
@@ -341,7 +384,47 @@ describe('useMutation Hook', () => {
 
         expect(fetchResult.data).toEqual(CREATE_TODO_RESULT);
         expect(fetchResult.errors[0].message).toEqual(CREATE_TODO_ERROR);
-      })
+      });
+
+      it(`should ignore errors when errorPolicy is 'ignore'`, async () => {
+        const errorMock = jest.spyOn(console, "error")
+          .mockImplementation(() => {});
+        const variables = {
+          description: 'Get milk!'
+        };
+
+        const mocks = [
+          {
+            request: {
+              query: CREATE_TODO_MUTATION,
+              variables,
+            },
+            result: {
+              errors: [new GraphQLError(CREATE_TODO_ERROR)],
+            },
+          }
+        ];
+
+        const { result } = renderHook(
+          () => useMutation(CREATE_TODO_MUTATION, { errorPolicy: "ignore" }),
+          { wrapper: ({ children }) => (
+            <MockedProvider mocks={mocks}>
+              {children}
+            </MockedProvider>
+          )},
+        );
+
+        const createTodo = result.current[0];
+        let fetchResult: any;
+        await act(async () => {
+          fetchResult = await createTodo({ variables });
+        });
+
+        expect(fetchResult).toEqual({});
+        expect(errorMock).toHaveBeenCalledTimes(1);
+        expect(errorMock.mock.calls[0][0]).toMatch("Missing field");
+        errorMock.mockRestore();
+      });
     });
 
     it('should return the current client instance in the result object', async () => {
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
@@ -3071,7 +3071,8 @@ describe('useQuery Hook', () => {
       expect(result.current.data).toEqual({ hello: 'world' });
     });
 
-    it('should not refetch when skip is true', async () => {
+    // Amusingly, #8270 thinks this is a bug, but #9101 thinks this is not.
+    it('should refetch when skip is true', async () => {
       const query = gql`{ hello }`;
       const link = new ApolloLink(() => Observable.of({
         data: { hello: 'world' },
@@ -3098,13 +3099,18 @@ describe('useQuery Hook', () => {
       expect(result.current.data).toBe(undefined);
       await expect(waitForNextUpdate({ timeout: 20 }))
         .rejects.toThrow('Timed out');
-      result.current.refetch();
-      await expect(waitForNextUpdate({ timeout: 20 }))
-        .rejects.toThrow('Timed out');
+      const promise = result.current.refetch();
+      // TODO: Not really sure about who is causing this render.
+      await waitForNextUpdate();
       expect(result.current.loading).toBe(false);
       expect(result.current.data).toBe(undefined);
-      expect(requestSpy).toHaveBeenCalledTimes(0);
+      expect(requestSpy).toHaveBeenCalledTimes(1);
       requestSpy.mockRestore();
+      expect(promise).resolves.toEqual({
+        data: {hello: "world"},
+        loading: false,
+        networkStatus: 7,
+      });
     });
   });