Fix component stuck in loading state for network-only fetch policy (#3126)

jasonpaulos · hwillson · commit 2b7073b870dc · 2019-06-21T12:28:47.000-04:00
* Add ability for Query to detect when lastResult is not accurate * Add regression test for #2899 * Streamline interactions with Apollo Client This commit streamlines some of the React Apollo <--> Apollo Client communication points, to help reduce temporary placeholders and variables used by React Apollo to control rendering. The current data result that is to be rendered now comes from only one place, the initialized `ObservableQuery` instance. * Code review tweaks * Changelog update
diff --git a/Changelog.md b/Changelog.md
@@ -31,9 +31,11 @@
   responses couldn't be handled). The `Query` component can now handle an
   error in a response, then continue to handle a valid response afterwards.  <br/>
   [@hwillson](https://github.com/hwillson) in [#3107](https://github.com/apollographql/react-apollo/pull/3107)
-- Reorder `Subscription` component code to avoid setting state on unmounted 
+- Reorder `Subscription` component code to avoid setting state on unmounted
   component.  <br/>
   [@jasonpaulos](https://github.com/jasonpaulos) in [#3139](https://github.com/apollographql/react-apollo/pull/3139)
+- Fix component stuck in `loading` state for `network-only` fetch policy.  <br/>
+  [@jasonpaulos](https://github.com/jasonpaulos) in [#3126](https://github.com/apollographql/react-apollo/pull/3126)
 
 
 ## 2.5.6 (2019-05-22)
diff --git a/src/Query.tsx b/src/Query.tsx
@@ -7,6 +7,7 @@ import ApolloClient, {
   NetworkStatus,
   FetchMoreOptions,
   FetchMoreQueryOptions,
+  ApolloCurrentResult
 } from 'apollo-client';
 import { DocumentNode } from 'graphql';
 import { ZenObservable } from 'zen-observable-ts';
@@ -115,7 +116,6 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
   // only delete queryObservable when we unmount the component.
   private queryObservable?: ObservableQuery<TData, TVariables> | null;
   private querySubscription?: ZenObservable.Subscription;
-  private previousData: any = {};
   private refetcherQueue?: {
     args: any;
     resolve: (value?: any | PromiseLike<any>) => void;
@@ -124,7 +124,7 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
 
   private hasMounted: boolean = false;
   private operation?: IDocumentDefinition;
-  private lastResult: ApolloQueryResult<TData> | null = null;
+  private lastRenderedResult: ApolloQueryResult<TData> | null = null;
 
   constructor(props: QueryProps<TData, TVariables>, context: QueryContext) {
     super(props, context);
@@ -175,7 +175,7 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
     this.hasMounted = true;
     if (this.props.skip) return;
 
-    this.startQuerySubscription(true);
+    this.startQuerySubscription();
     if (this.refetcherQueue) {
       const { args, resolve, reject } = this.refetcherQueue;
       this.queryObservable!.refetch(args)
@@ -187,6 +187,7 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
   componentWillReceiveProps(nextProps: QueryProps<TData, TVariables>, nextContext: QueryContext) {
     // the next render wants to skip
     if (nextProps.skip && !this.props.skip) {
+      this.queryObservable!.resetLastResults();
       this.removeQuerySubscription();
       return;
     }
@@ -201,11 +202,10 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
       this.client = nextClient;
       this.removeQuerySubscription();
       this.queryObservable = null;
-      this.previousData = {};
-      this.updateQuery(nextProps);
     }
 
     if (this.props.query !== nextProps.query) {
+      this.queryObservable!.resetLastResults();
       this.removeQuerySubscription();
     }
 
@@ -300,52 +300,28 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
       .catch(() => null);
   }
 
-  private startQuerySubscription = (justMounted: boolean = false) => {
+  private startQuerySubscription = () => {
     // When the `Query` component receives new props, or when we explicitly
     // re-subscribe to a query using `resubscribeToQuery`, we start a new
     // subscription in this method. To avoid un-necessary re-renders when
     // receiving new props or re-subscribing, we track the full last
     // observable result so it can be compared against incoming new data.
     // We only trigger a re-render if the incoming result is different than
-    // the stored `lastResult`.
-    //
-    // It's important to note that when a component is first mounted,
-    // the `startQuerySubscription` method is also triggered. During a first
-    // mount, we don't want to store or use the last result, as we always
-    // need the first render to happen, even if there was a previous last
-    // result (which can happen when the same component is mounted, unmounted,
-    // and mounted again).
-    if (!justMounted) {
-      this.lastResult = this.queryObservable!.getLastResult();
-    }
+    // the stored `lastRenderedResult`.
 
     if (this.querySubscription) return;
 
-    // store the initial renders worth of result
-    let initial: QueryResult<TData, TVariables> | undefined = this.getQueryResult();
-
     this.querySubscription = this.queryObservable!.subscribe({
-      next: ({ loading, networkStatus, data }) => {
-        // to prevent a quick second render from the subscriber
-        // we compare to see if the original started finished (from cache) and is unchanged
-        if (initial && initial.networkStatus === 7 && shallowEqual(initial.data, data)) {
-          initial = undefined;
-          return;
-        }
-
+      next: (result) => {
         if (
-          this.lastResult &&
-          this.lastResult.loading === loading &&
-          this.lastResult.networkStatus === networkStatus &&
-          shallowEqual(this.lastResult.data, data)
+          this.lastRenderedResult &&
+          this.lastRenderedResult.loading === result.loading &&
+          this.lastRenderedResult.networkStatus === result.networkStatus &&
+          shallowEqual(this.lastRenderedResult.data, result.data)
         ) {
           return;
         }
 
-        initial = undefined;
-        if (this.lastResult) {
-          this.lastResult = this.queryObservable!.getLastResult();
-        }
         this.updateCurrentData();
       },
       error: error => {
@@ -359,8 +335,8 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
 
   private removeQuerySubscription = () => {
     if (this.querySubscription) {
-      this.lastResult = this.queryObservable!.getLastResult();
       this.querySubscription.unsubscribe();
+      delete this.lastRenderedResult;
       delete this.querySubscription;
     }
   };
@@ -403,22 +379,21 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
   }
 
   private getQueryResult = (): QueryResult<TData, TVariables> => {
-    let data = { data: Object.create(null) as TData } as any;
+    let result = { data: Object.create(null) as TData } as any;
     // Attach bound methods
-    Object.assign(data, observableQueryFields(this.queryObservable!));
+    Object.assign(result, observableQueryFields(this.queryObservable!));
 
     // When skipping a query (ie. we're not querying for data but still want
     // to render children), make sure the `data` is cleared out and
     // `loading` is set to `false` (since we aren't loading anything).
     if (this.props.skip) {
-      data = {
-        ...data,
+      result = {
+        ...result,
         data: undefined,
         error: undefined,
         loading: false,
       };
     } else {
-      // Fetch the current result (if any) from the store.
       const currentResult = this.queryObservable!.currentResult();
       const { loading, partial, networkStatus, errors } = currentResult;
       let { error } = currentResult;
@@ -430,12 +405,15 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
       }
 
       const { fetchPolicy } = this.queryObservable!.options;
-      Object.assign(data, { loading, networkStatus, error });
+      Object.assign(result, { loading, networkStatus, error });
+
+      const previousData =
+        this.lastRenderedResult ? this.lastRenderedResult.data : {};
 
       if (loading) {
-        Object.assign(data.data, this.previousData, currentResult.data);
+        Object.assign(result.data, previousData, currentResult.data);
       } else if (error) {
-        Object.assign(data, {
+        Object.assign(result, {
           data: (this.queryObservable!.getLastResult() || {}).data,
         });
       } else if (
@@ -444,11 +422,13 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
       ) {
         // Make sure data pulled in by a `no-cache` query is preserved
         // when the components parent tree is re-rendered.
-        data.data = this.previousData;
+        result.data = previousData;
       } else {
         const { partialRefetch } = this.props;
         if (
           partialRefetch &&
+          currentResult.data !== null &&
+          typeof currentResult.data === 'object' &&
           Object.keys(currentResult.data).length === 0 &&
           partial &&
           fetchPolicy !== 'cache-only'
@@ -461,13 +441,13 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
           // the original `Query` component are expecting certain data values to
           // exist, and they're all of a sudden stripped away. To help avoid
           // this we'll attempt to refetch the `Query` data.
-          Object.assign(data, { loading: true, networkStatus: NetworkStatus.loading });
-          data.refetch();
-          return data;
+          Object.assign(result, { loading: true, networkStatus: NetworkStatus.loading });
+          result.refetch();
+          this.lastRenderedResult = result;
+          return result;
         }
 
-        Object.assign(data.data, currentResult.data);
-        this.previousData = currentResult.data;
+        Object.assign(result.data, currentResult.data);
       }
     }
 
@@ -491,9 +471,9 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
     // always hit the network with refetch, since the components data will be
     // updated and a network request is not currently active.
     if (!this.querySubscription) {
-      const oldRefetch = (data as QueryControls<TData, TVariables>).refetch;
+      const oldRefetch = (result as QueryControls<TData, TVariables>).refetch;
 
-      (data as QueryControls<TData, TVariables>).refetch = args => {
+      (result as QueryControls<TData, TVariables>).refetch = args => {
         if (this.querySubscription) {
           return oldRefetch(args);
         } else {
@@ -512,7 +492,8 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
       this.queryObservable!.resetQueryStoreErrors();
     });
 
-    data.client = this.client;
-    return data;
+    result.client = this.client;
+    this.lastRenderedResult = result;
+    return result;
   };
 }
diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx
@@ -524,14 +524,18 @@ describe('Query component', () => {
 
   describe('props allow', () => {
     it('custom fetch-policy', done => {
+      let count = 0;
       const Component = () => (
         <Query query={allPeopleQuery} fetchPolicy={'cache-only'}>
           {result => {
-            catchAsyncError(done, () => {
-              expect(result.loading).toBeFalsy();
-              expect(result.networkStatus).toBe(NetworkStatus.ready);
-              done();
-            });
+            if (count === 0) {
+              catchAsyncError(done, () => {
+                expect(result.loading).toBeFalsy();
+                expect(result.networkStatus).toBe(NetworkStatus.ready);
+                done();
+              });
+            }
+            count += 1;
             return null;
           }}
         </Query>
@@ -545,14 +549,18 @@ describe('Query component', () => {
     });
 
     it('default fetch-policy', done => {
+      let count = 0;
       const Component = () => (
         <Query query={allPeopleQuery}>
           {result => {
-            catchAsyncError(done, () => {
-              expect(result.loading).toBeFalsy();
-              expect(result.networkStatus).toBe(NetworkStatus.ready);
-              done();
-            });
+            if (count === 0) {
+              catchAsyncError(done, () => {
+                expect(result.loading).toBeFalsy();
+                expect(result.networkStatus).toBe(NetworkStatus.ready);
+                done();
+              });
+            }
+            count += 1;
             return null;
           }}
         </Query>
diff --git a/test/client/graphql/mutations/recycled-queries.test.tsx b/test/client/graphql/mutations/recycled-queries.test.tsx
@@ -128,7 +128,7 @@ describe('graphql(mutation) update queries', () => {
 
           render() {
             try {
-              switch (queryRenderCount++) {
+              switch (queryRenderCount) {
                 case 0:
                   expect(this.props.data!.loading).toBeTruthy();
                   expect(this.props.data!.todo_list).toBeFalsy();
@@ -141,21 +141,6 @@ describe('graphql(mutation) update queries', () => {
                   });
                   break;
                 case 2:
-                  expect(queryMountCount).toBe(1);
-                  expect(queryUnmountCount).toBe(0);
-                  expect(stripSymbols(this.props.data!.todo_list)).toEqual({
-                    id: '123',
-                    title: 'how to apollo',
-                    tasks: [
-                      {
-                        id: '99',
-                        text: 'This one was created with a mutation.',
-                        completed: true,
-                      },
-                    ],
-                  });
-                  break;
-                case 3:
                   expect(queryMountCount).toBe(2);
                   expect(queryUnmountCount).toBe(1);
                   expect(stripSymbols(this.props.data!.todo_list)).toEqual({
@@ -175,7 +160,7 @@ describe('graphql(mutation) update queries', () => {
                     ],
                   });
                   break;
-                case 4:
+                case 3:
                   expect(stripSymbols(this.props.data!.todo_list)).toEqual({
                     id: '123',
                     title: 'how to apollo',
@@ -196,6 +181,7 @@ describe('graphql(mutation) update queries', () => {
                 default:
                   throw new Error('Rendered too many times');
               }
+              queryRenderCount += 1;
             } catch (error) {
               reject(error);
             }
@@ -247,7 +233,7 @@ describe('graphql(mutation) update queries', () => {
                   expect(todoUpdateQueryCount).toBe(2);
                   expect(queryMountCount).toBe(2);
                   expect(queryUnmountCount).toBe(2);
-                  expect(queryRenderCount).toBe(4);
+                  expect(queryRenderCount).toBe(3);
                   resolve();
                 } catch (error) {
                   reject(error);
diff --git a/test/client/graphql/queries/lifecycle.test.tsx b/test/client/graphql/queries/lifecycle.test.tsx
@@ -581,20 +581,8 @@ describe('[queries] lifecycle', () => {
       { loading: false, a: 7, b: 8, c: 9 },
 
       // Load 6
-
-      // The first render is caused by the component having its state updated
-      // when switching the client. The 2nd and 3rd renders are caused by the
-      // component re-subscribing to get data from Apollo Client.
-      { loading: false, a: 1, b: 2, c: 3 },
-      { loading: false, a: 1, b: 2, c: 3 },
       { loading: false, a: 1, b: 2, c: 3 },
-
-      { loading: false, a: 4, b: 5, c: 6 },
       { loading: false, a: 4, b: 5, c: 6 },
-      { loading: false, a: 4, b: 5, c: 6 },
-
-      { loading: false, a: 7, b: 8, c: 9 },
-      { loading: false, a: 7, b: 8, c: 9 },
       { loading: false, a: 7, b: 8, c: 9 },
     ]);
   });
diff --git a/test/client/graphql/queries/loading.test.tsx b/test/client/graphql/queries/loading.test.tsx
