From eb252b248041736d93f4f057c80a751b392ec5fd Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Sun, 12 Jul 2020 14:55:56 -0400 Subject: [PATCH] Simulate loading result for original query before fetchMore. Should fix #6354, #6542, #6534, and #6459. Reliable delivery of loading results is one of the core benefits that Apollo Client strives to provide, especially since handling loading states tends to be such an annoying, error-prone task in hand-written state management code, and Apollo Client is all about keeping hand-written state management code to a minimum. Ever since I refactored the FetchPolicy system in #6221, the fetchMore method of ObservableQuery has not been delivering a loading state before sending its request. Instead, the observed query was updated only once, after the completion of the fetchMore network request. I've been aware of this problem for a while now, but I procrastinated solving it because I knew it would be, well, annoying. With the final release of AC3 right around the corner (Tuesday!), the time has come to get this right. This loading result doesn't fit neatly into the fetchQueryObservable system introduced by #6221, since the consumer of the loading result is the original ObservableQuery, but the network request gets made with a fresh query ID, using different variables (and possibly even a different query) passed to fetchMore. This separation is important so that we don't have to change the query/variables/options of the original ObservableQuery for the duration of the fetchMore request. Instead, the fetchMore request is a one-off, independent request that effectively bypasses the usual FetchPolicy system (technically, it always uses the no-cache FetchPolicy). In Apollo Client 2.x (and before #6221 was released in beta.46), this logic was guided by an extra fetchMoreForQueryId parameter passed to QueryManager#fetchQuery, which ended up appearing in a number (16) of different places, across three different methods of the QueryManager. I think it's an improvement that the logic is now confined to one block of code in ObservableQuery#fetchMore, which seems naturally responsible for any fetchMore-related logic. Still, I don't love the precedent that this simulated loading state sets, so I hope we can avoid similar hacks in the future. --- src/__tests__/fetchMore.ts | 12 ++++++-- src/core/ObservableQuery.ts | 33 +++++++++++++++++++++ src/react/hooks/__tests__/useQuery.test.tsx | 28 ++++++++++++++--- 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/src/__tests__/fetchMore.ts b/src/__tests__/fetchMore.ts index 17aeecd18df..56e1333a926 100644 --- a/src/__tests__/fetchMore.ts +++ b/src/__tests__/fetchMore.ts @@ -990,9 +990,13 @@ describe('fetchMore on an observable query with connection', () => { }); break; case 1: + expect(networkStatus).toBe(NetworkStatus.fetchMore); + expect((data as any).entry.comments.length).toBe(10); + break; + case 2: expect(networkStatus).toBe(NetworkStatus.ready); expect((data as any).entry.comments.length).toBe(20); - resolve(); + setTimeout(resolve, 10); break; default: reject(new Error('`next` called too many times')); @@ -1045,9 +1049,13 @@ describe('fetchMore on an observable query with connection', () => { }); break; case 1: + expect(networkStatus).toBe(NetworkStatus.fetchMore); + expect((data as any).entry.comments.length).toBe(10); + break; + case 2: expect(networkStatus).toBe(NetworkStatus.ready); expect((data as any).entry.comments.length).toBe(20); - resolve(); + setTimeout(resolve, 10); break; default: reject(new Error('`next` called too many times')); diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 9918ac66234..ba194b69934 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -305,6 +305,39 @@ export class ObservableQuery< const qid = this.queryManager.generateQueryId(); + if (this.options.notifyOnNetworkStatusChange) { + const currentResult = this.getCurrentResult(); + const queryInfo = this.queryManager.getQueryStoreValue(this.queryId); + if (queryInfo) { + // If we neglect to update queryInfo.networkStatus here, + // getCurrentResult may return a loading:false result while + // fetchMore is in progress, since getCurrentResult also consults + // queryInfo.networkStatus. Note: setting queryInfo.networkStatus + // to an in-flight status means that QueryInfo#shouldNotify will + // return false while fetchMore is in progress, which is why we + // call this.reobserve() explicitly in the .finally callback after + // fetchMore (below), since the cache write will not automatically + // trigger a notification, even though it does trigger a cache + // broadcast. This is a good thing, because it means we won't see + // intervening query notifications while fetchMore is pending. + queryInfo.networkStatus = NetworkStatus.fetchMore; + } + // Simulate a loading result for the original query with + // networkStatus === NetworkStatus.fetchMore. + this.observer.next!({ + // Note that currentResult is an ApolloCurrentQueryResult, + // whereas this.observer.next expects an ApolloQueryResult. + // Fortunately, ApolloCurrentQueryResult is a subtype of + // ApolloQueryResult (with additional .error and .partial fields), + // so TypeScript has no problem with this sleight of hand. + // TODO Consolidate these two types into a single type (most + // likely just ApolloQueryResult) after AC3 is released. + ...currentResult, + loading: true, + networkStatus: NetworkStatus.fetchMore, + }); + } + return this.queryManager.fetchQuery( qid, combinedOptions, diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index a769f1d186e..b2755292f5f 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -1169,7 +1169,7 @@ describe('useQuery Hook', () => { itAsync('updateQuery', (resolve, reject) => { let renderCount = 0; function App() { - const { loading, data, fetchMore } = useQuery(carQuery, { + const { loading, networkStatus, data, fetchMore } = useQuery(carQuery, { variables: { limit: 1 }, notifyOnNetworkStatusChange: true }); @@ -1177,9 +1177,12 @@ describe('useQuery Hook', () => { switch (++renderCount) { case 1: expect(loading).toBeTruthy(); + expect(networkStatus).toBe(NetworkStatus.loading); + expect(data).toBeUndefined(); break; case 2: expect(loading).toBeFalsy(); + expect(networkStatus).toBe(NetworkStatus.ready); expect(data).toEqual(carResults); fetchMore({ variables: { @@ -1194,7 +1197,13 @@ describe('useQuery Hook', () => { }); break; case 3: + expect(loading).toBeTruthy(); + expect(networkStatus).toBe(NetworkStatus.fetchMore); + expect(data).toEqual(carResults); + break; + case 4: expect(loading).toBeFalsy(); + expect(networkStatus).toBe(NetworkStatus.ready); expect(data).toEqual({ cars: [ carResults.cars[0], @@ -1203,6 +1212,7 @@ describe('useQuery Hook', () => { }); break; default: + reject("too many updates"); } return null; @@ -1215,14 +1225,14 @@ describe('useQuery Hook', () => { ); return wait(() => { - expect(renderCount).toBe(3); + expect(renderCount).toBe(4); }).then(resolve, reject); }); itAsync('field policy', (resolve, reject) => { let renderCount = 0; function App() { - const { loading, data, fetchMore } = useQuery(carQuery, { + const { loading, networkStatus, data, fetchMore } = useQuery(carQuery, { variables: { limit: 1 }, notifyOnNetworkStatusChange: true }); @@ -1230,9 +1240,12 @@ describe('useQuery Hook', () => { switch (++renderCount) { case 1: expect(loading).toBeTruthy(); + expect(networkStatus).toBe(NetworkStatus.loading); + expect(data).toBeUndefined(); break; case 2: expect(loading).toBeFalsy(); + expect(networkStatus).toBe(NetworkStatus.ready); expect(data).toEqual(carResults); fetchMore({ variables: { @@ -1241,7 +1254,13 @@ describe('useQuery Hook', () => { }); break; case 3: + expect(loading).toBeTruthy(); + expect(networkStatus).toBe(NetworkStatus.fetchMore); + expect(data).toEqual(carResults); + break; + case 4: expect(loading).toBeFalsy(); + expect(networkStatus).toBe(NetworkStatus.ready); expect(data).toEqual({ cars: [ carResults.cars[0], @@ -1250,6 +1269,7 @@ describe('useQuery Hook', () => { }); break; default: + reject("too many updates"); } return null; @@ -1272,7 +1292,7 @@ describe('useQuery Hook', () => { ); return wait(() => { - expect(renderCount).toBe(3); + expect(renderCount).toBe(4); }).then(resolve, reject); }); });