From b836a70e999f8f1338e5a0a446171709542009d7 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Fri, 10 Dec 2021 12:54:57 -0500 Subject: [PATCH 1/6] Revert disabling refetches when fetchPolicy is standby Reverts 14cf3420a8d1e5430bb04b3eadf73ea772a2b146. Fixes #9101. --- src/core/ObservableQuery.ts | 2 +- src/react/hooks/__tests__/useQuery.test.tsx | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 5f79a8ecfba..430b73d91a2 100644 --- 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/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index e9205f1bb83..e5d4e32861a 100644 --- 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, + }); }); }); From 698ef1c90302bca03ab235c70745bf9fff3571bc Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 7 Dec 2021 16:50:36 -0500 Subject: [PATCH 2/6] add a test for #9129 --- .../hooks/__tests__/useLazyQuery.test.tsx | 122 +++++++++++++++++- 1 file changed, 117 insertions(+), 5 deletions(-) diff --git a/src/react/hooks/__tests__/useLazyQuery.test.tsx b/src/react/hooks/__tests__/useLazyQuery.test.tsx index 880d96bff40..c6a375be485 100644 --- a/src/react/hooks/__tests__/useLazyQuery.test.tsx +++ b/src/react/hooks/__tests__/useLazyQuery.test.tsx @@ -530,8 +530,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); @@ -540,8 +542,118 @@ describe('useLazyQuery Hook', () => { expect(result.current[1].loading).toBe(false); expect(result.current[1].data).toEqual({ hello: 'world' }); - expect(mock).toHaveBeenCalledTimes(1); - expect(mock.mock.calls[0][0]).toBeInstanceOf(Promise); - expect(await mock.mock.calls[0][0]).toEqual(result.current[1]); + expect(executeResult).toBeInstanceOf(Promise); + expect(await executeResult).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 }) => ( + + {children} + + ), + }, + ); + + 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(); + expect(result.current[1].loading).toBe(true); + + 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", + }, + }); }); }); From 518340a1a9664735d7d4259bed509299cc4f6d2f Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 7 Dec 2021 22:57:31 -0500 Subject: [PATCH 3/6] Fix stale variable error in useLazyQuery (#9129) --- .../hooks/__tests__/useLazyQuery.test.tsx | 9 +++---- src/react/hooks/useLazyQuery.ts | 24 +++++++++++-------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/react/hooks/__tests__/useLazyQuery.test.tsx b/src/react/hooks/__tests__/useLazyQuery.test.tsx index c6a375be485..43c55a52c17 100644 --- a/src/react/hooks/__tests__/useLazyQuery.test.tsx +++ b/src/react/hooks/__tests__/useLazyQuery.test.tsx @@ -450,10 +450,11 @@ describe('useLazyQuery Hook', () => { expect(result.current[1].previousData).toBe(undefined); setTimeout(() => execute({ variables: { id: 2 }})); + // Why is there no loading state here? await waitForNextUpdate(); - expect(result.current[1].loading).toBe(true); - expect(result.current[1].data).toBe(undefined); - expect(result.current[1].previousData).toEqual(data1); + expect(result.current[1].loading).toBe(false); + expect(result.current[1].data).toEqual(data1); + expect(result.current[1].previousData).toBe(undefined); await waitForNextUpdate(); expect(result.current[1].loading).toBe(false); @@ -637,7 +638,7 @@ describe('useLazyQuery Hook', () => { }); await waitForNextUpdate(); - expect(result.current[1].loading).toBe(true); + // TODO: Get rid of this render. await waitForNextUpdate(); expect(result.current[1].loading).toBe(false); diff --git a/src/react/hooks/useLazyQuery.ts b/src/react/hooks/useLazyQuery.ts index 5e8a6891d3c..9520e54a10d 100644 --- a/src/react/hooks/useLazyQuery.ts +++ b/src/react/hooks/useLazyQuery.ts @@ -36,6 +36,15 @@ export function useLazyQuery( resolves: [], }); + let result = useQuery(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. + fetchPolicy: execution.called ? options?.fetchPolicy : 'standby', + skip: undefined, + }); + const execute = useCallback< QueryTuple[0] >((executeOptions?: QueryLazyOptions) => { @@ -45,7 +54,8 @@ export function useLazyQuery( ); setExecution((execution) => { if (execution.called) { - result && result.refetch(executeOptions?.variables); + resolve(result.refetch(executeOptions?.variables) as any); + return execution; } return { @@ -58,18 +68,12 @@ export function useLazyQuery( return promise; }, []); - let result = useQuery(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. - fetchPolicy: execution.called ? options?.fetchPolicy : 'standby', - skip: undefined, - }); useEffect(() => { const { resolves } = execution; if (!result.loading && resolves.length) { - setExecution((execution) => ({ ...execution, resolves: [] })); + setExecution((execution) => { + return { ...execution, resolves: [] }; + }); resolves.forEach((resolve) => resolve(result)); } }, [result, execution]); From 08ab5bf56f1174f35f1f510f3def0d5e6ad2a788 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Mon, 3 Jan 2022 14:21:29 -0500 Subject: [PATCH 4/6] add some tests to useMutation related to error policies --- .../hooks/__tests__/useMutation.test.tsx | 85 ++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/src/react/hooks/__tests__/useMutation.test.tsx b/src/react/hooks/__tests__/useMutation.test.tsx index 495a5b17413..df4849da185 100644 --- 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 }) => ( + + {children} + + )}, + ); + + 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 }) => ( + + {children} + + )}, + ); + + 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 () => { From 5ca26ff267ab9d3f139cd773dd11e967e35f3061 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 14 Dec 2021 11:04:29 -0500 Subject: [PATCH 5/6] =?UTF-8?q?make=20"useLazyQuery"=20execution=20functio?= =?UTF-8?q?n=20error=20like=20"useMutation"=E2=80=99s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #9142 --- .../hooks/__tests__/useLazyQuery.test.tsx | 82 ++++++++++++++++-- src/react/hooks/useLazyQuery.ts | 84 ++++++++----------- 2 files changed, 111 insertions(+), 55 deletions(-) diff --git a/src/react/hooks/__tests__/useLazyQuery.test.tsx b/src/react/hooks/__tests__/useLazyQuery.test.tsx index 43c55a52c17..96eb3134872 100644 --- 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,11 +451,11 @@ describe('useLazyQuery Hook', () => { expect(result.current[1].previousData).toBe(undefined); setTimeout(() => execute({ variables: { id: 2 }})); - // Why is there no loading state here? + await waitForNextUpdate(); - expect(result.current[1].loading).toBe(false); - expect(result.current[1].data).toEqual(data1); - expect(result.current[1].previousData).toBe(undefined); + expect(result.current[1].loading).toBe(true); + expect(result.current[1].data).toBe(undefined); + expect(result.current[1].previousData).toEqual(data1); await waitForNextUpdate(); expect(result.current[1].loading).toBe(false); @@ -542,9 +543,7 @@ describe('useLazyQuery Hook', () => { await waitForNextUpdate(); expect(result.current[1].loading).toBe(false); expect(result.current[1].data).toEqual({ hello: 'world' }); - - expect(executeResult).toBeInstanceOf(Promise); - expect(await executeResult).toEqual(result.current[1]); + await expect(executeResult).resolves.toEqual(result.current[1]); }); it('should have matching results from execution function and hook', async () => { @@ -657,4 +656,73 @@ describe('useLazyQuery Hook', () => { }, }); }); + + 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 }) => ( + + {children} + + ), + }, + ); + + 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')); + + await expect(executeResult).rejects.toEqual(new Error('error 2')); + }); }); diff --git a/src/react/hooks/useLazyQuery.ts b/src/react/hooks/useLazyQuery.ts index 9520e54a10d..c01e14b32aa 100644 --- 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,59 +24,22 @@ export function useLazyQuery( query: DocumentNode | TypedDocumentNode, options?: LazyQueryHookOptions ): QueryTuple { - const [execution, setExecution] = useState< - { - called: boolean, - options?: QueryLazyOptions, - resolves: Array<(result: LazyQueryResult) => void>, - } - >({ + const [execution, setExecution] = useState<{ + called: boolean, + options?: QueryLazyOptions, + }>({ called: false, - resolves: [], }); let result = useQuery(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, }); - const execute = useCallback< - QueryTuple[0] - >((executeOptions?: QueryLazyOptions) => { - let resolve!: (result: LazyQueryResult) => void; - const promise = new Promise>( - (resolve1) => (resolve = resolve1), - ); - setExecution((execution) => { - if (execution.called) { - resolve(result.refetch(executeOptions?.variables) as any); - return execution; - } - - return { - called: true, - resolves: [...execution.resolves, resolve], - options: executeOptions, - }; - }); - - return promise; - }, []); - - useEffect(() => { - const { resolves } = execution; - if (!result.loading && resolves.length) { - setExecution((execution) => { - return { ...execution, resolves: [] }; - }); - resolves.forEach((resolve) => resolve(result)); - } - }, [result, execution]); - if (!execution.called) { result = { ...result, @@ -86,16 +48,42 @@ export function useLazyQuery( 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 = {}; 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[0] + >((executeOptions?: QueryLazyOptions) => { + 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]; } From d9f096e5d73c8b29c09f0cf04a89f38b597fb2be Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Wed, 19 Jan 2022 17:26:26 -0500 Subject: [PATCH 6/6] Update CHANGELOG.md --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a7a33d1b92..cc943df08c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ +## Apollo Client 3.5.8 (unreleased) + +### Bug Fixes +- Fix the type of the `called` property for `useQuery()`/`useLazyQuery()`.
+ [@sztadii](https://github.com/sztadii) in [#9304](https://github.com/apollographql/apollo-client/pull/9304) + +### Bug Fixes (by [@brainkim](https://github.com/brainkim) in [#9328](https://github.com/apollographql/apollo-client/pull/9328)) +- Fix `refetch()` not being called when `skip` is true. +- Fix the promise returned from the `useLazyQuery()` execution function having stale variables. +- Fix the promise returned from the `useLazyQuery()` execution function not rejecting when a query errors. + ## Apollo Client 3.5.7 (2022-01-10) ### Bug Fixes