Fix problematic fetchMore notifyOnNetworkStatusChange renders

Prior to this commit, when `notifyOnNetworkStatusChange` is true
calls to `fetchMore` do not wait for `updateQuery` to finish,
before triggering a re-render. This means that after a `fetchMore`
call, components will flip to `loading: true`, then
`loading: false`,  then un-necessarily re-render once more as
`loading: false` with the result from `updateQuery`. This commit
adds a check to see if `fetchMore` has been called and then holds
off on re-rendering the `loading: false` state, until `updateQuery`
has fully finished.

Fixes #3333

Changelog.md

@@ -10,6 +10,8 @@
   [@n1ru4l](https://github.com/n1ru4l) in [#3356](https://github.com/apollographql/react-apollo/pull/3356)
 - Makes sure `refetch`, `fetchMore`, `updateQuery`, `startPolling`, `stopPolling`, and `subscribeToMore` maintain a stable identity when they're passed back alongside query results. <br/>
   [@hwillson](https://github.com/hwillson) in [#3422](https://github.com/apollographql/react-apollo/pull/3422)
+- Fixed problematic re-renders that were caused by using `fetchMore.updateQuery` with `notifyOnNetworkStatusChange` set to true. When `notifyOnNetworkStatusChange` is true, re-renders will now wait until `updateQuery` has completed, to make sure the updated data is used during the render. <br/>
+  [@hwillson](https://github.com/hwillson) in [#3433](https://github.com/apollographql/react-apollo/pull/3433)
 - Documentation fixes. <br/>
   [@SeanRoberts](https://github.com/SeanRoberts) in [#3380](https://github.com/apollographql/react-apollo/pull/3380)
 
@@ -74,6 +76,7 @@ Consult the [Hooks migration guide](https://www.apollographql.com/docs/react/hoo
   ```js
   import * as compose from 'lodash.flowright';
   ```
+
 - Render prop components (`Query`, `Mutation` and `Subscription`) can no longer be extended. In other words, this is no longer possible:
 
   ```js
@@ -94,7 +97,6 @@ Consult the [Hooks migration guide](https://www.apollographql.com/docs/react/hoo
   );
   ```
 
-
 ## 2.5.7 (2019-06-21)
 
 ### Improvements

examples/hooks/server/package.json

@@ -2,8 +2,8 @@
   "name": "hooks-demo-server",
   "description": "",
   "dependencies": {
-    "apollo-server": "2.9.0",
-    "apollo-server-express": "2.9.0",
+    "apollo-server": "2.9.1",
+    "apollo-server-express": "2.9.1",
     "express": "4.17.1",
     "graphql": "14.5.3",
     "subscriptions-transport-ws": "0.9.16"

packages/hoc/src/__tests__/queries/skip.test.tsx

@@ -1,5 +1,5 @@
 import React from 'react';
-import { render, cleanup } from '@testing-library/react';
+import { render, cleanup, wait } from '@testing-library/react';
 import gql from 'graphql-tag';
 import ApolloClient from 'apollo-client';
 import { ApolloLink } from 'apollo-link';
@@ -555,7 +555,7 @@ describe('[queries] skip', () => {
     );
   });
 
-  it('allows you to skip then unskip a query with opts syntax', done => {
+  it('allows you to skip then unskip a query with opts syntax', async () => {
     const query: DocumentNode = gql`
       query people {
         allPeople(first: 1) {
@@ -627,7 +627,6 @@ describe('[queries] skip', () => {
               break;
             case 4:
               expect(this.props.data!.loading).toBeFalsy();
-              done();
               break;
             default:
           }
@@ -654,9 +653,13 @@ describe('[queries] skip', () => {
         <Parent />
       </ApolloProvider>
     );
+
+    await wait(() => {
+      expect(count).toEqual(5);
+    });
   });
 
-  it('removes the injected props if skip becomes true', done => {
+  it('removes the injected props if skip becomes true', async () => {
     let count = 0;
     const query: DocumentNode = gql`
       query people($first: Int) {
@@ -697,17 +700,12 @@ describe('[queries] skip', () => {
       class extends React.Component<ChildProps<Vars, Data>> {
         componentDidUpdate() {
           const { data } = this.props;
-          try {
-            // loading is true, but data still there
-            if (count === 0)
-              expect(stripSymbols(data!.allPeople)).toEqual(data1.allPeople);
-            if (count === 1) expect(data).toBeUndefined();
-            if (count === 2 && !data!.loading) {
-              expect(stripSymbols(data!.allPeople)).toEqual(data3.allPeople);
-              done();
-            }
-          } catch (error) {
-            done.fail(error);
+          // loading is true, but data still there
+          if (count === 0)
+            expect(stripSymbols(data!.allPeople)).toEqual(data1.allPeople);
+          if (count === 1) expect(data).toBeUndefined();
+          if (count === 2 && !data!.loading) {
+            expect(stripSymbols(data!.allPeople)).toEqual(data3.allPeople);
           }
         }
         render() {
@@ -740,6 +738,10 @@ describe('[queries] skip', () => {
         <ChangingProps />
       </ApolloProvider>
     );
+
+    await wait(() => {
+      expect(count).toEqual(2);
+    });
   });
 
   it('allows you to unmount a skipped query', done => {

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

@@ -346,8 +346,6 @@ describe('useQuery Hook', () => {
           }
         );
 
-        console.log('ns', networkStatus);
-
         switch (renderCount) {
           case 0:
             expect(loading).toBeTruthy();
@@ -437,4 +435,207 @@ describe('useQuery Hook', () => {
       );
     });
   });
+
+  describe('Pagination', () => {
+    it(
+      'should render `fetchMore.updateQuery` updated results with proper ' +
+        'loading status, when `notifyOnNetworkStatusChange` is true',
+      async () => {
+        const carQuery: DocumentNode = gql`
+          query cars($limit: Int) {
+            cars(limit: $limit) {
+              id
+              make
+              model
+              vin
+              __typename
+            }
+          }
+        `;
+
+        const carResults = {
+          cars: [
+            {
+              id: 1,
+              make: 'Audi',
+              model: 'RS8',
+              vin: 'DOLLADOLLABILL',
+              __typename: 'Car'
+            }
+          ]
+        };
+
+        const moreCarResults = {
+          cars: [
+            {
+              id: 2,
+              make: 'Audi',
+              model: 'eTron',
+              vin: 'TREESRGOOD',
+              __typename: 'Car'
+            }
+          ]
+        };
+
+        const mocks = [
+          {
+            request: { query: carQuery, variables: { limit: 1 } },
+            result: { data: carResults }
+          },
+          {
+            request: { query: carQuery, variables: { limit: 1 } },
+            result: { data: moreCarResults }
+          }
+        ];
+
+        let renderCount = 0;
+        function App() {
+          const { loading, data, fetchMore } = useQuery(carQuery, {
+            variables: { limit: 1 },
+            notifyOnNetworkStatusChange: true
+          });
+
+          switch (renderCount) {
+            case 0:
+              expect(loading).toBeTruthy();
+              break;
+            case 1:
+              expect(loading).toBeFalsy();
+              expect(data).toEqual(carResults);
+              fetchMore({
+                variables: {
+                  limit: 1
+                },
+                updateQuery: (prev, { fetchMoreResult }) => ({
+                  cars: [...prev.cars, ...fetchMoreResult.cars]
+                })
+              });
+              break;
+            case 2:
+              expect(loading).toBeTruthy();
+              break;
+            case 3:
+              expect(loading).toBeFalsy();
+              expect(data).toEqual({
+                cars: [carResults.cars[0], moreCarResults.cars[0]]
+              });
+              break;
+            default:
+          }
+
+          renderCount += 1;
+          return null;
+        }
+
+        render(
+          <MockedProvider mocks={mocks}>
+            <App />
+          </MockedProvider>
+        );
+
+        await wait(() => {
+          expect(renderCount).toBe(4);
+        });
+      }
+    );
+
+    it(
+      'should render `fetchMore.updateQuery` updated results with no ' +
+        'loading status, when `notifyOnNetworkStatusChange` is false',
+      async () => {
+        const carQuery: DocumentNode = gql`
+          query cars($limit: Int) {
+            cars(limit: $limit) {
+              id
+              make
+              model
+              vin
+              __typename
+            }
+          }
+        `;
+
+        const carResults = {
+          cars: [
+            {
+              id: 1,
+              make: 'Audi',
+              model: 'RS8',
+              vin: 'DOLLADOLLABILL',
+              __typename: 'Car'
+            }
+          ]
+        };
+
+        const moreCarResults = {
+          cars: [
+            {
+              id: 2,
+              make: 'Audi',
+              model: 'eTron',
+              vin: 'TREESRGOOD',
+              __typename: 'Car'
+            }
+          ]
+        };
+
+        const mocks = [
+          {
+            request: { query: carQuery, variables: { limit: 1 } },
+            result: { data: carResults }
+          },
+          {
+            request: { query: carQuery, variables: { limit: 1 } },
+            result: { data: moreCarResults }
+          }
+        ];
+
+        let renderCount = 0;
+        function App() {
+          const { loading, data, fetchMore } = useQuery(carQuery, {
+            variables: { limit: 1 },
+            notifyOnNetworkStatusChange: false
+          });
+
+          switch (renderCount) {
+            case 0:
+              expect(loading).toBeTruthy();
+              break;
+            case 1:
+              expect(loading).toBeFalsy();
+              expect(data).toEqual(carResults);
+              fetchMore({
+                variables: {
+                  limit: 1
+                },
+                updateQuery: (prev, { fetchMoreResult }) => ({
+                  cars: [...prev.cars, ...fetchMoreResult.cars]
+                })
+              });
+              break;
+            case 2:
+              expect(loading).toBeFalsy();
+              expect(data).toEqual({
+                cars: [carResults.cars[0], moreCarResults.cars[0]]
+              });
+              break;
+            default:
+          }
+
+          renderCount += 1;
+          return null;
+        }
+
+        render(
+          <MockedProvider mocks={mocks}>
+            <App />
+          </MockedProvider>
+        );
+
+        await wait(() => {
+          expect(renderCount).toBe(3);
+        });
+      }
+    );
+  });
 });

packages/hooks/src/data/QueryData.ts

@@ -261,13 +261,32 @@ export class QueryData<TData, TVariables> extends OperationData {
     const obsQuery = this.currentObservable.query!;
     this.currentObservable.subscription = obsQuery.subscribe({
       next: ({ loading, networkStatus, data }) => {
-        if (
-          this.previousData.result &&
-          this.previousData.result.loading === loading &&
-          this.previousData.result.networkStatus === networkStatus &&
-          isEqual(this.previousData.result.data, data)
-        ) {
-          return;
+        const previousResult = this.previousData.result;
+
+        if (previousResult) {
+          // Calls to `ObservableQuery.fetchMore` return a result before the
+          // `updateQuery` function fully finishes. This can lead to an
+          // extra un-necessary re-render since the initially returned data is
+          // the same as data that has already been rendered. We'll
+          // prevent the un-necessary render from happening, making sure
+          // `fetchMore` results are only rendered when `updateQuery` has
+          // completed.
+          if (
+            previousResult.loading &&
+            previousResult.networkStatus === NetworkStatus.fetchMore &&
+            isEqual(previousResult.data, data)
+          ) {
+            return;
+          }
+
+          // Make sure we're not attempting to re-render similar results
+          if (
+            previousResult.loading === loading &&
+            previousResult.networkStatus === networkStatus &&
+            isEqual(previousResult.data, data)
+          ) {
+            return;
+          }
         }
 
         this.forceUpdate();