Simulate loading result for original query before fetchMore. (#6583)

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.

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'));

src/core/ObservableQuery.ts

@@ -305,6 +305,39 @@ export class ObservableQuery<
 
     const qid = this.queryManager.generateQueryId();
 
+    if (combinedOptions.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<TData>,
+        // whereas this.observer.next expects an ApolloQueryResult<TData>.
+        // 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,

src/react/hooks/__tests__/useQuery.test.tsx

@@ -1169,17 +1169,20 @@ 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
           });
 
           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,24 +1225,27 @@ 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
           });
 
           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);
       });
     });