From f124728ed908b5a2e01c70cf29960b31ac51e988 Mon Sep 17 00:00:00 2001 From: Daniel Merz Date: Wed, 9 Jan 2019 06:51:25 +0100 Subject: [PATCH 1/5] Add test for Query bug which is caused by changing props (#1749) --- test/client/Query.test.tsx | 69 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx index 46693f33c4..b82969e12b 100644 --- a/test/client/Query.test.tsx +++ b/test/client/Query.test.tsx @@ -1402,6 +1402,75 @@ describe('Query component', () => { ); }); + it('should reset error state after props change variables to accepted ones', done => { + const query: DocumentNode = gql` + query somethingelse ($variable: Boolean) { + allPeople(first: 1, yetisArePeople: $variable) { + people { + name + } + } + }`; + + const data = {allPeople: {people: [{name: 'Luke Skywalker'}]}}; + const variableGood = {variable: true} + const variableBad = {variable: false} + const link = mockSingleLink( + {request: {query, variables: variableGood}, result: {data}}, + {request: {query, variables: variableBad}, result: {errors: [new Error('This is an error!')]}}, + {request: {query, variables: variableGood}, result: {data}}, + ); + const client = new ApolloClient({ + link, + cache: new Cache({addTypename: false}), + }); + + let count = 0 + // @ts-ignore + const DummyComp = (props) => { + if (!props.loading) { + try { + switch (count++) { + case 0: + expect(props.data.allPeople).toBeTruthy() + expect(props.error).toBeFalsy() + // set graphql-variables for query to a non accepted set + // @ts-ignore + setTimeout(() => wrapper.setProps({variables: variableBad}), 0) + break + case 1: + // @ts-ignore + expect(props.error).toBeTruthy() + // Changed variables => different props to query => no data should be passed in as prop + expect(props.data.allPeople).toBeFalsy() + // set graphql-variables for query back to an accepted set + // @ts-ignore + setTimeout(() => wrapper.setProps({variables: variableGood}), 0) + break + case 2: + // good variables are passed in again, data should be retrieved + expect(props.data.allPeople).toBeTruthy() + expect(props.error).toBeFalsy() + done() + break + default: + done.fail('Unknown count') + } + } catch(e) {done.fail(e)} + } + return
+ } + + wrapper = mount( + // @ts-ignore + + {(result: any) => { + return + }} + + ) + }); + describe('Partial refetching', () => { it( 'should attempt a refetch when the query result was marked as being ' + From 6b346d661afd8e70665bbf49044217ca18d21d7c Mon Sep 17 00:00:00 2001 From: Daniel Merz Date: Wed, 9 Jan 2019 07:01:43 +0100 Subject: [PATCH 2/5] Skip the failing test (#1749) --- test/client/Query.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx index b82969e12b..98eed35f2a 100644 --- a/test/client/Query.test.tsx +++ b/test/client/Query.test.tsx @@ -1402,7 +1402,7 @@ describe('Query component', () => { ); }); - it('should reset error state after props change variables to accepted ones', done => { + it.skip('should reset error state after props change variables to accepted ones', done => { const query: DocumentNode = gql` query somethingelse ($variable: Boolean) { allPeople(first: 1, yetisArePeople: $variable) { From cfe8a8439c43cc597d0b640826c5469d50ae568f Mon Sep 17 00:00:00 2001 From: Hugh Willson Date: Mon, 11 Mar 2019 10:28:58 -0400 Subject: [PATCH 3/5] Enable failing test --- test/client/Query.test.tsx | 156 ++++++++++++++++++++++--------------- 1 file changed, 94 insertions(+), 62 deletions(-) diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx index 1f066a6867..9e39a5e7e0 100644 --- a/test/client/Query.test.tsx +++ b/test/client/Query.test.tsx @@ -1387,74 +1387,106 @@ describe('Query component', () => { ); }); - it.skip('should reset error state after props change variables to accepted ones', done => { - const query: DocumentNode = gql` - query somethingelse ($variable: Boolean) { - allPeople(first: 1, yetisArePeople: $variable) { - people { - name + it( + 'should not persist previous result errors when a subsequent valid ' + + 'result is received', + done => { + const query: DocumentNode = gql` + query somethingelse ($variable: Boolean) { + allPeople(first: 1, yetisArePeople: $variable) { + people { + name + } } - } - }`; + }`; - const data = {allPeople: {people: [{name: 'Luke Skywalker'}]}}; - const variableGood = {variable: true} - const variableBad = {variable: false} - const link = mockSingleLink( - {request: {query, variables: variableGood}, result: {data}}, - {request: {query, variables: variableBad}, result: {errors: [new Error('This is an error!')]}}, - {request: {query, variables: variableGood}, result: {data}}, - ); - const client = new ApolloClient({ - link, - cache: new Cache({addTypename: false}), - }); + const data = { allPeople: { people: [{ name: 'Luke Skywalker' }] } }; + const variableGood = { variable: true } + const variableBad = { variable: false } + + const link = mockSingleLink( + { + request: { + query, + variables: variableGood, + }, + result: { + data, + }, + }, + { + request: { + query, + variables: variableBad, + }, + result: { + errors: [new Error('This is an error!')], + }, + }, + { + request: { + query, + variables: variableGood, + }, + result: { + data, + }, + }, + ); + + const client = new ApolloClient({ + link, + cache: new Cache({ addTypename: false }), + }); - let count = 0 - // @ts-ignore - const DummyComp = (props) => { - if (!props.loading) { - try { - switch (count++) { - case 0: - expect(props.data.allPeople).toBeTruthy() - expect(props.error).toBeFalsy() - // set graphql-variables for query to a non accepted set - // @ts-ignore - setTimeout(() => wrapper.setProps({variables: variableBad}), 0) - break - case 1: - // @ts-ignore - expect(props.error).toBeTruthy() - // Changed variables => different props to query => no data should be passed in as prop - expect(props.data.allPeople).toBeFalsy() - // set graphql-variables for query back to an accepted set - // @ts-ignore - setTimeout(() => wrapper.setProps({variables: variableGood}), 0) - break - case 2: - // good variables are passed in again, data should be retrieved - expect(props.data.allPeople).toBeTruthy() - expect(props.error).toBeFalsy() - done() - break - default: - done.fail('Unknown count') + let count = 0; + const DummyComp = (props: any) => { + if (!props.loading) { + try { + switch (count++) { + case 0: + expect(props.data.allPeople).toBeTruthy(); + expect(props.error).toBeFalsy(); + // Change query variables to trigger bad result. + setTimeout(() => { + wrapper!.setProps({ variables: variableBad }); + }, 0); + break; + case 1: + // Error should be received, but last known good value + // should still be accessible (in-case the UI needs it). + expect(props.error).toBeTruthy(); + expect(props.data.allPeople).toBeTruthy(); + // Change query variables to trigger a good result. + setTimeout(() => { + wrapper!.setProps({ variables: variableGood }); + }, 0); + break + case 2: + // Good result should be received without any errors. + expect(props.error).toBeFalsy(); + expect(props.data.allPeople).toBeTruthy(); + done(); + break; + default: + done.fail('Unknown count'); + } + } catch (error) { + done.fail(error); } - } catch(e) {done.fail(e)} + } + return null; } - return
- } - wrapper = mount( - // @ts-ignore - - {(result: any) => { - return - }} - - ) - }); + wrapper = mount( + + {(result: any) => { + return ; + }} + + ); + } + ); it('should not repeatedly call onCompleted if setState in it', done => { const query = gql` From 6c759c9cae9b8f60be9cbc8ca242fd9831d59ef8 Mon Sep 17 00:00:00 2001 From: Hugh Willson Date: Thu, 14 Mar 2019 21:40:59 -0400 Subject: [PATCH 4/5] Leverage lastResult to limit unnecessary subscription re-creation When the Observable subscription created in `startQuerySubcription` receives an error, we only want to remove the old subscription, and start a new subscription, when we don't have a previous result stored. So if no previous result was received due to problems fetching data, or the previous result has been forcefully cleared out for some reason, we shoule re-create a new subscription. Otherwise we might be unnecessarily starting a new subscription the `Query` component isn't expecting, and isn't tying into its update cycle properly. --- src/Query.tsx | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/Query.tsx b/src/Query.tsx index 367219919b..42280f7948 100644 --- a/src/Query.tsx +++ b/src/Query.tsx @@ -345,10 +345,15 @@ export default class Query extends this.updateCurrentData(); }, error: error => { - this.resubscribeToQuery(); - // Quick fix for https://github.com/apollostack/react-apollo/issues/378 + if (!this.lastResult) { + // We only want to remove the old subscription, and start a new + // subscription, when an error was received and we don't have a + // previous result stored. This means either no previous result was + // received due to problems fetching data, or the previous result + // has been forcefully cleared out. + this.resubscribeToQuery(); + } if (!error.hasOwnProperty('graphQLErrors')) throw error; - this.updateCurrentData(); }, }); @@ -365,13 +370,15 @@ export default class Query extends private resubscribeToQuery() { this.removeQuerySubscription(); + // Unfortunately, if `lastError` is set in the current + // `queryObservable` when the subscription is re-created, + // the subscription will immediately receive the error, which will + // cause it to terminate again. To avoid this, we first clear + // the last error/result from the `queryObservable` before re-starting + // the subscription, and restore it afterwards (so the subscription + // has a chance to stay open). const lastError = this.queryObservable!.getLastError(); - const lastResult = this.lastResult; - - // If lastError is set, the observable will immediately - // send it, causing the stream to terminate on initialization. - // We clear everything here and restore it afterward to - // make sure the new subscription sticks. + const lastResult = this.queryObservable!.getLastResult(); this.queryObservable!.resetLastResults(); this.startQuerySubscription(); Object.assign(this.queryObservable!, { lastError, lastResult }); From 7d90d92a275654758e6f056b0bb691c2489d1673 Mon Sep 17 00:00:00 2001 From: Hugh Willson Date: Thu, 14 Mar 2019 22:14:59 -0400 Subject: [PATCH 5/5] Changelog update --- Changelog.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Changelog.md b/Changelog.md index 1826317039..eb5299387f 100644 --- a/Changelog.md +++ b/Changelog.md @@ -7,6 +7,10 @@ - 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) +- Fixed an issue that prevented good results from showing up in a `Query` + component, after an error was received, variables were adjusted, and then + the good data was fetched.
+ [@MerzDaniel](https://github.com/MerzDaniel) in [#2718](https://github.com/apollographql/react-apollo/pull/2718) ### Improvements