diff --git a/Changelog.md b/Changelog.md index a4e326a280..1826317039 100644 --- a/Changelog.md +++ b/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.
+ [@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 diff --git a/package-lock.json b/package-lock.json index 2d357908c5..142be5d8d7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -419,6 +419,21 @@ "integrity": "sha512-yALhelO3i0hqZwhjtcr6dYyaLoCHbAMshwtj6cGxTvHZAKXHsYGdff6E8EPw3xLKY0ELUTQ69Q1rQiJENnccMA==", "dev": true }, + "@types/lodash": { + "version": "4.14.121", + "resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.14.121.tgz", + "integrity": "sha512-ORj7IBWj13iYufXt/VXrCNMbUuCTJfhzme5kx9U/UtcIPdJYuvPDUAlHlbNhz/8lKCLy9XGIZnGrqXOtQbPGoQ==", + "dev": true + }, + "@types/lodash.isequal": { + "version": "4.5.4", + "resolved": "https://registry.npmjs.org/@types/lodash.isequal/-/lodash.isequal-4.5.4.tgz", + "integrity": "sha512-yr4cwbZvWysLrj3R1A9phPPzZSkV16q5/6Uom3ZDglW4ZZJJ1YdUvC9FQovp8e7kniomGkhDjjrn/0apHipO4w==", + "dev": true, + "requires": { + "@types/lodash": "*" + } + }, "@types/node": { "version": "11.9.5", "resolved": "https://registry.npmjs.org/@types/node/-/node-11.9.5.tgz", diff --git a/package.json b/package.json index aa45a778b6..2d94d2a236 100644 --- a/package.json +++ b/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", diff --git a/rollup.config.js b/rollup.config.js index 77cf76dfc8..716ae9b7ac 100644 --- a/rollup.config.js +++ b/rollup.config.js @@ -20,6 +20,7 @@ const globals = { 'react': 'react', 'ts-invariant': 'invariant', 'tslib': 'tslib', + 'lodash.isequal': 'isEqual', }; function external(id) { diff --git a/src/Query.tsx b/src/Query.tsx index 0af9e3cec5..367219919b 100644 --- a/src/Query.tsx +++ b/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 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) { + 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 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 => { let data = { data: Object.create(null) as TData } as any; // Attach bound methods diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx index 34fbc26edd..92d26ac6b5 100644 --- a/test/client/Query.test.tsx +++ b/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 ( + + {() => null} + + ); + } + } + + wrapper = mount( + + + , + ); + }); + + 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 ( + + {() => null} + + ); + } + } + + wrapper = mount( + + + , + ); + }); + + 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 ( + + {() => null} + + ); + } + } + + wrapper = mount( + + + , + ); + }); + describe('Partial refetching', () => { it( 'should attempt a refetch when the query result was marked as being ' +