From f4f0ad518168ad0bfd10c12fd208aeb1f2d002cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=B3=E4=B9=99=E5=B1=B1?= Date: Tue, 22 Jan 2019 17:28:26 +0800 Subject: [PATCH 1/9] Fix repeatedly call Query.onCompleted when setState in it --- src/Query.tsx | 14 +++- test/client/Query.test.tsx | 146 +++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 2 deletions(-) diff --git a/src/Query.tsx b/src/Query.tsx index 97a88c9e3d..fc66a02da9 100644 --- a/src/Query.tsx +++ b/src/Query.tsx @@ -19,6 +19,7 @@ import { RenderPromises } from './getDataFromTree'; const shallowEqual = require('fbjs/lib/shallowEqual'); const invariant = require('invariant'); +const isEqual = require('lodash.isequal'); export type ObservableQueryFields = Pick< ObservableQuery, @@ -225,12 +226,13 @@ export default class Query extends this.hasMounted = false; } - componentDidUpdate() { + componentDidUpdate(prevProps: QueryProps) { const { onCompleted, onError } = this.props; if (onCompleted || onError) { const currentResult = this.queryObservable!.currentResult(); const { loading, error, data } = currentResult; - if (onCompleted && !loading && !error) { + const isDiffRequest = !isEqual(prevProps.query, this.props.query) || !isEqual(prevProps.variables, this.props.variables); + if (onCompleted && !loading && !error && isDiffRequest) { onCompleted(data); } else if (onError && !loading && error) { onError(error); @@ -361,6 +363,14 @@ export default class Query extends private updateCurrentData = () => { // force a rerender that goes through shouldComponentUpdate + const result = this.queryObservable!.currentResult(); + const { data, loading, error } = result; + const { onCompleted, onError } = this.props; + if (onCompleted && !loading && !error) { + onCompleted(data); + } else if (onError && !loading && error) { + onError(error); + } if (this.hasMounted) this.forceUpdate(); }; diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx index 46693f33c4..8fa74691b4 100644 --- a/test/client/Query.test.tsx +++ b/test/client/Query.test.tsx @@ -1402,6 +1402,152 @@ 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) { + // the cDU in Query is triggered by setState in onCompleted + // will be called before cDU in Component + // onCompleted should have been called only once in whole 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( + + + , + ); + }); + describe('Partial refetching', () => { it( 'should attempt a refetch when the query result was marked as being ' + From fa8429463310c427ec28dbc2fa03ab2179305f07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=B3=E4=B9=99=E5=B1=B1?= Date: Tue, 22 Jan 2019 22:17:33 +0800 Subject: [PATCH 2/9] Fix repeatedly call Query.onError when setState in it --- src/Query.tsx | 2 +- test/client/Query.test.tsx | 46 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/Query.tsx b/src/Query.tsx index fc66a02da9..09b5807d71 100644 --- a/src/Query.tsx +++ b/src/Query.tsx @@ -234,7 +234,7 @@ export default class Query extends const isDiffRequest = !isEqual(prevProps.query, this.props.query) || !isEqual(prevProps.variables, this.props.variables); if (onCompleted && !loading && !error && isDiffRequest) { onCompleted(data); - } else if (onError && !loading && error) { + } else if (onError && !loading && error && isDiffRequest) { onError(error); } } diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx index 8fa74691b4..06632b3125 100644 --- a/test/client/Query.test.tsx +++ b/test/client/Query.test.tsx @@ -1548,6 +1548,52 @@ describe('Query component', () => { ); }); + + 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 ' + From 6152de80b8d43f24854222ab1bb1c4ab1615aa04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=B3=E4=B9=99=E5=B1=B1?= Date: Tue, 22 Jan 2019 22:24:45 +0800 Subject: [PATCH 3/9] Refactor to extract handleErrorOrCompleted in Query --- src/Query.tsx | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/Query.tsx b/src/Query.tsx index 09b5807d71..d4747ff579 100644 --- a/src/Query.tsx +++ b/src/Query.tsx @@ -227,16 +227,10 @@ export default class Query extends } componentDidUpdate(prevProps: QueryProps) { - const { onCompleted, onError } = this.props; - if (onCompleted || onError) { - const currentResult = this.queryObservable!.currentResult(); - const { loading, error, data } = currentResult; - const isDiffRequest = !isEqual(prevProps.query, this.props.query) || !isEqual(prevProps.variables, this.props.variables); - if (onCompleted && !loading && !error && isDiffRequest) { - onCompleted(data); - } else if (onError && !loading && error && isDiffRequest) { - onError(error); - } + const isDiffRequest = !isEqual(prevProps.query, this.props.query) || !isEqual(prevProps.variables, this.props.variables); + if (isDiffRequest) { + // call onError / onCompleted here for local cache result + this.handleErrorOrCompleted(); } } @@ -362,7 +356,13 @@ export default class Query extends } private updateCurrentData = () => { + // call onError / onCompleted here for network result + 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; @@ -371,8 +371,7 @@ export default class Query extends } else if (onError && !loading && error) { onError(error); } - if (this.hasMounted) this.forceUpdate(); - }; + } private getQueryResult = (): QueryResult => { let data = { data: Object.create(null) as TData } as any; From 0d4c248d82c75ced392fa178f0d8744db1c066b8 Mon Sep 17 00:00:00 2001 From: Hugh Willson Date: Wed, 27 Feb 2019 21:09:34 -0500 Subject: [PATCH 4/9] Add `@types/lodash.isequal` --- package-lock.json | 15 +++++++++++++++ package.json | 1 + 2 files changed, 16 insertions(+) diff --git a/package-lock.json b/package-lock.json index b0df6fade2..9b98429a97 100644 --- a/package-lock.json +++ b/package-lock.json @@ -188,6 +188,21 @@ "integrity": "sha512-Q5hTcfdudEL2yOmluA1zaSyPbzWPmJ3XfSWeP3RyoYvS9hnje1ZyagrZOuQ6+1nQC1Gw+7gap3pLNL3xL6UBug==", "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 383768f6f3..a59dfa318b 100644 --- a/package.json +++ b/package.json @@ -100,6 +100,7 @@ "@types/hoist-non-react-statics": "^3.0.1", "@types/invariant": "2.2.29", "@types/jest": "23.3.14", + "@types/lodash.isequal": "^4.5.4", "@types/object-assign": "4.0.30", "@types/prop-types": "15.5.9", "@types/react": "16.4.18", From 3a19e2f27fa60d7794bbc6157603f1d50d959cd0 Mon Sep 17 00:00:00 2001 From: Hugh Willson Date: Wed, 27 Feb 2019 21:12:47 -0500 Subject: [PATCH 5/9] Switch to ES2015 module syntax for the `lodash.isequal` import --- src/Query.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Query.tsx b/src/Query.tsx index c8127848e3..1b6e658506 100644 --- a/src/Query.tsx +++ b/src/Query.tsx @@ -15,7 +15,7 @@ import { parser, DocumentType, IDocumentDefinition } from './parser'; import { getClient } from './component-utils'; import { RenderPromises } from './getDataFromTree'; -const isEqual = require('lodash.isequal'); +import isEqual from 'lodash.isequal'; import shallowEqual from './utils/shallowEqual'; import { invariant } from 'ts-invariant'; From 44acecb01d713718f24e1a32bd6bf426e2fb848b Mon Sep 17 00:00:00 2001 From: Hugh Willson Date: Wed, 27 Feb 2019 21:41:33 -0500 Subject: [PATCH 6/9] Code/comment formatting adjustments --- src/Query.tsx | 13 +++++++++---- test/client/Query.test.tsx | 10 +++++----- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/Query.tsx b/src/Query.tsx index 1b6e658506..0f016b87ae 100644 --- a/src/Query.tsx +++ b/src/Query.tsx @@ -232,9 +232,12 @@ export default class Query extends } componentDidUpdate(prevProps: QueryProps) { - const isDiffRequest = !isEqual(prevProps.query, this.props.query) || !isEqual(prevProps.variables, this.props.variables); + const isDiffRequest = + !isEqual(prevProps.query, this.props.query) || + !isEqual(prevProps.variables, this.props.variables); if (isDiffRequest) { - // call onError / onCompleted here for local cache result + // If specified, `onError` / `onCompleted` callbacks are called here + // after local cache results are loaded. this.handleErrorOrCompleted(); } } @@ -401,9 +404,11 @@ export default class Query extends } private updateCurrentData = () => { - // call onError / onCompleted here for network result + // 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 + + // Force a rerender that goes through shouldComponentUpdate. if (this.hasMounted) this.forceUpdate(); }; diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx index c24e3ae412..92d26ac6b5 100644 --- a/test/client/Query.test.tsx +++ b/test/client/Query.test.tsx @@ -1425,9 +1425,10 @@ describe('Query component', () => { componentDidUpdate() { updateCount += 1; if (updateCount === 1) { - // the cDU in Query is triggered by setState in onCompleted - // will be called before cDU in Component - // onCompleted should have been called only once in whole lifecycle + // `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(); } @@ -1509,7 +1510,7 @@ describe('Query component', () => { componentDidUpdate() { updateCount += 1; if (updateCount === 2) { - // should be 3 since we change variables twice + initial variables + // Should be 3 since we change variables twice + initial variables. expect(onCompletedCallCount).toBe(3); done(); } @@ -1533,7 +1534,6 @@ describe('Query component', () => { ); }); - it('should not repeatedly call onError if setState in it', done => { const mockError = [ { From 822985a4016c7d154a09cdf5b0a16dc45f0cfffb Mon Sep 17 00:00:00 2001 From: Hugh Willson Date: Wed, 27 Feb 2019 21:48:47 -0500 Subject: [PATCH 7/9] Changelog update --- Changelog.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Changelog.md b/Changelog.md index 879fb15967..ca9e268181 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,14 @@ # Change log +## 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) + + ## 2.5.1 ### Bug Fixes From 1d57bd12287dddb7a5c93fe3ed58e32aa7206398 Mon Sep 17 00:00:00 2001 From: Hugh Willson Date: Sun, 10 Mar 2019 14:26:28 -0400 Subject: [PATCH 8/9] Appease the rollup gods by marking lodash.isequal as a global --- rollup.config.js | 1 + 1 file changed, 1 insertion(+) 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) { From aa18cbd115336a06cde864da26778d4ab75da9c6 Mon Sep 17 00:00:00 2001 From: Hugh Willson Date: Sun, 10 Mar 2019 14:29:51 -0400 Subject: [PATCH 9/9] Bump the bundlesize limit --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index a4c09833fa..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": {