Fix infinite loop caused by set state in onError / onCompleted props of Query

Changelog.md

@@ -2,6 +2,12 @@
 
 ## vNEXT
 
+### Bug Fixes
+
+- Fix an infinite loop caused by using `setState` in the
+  `onError` / `onCompleted` callbacks of the `Query` component.  <br/>
+  [@chenesan](https://github.com/chenesan) in [#2751](https://github.com/apollographql/react-apollo/pull/2751)
+
 ### Improvements
 
 - `MockedProvider` now accepts a `childProps` prop that can be used to pass

package.json

@@ -52,7 +52,7 @@
   "bundlesize": [
     {
       "path": "./dist/bundlesize.js",
-      "maxSize": "5.2 KB"
+      "maxSize": "5.3 KB"
     }
   ],
   "renovate": {
@@ -113,6 +113,7 @@
     "@types/hoist-non-react-statics": "3.0.1",
     "@types/invariant": "2.2.29",
     "@types/jest": "24.0.9",
+    "@types/lodash.isequal": "^4.5.4",
     "@types/object-assign": "4.0.30",
     "@types/prop-types": "15.7.0",
     "@types/react": "16.4.18",

rollup.config.js

@@ -20,6 +20,7 @@ const globals = {
   'react': 'react',
   'ts-invariant': 'invariant',
   'tslib': 'tslib',
+  'lodash.isequal': 'isEqual',
 };
 
 function external(id) {

src/Query.tsx

@@ -15,6 +15,7 @@ import { parser, DocumentType, IDocumentDefinition } from './parser';
 import { getClient } from './component-utils';
 import { RenderPromises } from './getDataFromTree';
 
+import isEqual from 'lodash.isequal';
 import shallowEqual from './utils/shallowEqual';
 import { invariant } from 'ts-invariant';
 
@@ -217,16 +218,14 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
     this.hasMounted = false;
   }
 
-  componentDidUpdate() {
-    const { onCompleted, onError } = this.props;
-    if (onCompleted || onError) {
-      const currentResult = this.queryObservable!.currentResult();
-      const { loading, error, data } = currentResult;
-      if (onCompleted && !loading && !error) {
-        onCompleted(data as TData);
-      } else if (onError && !loading && error) {
-        onError(error);
-      }
+  componentDidUpdate(prevProps: QueryProps<TData, TVariables>) {
+    const isDiffRequest =
+      !isEqual(prevProps.query, this.props.query) ||
+      !isEqual(prevProps.variables, this.props.variables);
+    if (isDiffRequest) {
+      // If specified, `onError` / `onCompleted` callbacks are called here
+      // after local cache results are loaded.
+      this.handleErrorOrCompleted();
     }
   }
 
@@ -379,10 +378,25 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
   }
 
   private updateCurrentData = () => {
-    // force a rerender that goes through shouldComponentUpdate
+    // If specified, `onError` / `onCompleted` callbacks are called here
+    // after a network based Query result has been received.
+    this.handleErrorOrCompleted();
+
+    // Force a rerender that goes through shouldComponentUpdate.
     if (this.hasMounted) this.forceUpdate();
   };
 
+  private handleErrorOrCompleted = () => {
+    const result = this.queryObservable!.currentResult();
+    const { data, loading, error } = result;
+    const { onCompleted, onError } = this.props;
+    if (onCompleted && !loading && !error) {
+      onCompleted(data as TData);
+    } else if (onError && !loading && error) {
+      onError(error);
+    }
+  }
+
   private getQueryResult = (): QueryResult<TData, TVariables> => {
     let data = { data: Object.create(null) as TData } as any;
     // Attach bound methods

test/client/Query.test.tsx

@@ -1387,6 +1387,198 @@ describe('Query component', () => {
     );
   });
 
+  it('should not repeatedly call onCompleted if setState in it', done => {
+    const query = gql`
+      query people($first: Int) {
+        allPeople(first: $first) {
+          people {
+            name
+          }
+        }
+      }
+    `;
+
+    const data1 = { allPeople: { people: [{ name: 'Luke Skywalker' }] } };
+    const data2 = { allPeople: { people: [{ name: 'Han Solo' }] } };
+    const mocks = [
+      {
+        request: { query, variables: { first: 1 } },
+        result: { data: data1 },
+      },
+      {
+        request: { query, variables: { first: 2 } },
+        result: { data: data2 },
+      },
+    ];
+
+    let onCompletedCallCount = 0, updateCount = 0;
+    class Component extends React.Component {
+      state = {
+        variables: {
+          first: 1
+        }
+      }
+      onCompleted = () => {
+        onCompletedCallCount += 1;
+        this.setState({ causeUpdate: true });
+      }
+      componentDidUpdate() {
+        updateCount += 1;
+        if (updateCount === 1) {
+          // `componentDidUpdate` in `Query` is triggered by the `setState`
+          // in `onCompleted`. It will be called before `componentDidUpdate`
+          // in `Component`. `onCompleted` should have been called only once
+          // in the entire lifecycle.
+          expect(onCompletedCallCount).toBe(1);
+          done();
+        }
+      }
+      render() {
+        return (
+          <Query query={query} variables={this.state.variables} onCompleted={this.onCompleted}>
+            {() => null}
+          </Query>
+        );
+      }
+    }
+
+    wrapper = mount(
+      <MockedProvider mocks={mocks} addTypename={false}>
+        <Component />
+      </MockedProvider>,
+    );
+  });
+
+  it('should not repeatedly call onCompleted when cache exists if setState in it', done => {
+    const query = gql`
+      query people($first: Int) {
+        allPeople(first: $first) {
+          people {
+            name
+          }
+        }
+      }
+    `;
+
+    const data1 = { allPeople: { people: [{ name: 'Luke Skywalker' }] } };
+    const data2 = { allPeople: { people: [{ name: 'Han Solo' }] } };
+    const mocks = [
+      {
+        request: { query, variables: { first: 1 } },
+        result: { data: data1 },
+      },
+      {
+        request: { query, variables: { first: 2 } },
+        result: { data: data2 },
+      },
+    ];
+
+    let onCompletedCallCount = 0, updateCount = 0;
+    expect.assertions(1);
+
+    class Component extends React.Component {
+      state = {
+        variables: {
+          first: 1,
+        },
+      };
+
+      componentDidMount() {
+        setTimeout(() => {
+          this.setState({
+            variables: {
+              first: 2,
+            },
+          });
+          setTimeout(() => {
+            this.setState({
+              variables: {
+                first: 1,
+              },
+            });
+          }, 50);
+        }, 50);
+      }
+
+      // Make sure `onCompleted` is called both when new data is being
+      // fetched over the network, and when data is coming back from
+      // the cache.
+      onCompleted() {
+        onCompletedCallCount += 1;
+      }
+
+      componentDidUpdate() {
+        updateCount += 1;
+        if (updateCount === 2) {
+          // Should be 3 since we change variables twice + initial variables.
+          expect(onCompletedCallCount).toBe(3);
+          done();
+        }
+      }
+
+      render() {
+        const { variables } = this.state;
+
+        return (
+          <AllPeopleQuery query={query} variables={variables} onCompleted={this.onCompleted}>
+            {() => null}
+          </AllPeopleQuery>
+        );
+      }
+    }
+
+    wrapper = mount(
+      <MockedProvider mocks={mocks} addTypename={false}>
+        <Component />
+      </MockedProvider>,
+    );
+  });
+
+  it('should not repeatedly call onError if setState in it', done => {
+    const mockError = [
+      {
+        request: { query: allPeopleQuery },
+        error: new Error('error occurred'),
+      },
+    ];
+
+    let onErrorCallCount = 0, updateCount = 0;
+    class Component extends React.Component {
+      state = {
+        variables: {
+          first: 1
+        }
+      }
+      onError = () => {
+        onErrorCallCount += 1;
+        this.setState({ causeUpdate: true });
+      }
+      componentDidUpdate() {
+        updateCount += 1;
+        if (updateCount === 1) {
+          // the cDU in Query is triggered by setState in onError
+          // will be called before cDU in Component
+          // onError should have been called only once in whole lifecycle
+          expect(onErrorCallCount).toBe(1);
+          done();
+        }
+      }
+      render() {
+        return (
+          <Query query={allPeopleQuery} variables={this.state.variables} onError={this.onError}>
+            {() => null}
+          </Query>
+        );
+      }
+    }
+
+    wrapper = mount(
+      <MockedProvider mocks={mockError} addTypename={false}>
+        <Component />
+      </MockedProvider>,
+    );
+  });
+
   describe('Partial refetching', () => {
     it(
       'should attempt a refetch when the query result was marked as being ' +