diff --git a/.changeset/nine-lemons-swim.md b/.changeset/nine-lemons-swim.md new file mode 100644 index 00000000000..c7e386bf0f2 --- /dev/null +++ b/.changeset/nine-lemons-swim.md @@ -0,0 +1,9 @@ +--- +'@apollo/client': patch +--- + +Changes the behavior of `useLazyQuery` introduced in [#10427](https://github.com/apollographql/apollo-client/pull/10427) where unmounting a component before a query was resolved would reject the promise with an abort error. Instead, the promise will now resolve naturally with the result from the request. + +Other notable fixes: +- Kicking off multiple requests in parallel with the execution function will now ensure each returned promise is resolved with the data from its request. Previously, each promise was resolved with data from the last execution. +- Re-rendering `useLazyQuery` with a different query document will now ensure the execution function uses the updated query document. Previously, only the query document rendered the first time would be used for the request. diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index b59f5030751..cac020e0eab 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -782,10 +782,10 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`); return this.last; } - public reobserve( + public reobserveAsConcast( newOptions?: Partial>, newNetworkStatus?: NetworkStatus, - ): Promise> { + ): Concast> { this.isTornDown = false; const useDisposableConcast = @@ -858,7 +858,14 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`); concast.addObserver(observer); - return concast.promise; + return concast; + } + + public reobserve( + newOptions?: Partial>, + newNetworkStatus?: NetworkStatus, + ) { + return this.reobserveAsConcast(newOptions, newNetworkStatus).promise; } // (Re)deliver the current result to this.observers without applying fetch diff --git a/src/react/hooks/__tests__/useLazyQuery.test.tsx b/src/react/hooks/__tests__/useLazyQuery.test.tsx index 9863eff19a0..a60d0f36395 100644 --- a/src/react/hooks/__tests__/useLazyQuery.test.tsx +++ b/src/react/hooks/__tests__/useLazyQuery.test.tsx @@ -3,10 +3,23 @@ import { GraphQLError } from 'graphql'; import gql from 'graphql-tag'; import { act, renderHook, waitFor } from '@testing-library/react'; -import { ApolloClient, ApolloLink, ErrorPolicy, InMemoryCache, NetworkStatus, TypedDocumentNode } from '../../../core'; +import { + ApolloClient, + ApolloLink, + ErrorPolicy, + InMemoryCache, + NetworkStatus, + TypedDocumentNode +} from '../../../core'; import { Observable } from '../../../utilities'; import { ApolloProvider, resetApolloContext } from '../../../react'; -import { MockedProvider, mockSingleLink, wait, tick } from '../../../testing'; +import { + MockedProvider, + mockSingleLink, + wait, + tick, + MockSubscriptionLink +} from '../../../testing'; import { useLazyQuery } from '../useLazyQuery'; import { QueryResult } from '../../types/types'; @@ -452,10 +465,12 @@ describe('useLazyQuery Hook', () => { { request: { query: query1 }, result: { data: { hello: "world" } }, + delay: 20 }, { request: { query: query2 }, result: { data: { name: "changed" } }, + delay: 20 }, ]; @@ -1082,21 +1097,11 @@ describe('useLazyQuery Hook', () => { await wait(50); }); - it('aborts in-flight requests when component unmounts', async () => { - const query = gql` - query { - hello - } - `; - - const link = new ApolloLink(() => { - // Do nothing to prevent - return null - }); - + it('allows in-flight requests to resolve when component unmounts', async () => { + const link = new MockSubscriptionLink(); const client = new ApolloClient({ link, cache: new InMemoryCache() }) - const { result, unmount } = renderHook(() => useLazyQuery(query), { + const { result, unmount } = renderHook(() => useLazyQuery(helloQuery), { wrapper: ({ children }) => {children} @@ -1105,32 +1110,27 @@ describe('useLazyQuery Hook', () => { const [execute] = result.current; - let promise: Promise + let promise: Promise> act(() => { - promise = execute() + promise = execute(); }) unmount(); - await expect(promise!).rejects.toEqual( - new DOMException('The operation was aborted.', 'AbortError') - ); - }); + link.simulateResult({ result: { data: { hello: 'Greetings' }}}, true); - it('handles aborting multiple in-flight requests when component unmounts', async () => { - const query = gql` - query { - hello - } - `; + const queryResult = await promise!; - const link = new ApolloLink(() => { - return null - }); + expect(queryResult.data).toEqual({ hello: 'Greetings' }); + expect(queryResult.loading).toBe(false); + expect(queryResult.networkStatus).toBe(NetworkStatus.ready); + }); + it('handles resolving multiple in-flight requests when component unmounts', async () => { + const link = new MockSubscriptionLink(); const client = new ApolloClient({ link, cache: new InMemoryCache() }) - const { result, unmount } = renderHook(() => useLazyQuery(query), { + const { result, unmount } = renderHook(() => useLazyQuery(helloQuery), { wrapper: ({ children }) => {children} @@ -1139,8 +1139,8 @@ describe('useLazyQuery Hook', () => { const [execute] = result.current; - let promise1: Promise - let promise2: Promise + let promise1: Promise> + let promise2: Promise> act(() => { promise1 = execute(); promise2 = execute(); @@ -1148,12 +1148,246 @@ describe('useLazyQuery Hook', () => { unmount(); - const expectedError = new DOMException('The operation was aborted.', 'AbortError'); + link.simulateResult({ result: { data: { hello: 'Greetings' }}}, true); + + const expectedResult = { + data: { hello: 'Greetings' }, + loading: false, + networkStatus: NetworkStatus.ready, + }; + + await expect(promise1!).resolves.toMatchObject(expectedResult); + await expect(promise2!).resolves.toMatchObject(expectedResult); + }); + + // https://github.com/apollographql/apollo-client/issues/9755 + it('resolves each execution of the query with the appropriate result and renders with the result from the latest execution', async () => { + interface Data { + user: { id: string, name: string } + } + + interface Variables { + id: string + } + + const query: TypedDocumentNode = gql` + query UserQuery($id: ID!) { + user(id: $id) { + id + name + } + } + `; + + const mocks = [ + { + request: { query, variables: { id: '1' } }, + result: { data: { user: { id: '1', name: 'John Doe' }}}, + delay: 20 + }, + { + request: { query, variables: { id: '2' } }, + result: { data: { user: { id: '2', name: 'Jane Doe' }}}, + delay: 20 + }, + ] + + const { result } = renderHook(() => useLazyQuery(query), { + wrapper: ({ children }) => + + {children} + + }); + + const [execute] = result.current; + + await act(async () => { + const promise1 = execute({ variables: { id: '1' }}); + const promise2 = execute({ variables: { id: '2' }}); + + await expect(promise1).resolves.toMatchObject({ + ...mocks[0].result, + loading: false , + called: true, + }); + + await expect(promise2).resolves.toMatchObject({ + ...mocks[1].result, + loading: false , + called: true, + }); + }); + + expect(result.current[1]).toMatchObject({ + ...mocks[1].result, + loading: false, + called: true, + }); + }); + + it('uses the most recent options when the hook rerenders before execution', async () => { + interface Data { + user: { id: string, name: string } + } + + interface Variables { + id: string + } + + const query: TypedDocumentNode = gql` + query UserQuery($id: ID!) { + user(id: $id) { + id + name + } + } + `; + + const mocks = [ + { + request: { query, variables: { id: '1' } }, + result: { data: { user: { id: '1', name: 'John Doe' }}}, + delay: 30 + }, + { + request: { query, variables: { id: '2' } }, + result: { data: { user: { id: '2', name: 'Jane Doe' }}}, + delay: 20 + }, + ] + + const { result, rerender } = renderHook( + ({ id }) => useLazyQuery(query, { variables: { id } }), + { + initialProps: { id: '1' }, + wrapper: ({ children }) => + + {children} + + } + ); + + rerender({ id: '2' }); + + const [execute] = result.current; + + let promise: Promise>; + act(() => { + promise = execute(); + }); + + await waitFor(() => { + expect(result.current[1].data).toEqual(mocks[1].result.data); + }) - await expect(promise1!).rejects.toEqual(expectedError); - await expect(promise2!).rejects.toEqual(expectedError); + await expect(promise!).resolves.toMatchObject( + { data: mocks[1].result.data } + ); }); + // https://github.com/apollographql/apollo-client/issues/10198 + it('uses the most recent query document when the hook rerenders before execution', async () => { + const query = gql` + query DummyQuery { + shouldNotBeUsed + } + `; + + const mocks = [ + { + request: { query: helloQuery }, + result: { data: { hello: 'Greetings' } }, + delay: 20 + }, + ] + + const { result, rerender } = renderHook( + ({ query }) => useLazyQuery(query), + { + initialProps: { query }, + wrapper: ({ children }) => + + {children} + + } + ); + + rerender({ query: helloQuery }); + + const [execute] = result.current; + + let promise: Promise>; + act(() => { + promise = execute(); + }); + + await waitFor(() => { + expect(result.current[1].data).toEqual({ hello: 'Greetings' }); + }) + + await expect(promise!).resolves.toMatchObject( + { data: { hello: 'Greetings' } } + ); + }); + + it('does not refetch when rerendering after executing query', async () => { + interface Data { + user: { id: string, name: string } + } + + interface Variables { + id: string + } + + const query: TypedDocumentNode = gql` + query UserQuery($id: ID!) { + user(id: $id) { + id + name + } + } + `; + + let fetchCount = 0; + + const link = new ApolloLink((operation) => { + fetchCount++; + return new Observable((observer) => { + setTimeout(() => { + observer.next({ + data: { user: { id: operation.variables.id, name: 'John Doe' } } + }); + observer.complete(); + }, 20) + }); + }); + + const client = new ApolloClient({ link, cache: new InMemoryCache() }); + + const { result, rerender } = renderHook( + () => useLazyQuery(query, { variables: { id: '1' }}), + { + initialProps: { id: '1' }, + wrapper: ({ children }) => + + {children} + + } + ); + + const [execute] = result.current; + + await act(() => execute({ variables: { id: '2' }})); + + expect(fetchCount).toBe(1); + + rerender(); + + await wait(10); + + expect(fetchCount).toBe(1); + }) + describe("network errors", () => { async function check(errorPolicy: ErrorPolicy) { const networkError = new Error("from the network"); @@ -1163,7 +1397,7 @@ describe('useLazyQuery Hook', () => { link: new ApolloLink(request => new Observable(observer => { setTimeout(() => { observer.error(networkError); - }); + }, 20); })), }); diff --git a/src/react/hooks/useLazyQuery.ts b/src/react/hooks/useLazyQuery.ts index e0ffeab91dc..20fb1050e89 100644 --- a/src/react/hooks/useLazyQuery.ts +++ b/src/react/hooks/useLazyQuery.ts @@ -1,6 +1,6 @@ import { DocumentNode } from 'graphql'; import { TypedDocumentNode } from '@graphql-typed-document-node/core'; -import { useCallback, useEffect, useMemo, useRef } from 'react'; +import { useCallback, useMemo, useRef } from 'react'; import { OperationVariables } from '../../core'; import { mergeOptions } from '../../utilities'; @@ -27,14 +27,20 @@ export function useLazyQuery, options?: LazyQueryHookOptions ): LazyQueryResultTuple { - const abortControllersRef = useRef(new Set()); - const execOptionsRef = useRef>>(); + const optionsRef = useRef>(); + const queryRef = useRef>(); const merged = execOptionsRef.current ? mergeOptions(options, execOptionsRef.current) : options; + const document = merged?.query ?? query; + + // Use refs to track options and the used query to ensure the `execute` + // function remains referentially stable between renders. + optionsRef.current = merged; + queryRef.current = document; const internalState = useInternalState( useApolloClient(options && options.client), - merged?.query ?? query + document ); const useQueryResult = internalState.useQuery({ @@ -71,20 +77,9 @@ export function useLazyQuery { - return () => { - abortControllersRef.current.forEach((controller) => { - controller.abort(); - }); - } - }, []) - const execute = useCallback< LazyQueryResultTuple[0] >(executeOptions => { - const controller = new AbortController(); - abortControllersRef.current.add(controller); - execOptionsRef.current = executeOptions ? { ...executeOptions, fetchPolicy: executeOptions.fetchPolicy || initialFetchPolicy, @@ -92,17 +87,18 @@ export function useLazyQuery { - abortControllersRef.current.delete(controller); + const options = mergeOptions(optionsRef.current, { + query: queryRef.current, + ...execOptionsRef.current, + }) - return Object.assign(queryResult, eagerMethods); - }); + const promise = internalState + .executeQuery({ ...options, skip: false }) + .then((queryResult) => Object.assign(queryResult, eagerMethods)); - promise.catch(() => { - abortControllersRef.current.delete(controller); - }); + // Because the return value of `useLazyQuery` is usually floated, we need + // to catch the promise to prevent unhandled rejections. + promise.catch(() => {}); return promise; }, []); diff --git a/src/react/hooks/useQuery.ts b/src/react/hooks/useQuery.ts index 39430cb287b..084b68a15ce 100644 --- a/src/react/hooks/useQuery.ts +++ b/src/react/hooks/useQuery.ts @@ -30,7 +30,7 @@ import { import { DocumentType, verifyDocumentType } from '../parser'; import { useApolloClient } from './useApolloClient'; -import { canUseWeakMap, canUseWeakSet, compact, isNonEmptyArray, maybeDeepFreeze } from '../../utilities'; +import { canUseWeakMap, compact, isNonEmptyArray, maybeDeepFreeze } from '../../utilities'; const { prototype: { @@ -101,31 +101,46 @@ class InternalState { invariant.warn("Calling default no-op implementation of InternalState#forceUpdate"); } - asyncUpdate(signal: AbortSignal) { - return new Promise>((resolve, reject) => { - const watchQueryOptions = this.watchQueryOptions; - - const handleAborted = () => { - this.asyncResolveFns.delete(resolve) - this.optionsToIgnoreOnce.delete(watchQueryOptions); - signal.removeEventListener('abort', handleAborted) - reject(signal.reason); - }; - - this.asyncResolveFns.add(resolve); - this.optionsToIgnoreOnce.add(watchQueryOptions); - signal.addEventListener('abort', handleAborted) - this.forceUpdate(); - }); - } + executeQuery(options: QueryHookOptions) { + if (options.query) { + Object.assign(this, { query: options.query }) + } - private asyncResolveFns = new Set< - (result: QueryResult) => void - >(); + this.watchQueryOptions = this.createWatchQueryOptions( + this.queryHookOptions = options, + ); - private optionsToIgnoreOnce = new (canUseWeakSet ? WeakSet : Set)< - WatchQueryOptions - >(); + const concast = this.observable.reobserveAsConcast( + this.getObsQueryOptions() + ); + + // Make sure getCurrentResult returns a fresh ApolloQueryResult, + // but save the current data as this.previousData, just like setResult + // usually does. + this.previousData = this.result?.data || this.previousData; + this.result = void 0; + this.forceUpdate(); + + return new Promise>((resolve) => { + let result: ApolloQueryResult; + + // Subscribe to the concast independently of the ObservableQuery in case + // the component gets unmounted before the promise resolves. This prevents + // the concast from terminating early and resolving with `undefined` when + // there are no more subscribers for the concast. + concast.subscribe({ + next: (value) => { + result = value; + }, + error: () => { + resolve(this.toQueryResult(this.observable.getCurrentResult())); + }, + complete: () => { + resolve(this.toQueryResult(result)); + } + }); + }); + } // Methods beginning with use- should be called according to the standard // rules of React hooks: only at the top level of the calling function, and @@ -232,14 +247,7 @@ class InternalState { // TODO Remove this method when we remove support for options.partialRefetch. this.unsafeHandlePartialRefetch(result); - const queryResult = this.toQueryResult(result); - - if (!queryResult.loading && this.asyncResolveFns.size) { - this.asyncResolveFns.forEach(resolve => resolve(queryResult)); - this.asyncResolveFns.clear(); - } - - return queryResult; + return this.toQueryResult(result); } // These members (except for renderPromises) are all populated by the @@ -262,26 +270,10 @@ class InternalState { // this.watchQueryOptions elsewhere. const currentWatchQueryOptions = this.watchQueryOptions; - // To force this equality test to "fail," thereby reliably triggering - // observable.reobserve, add any current WatchQueryOptions object(s) you - // want to be ignored to this.optionsToIgnoreOnce. A similar effect could be - // achieved by nullifying this.watchQueryOptions so the equality test - // immediately fails because currentWatchQueryOptions is null, but this way - // we can promise a truthy this.watchQueryOptions at all times. - if ( - this.optionsToIgnoreOnce.has(currentWatchQueryOptions) || - !equal(watchQueryOptions, currentWatchQueryOptions) - ) { + if (!equal(watchQueryOptions, currentWatchQueryOptions)) { this.watchQueryOptions = watchQueryOptions; if (currentWatchQueryOptions && this.observable) { - // As advertised in the -Once of this.optionsToIgnoreOnce, this trick is - // only good for one forced execution of observable.reobserve per - // ignored WatchQueryOptions object, though it is unlikely we will ever - // see this exact currentWatchQueryOptions object again here, since we - // just replaced this.watchQueryOptions with watchQueryOptions. - this.optionsToIgnoreOnce.delete(currentWatchQueryOptions); - // Though it might be tempting to postpone this reobserve call to the // useEffect block, we need getCurrentResult to return an appropriate // loading:true result synchronously (later within the same call to