From a9981b621346a7052f37a9bb0783e01ee76e766f Mon Sep 17 00:00:00 2001 From: Jason Paulos <5856867+jasonpaulos@users.noreply.github.com> Date: Mon, 17 Jun 2019 11:43:10 -0700 Subject: [PATCH 1/5] Add ability for Query to detect when lastResult is not accurate --- src/Query.tsx | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/Query.tsx b/src/Query.tsx index 3e486fedce..f2a87033ed 100644 --- a/src/Query.tsx +++ b/src/Query.tsx @@ -125,6 +125,7 @@ export default class Query extends private hasMounted: boolean = false; private operation?: IDocumentDefinition; private lastResult: ApolloQueryResult | null = null; + private lastRenderIsDifferent: boolean = false; constructor(props: QueryProps, context: QueryContext) { super(props, context); @@ -232,7 +233,14 @@ export default class Query extends render(): React.ReactNode { const { context } = this; - const finish = () => this.props.children(this.getQueryResult()); + const finish = () => { + const result = this.getQueryResult(); + this.lastRenderIsDifferent = + !this.lastResult || + this.lastResult.loading !== result.loading || + this.lastResult.networkStatus !== result.networkStatus; + return this.props.children(result); + }; if (context && context.renderPromises) { return context.renderPromises.addQueryPromise(this, finish); } @@ -334,6 +342,7 @@ export default class Query extends } if ( + !this.lastRenderIsDifferent && this.lastResult && this.lastResult.loading === loading && this.lastResult.networkStatus === networkStatus && @@ -393,6 +402,8 @@ export default class Query extends // after a network based Query result has been received. this.handleErrorOrCompleted(); + this.lastRenderIsDifferent = false; + // Force a rerender that goes through shouldComponentUpdate. if (this.hasMounted) this.forceUpdate(); }; From d4ce37e57ab5f95c6db1935ef955c2b1f1436267 Mon Sep 17 00:00:00 2001 From: Jason Paulos <5856867+jasonpaulos@users.noreply.github.com> Date: Mon, 17 Jun 2019 12:00:05 -0700 Subject: [PATCH 2/5] Add regression test for #2899 --- test/client/graphql/queries/loading.test.tsx | 107 +++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/test/client/graphql/queries/loading.test.tsx b/test/client/graphql/queries/loading.test.tsx index 88d0254419..18b047f68e 100644 --- a/test/client/graphql/queries/loading.test.tsx +++ b/test/client/graphql/queries/loading.test.tsx @@ -614,4 +614,111 @@ describe('[queries] loading', () => { , ); }); + + it('correctly sets loading state on component with changed variables, unchanged result, and network-only', done => { + const query: DocumentNode = gql` + query remount($first: Int) { + allPeople(first: $first) { + people { + name + } + } + } + `; + interface Data { + allPeople: { + people: { name: string }[]; + }; + } + + const data = { allPeople: { people: [{ name: 'Luke Skywalker' }] } }; + const variables = { first: 1 }; + const variables2 = { first: 2 }; + + type Vars = typeof variables; + const link = mockSingleLink( + { request: { query, variables }, result: { data }, delay: 10 }, + { + request: { query, variables: variables2 }, + result: { data }, + delay: 10, + }, + ); + const client = new ApolloClient({ + link, + cache: new Cache({ addTypename: false }), + }); + let count = 0; + + interface Props extends Vars { + setFirst: (first: number) => void; + } + + const connect = (component: React.ComponentType): React.ComponentType<{}> => { + return class extends React.Component<{}, { first: number }> { + constructor(props: {}) { + super(props); + + this.state = { + first: 1, + }; + this.setFirst = this.setFirst.bind(this); + } + + setFirst(first: number) { + this.setState({ first }); + } + + render() { + return React.createElement(component, { + first: this.state.first, + setFirst: this.setFirst, + }); + } + }; + }; + + const Container = connect( + graphql(query, { + options: ({ first }) => ({ variables: { first }, fetchPolicy: 'network-only' }) + })( + class extends React.Component> { + componentWillReceiveProps(props: ChildProps) { + if (count === 0) { + expect(props.data!.loading).toBeTruthy(); + } + + if (count === 1) { + expect(props.data!.loading).toBeFalsy(); // has initial data + expect(props.data!.allPeople).toEqual(data.allPeople); + setTimeout(() => { + this.props.setFirst(2); + }, 5); + } + + if (count === 2) { + expect(props.data!.loading).toBeTruthy(); // on variables change + } + + if (count === 3) { + // new data after fetch + expect(props.data!.loading).toBeFalsy(); + expect(props.data!.allPeople).toEqual(data.allPeople); + done(); + } + count++; + } + render() { + return null; + } + }, + ), + ); + + renderer.create( + + + , + ); + }); }); From 5839fb9b1de451c2d4dfe86c928904dfe50302ae Mon Sep 17 00:00:00 2001 From: hwillson Date: Tue, 18 Jun 2019 21:24:38 -0400 Subject: [PATCH 3/5] Streamline interactions with Apollo Client This commit streamlines some of the React Apollo <--> Apollo Client communication points, to help reduce temporary placeholders and variables used by React Apollo to control rendering. The current data result that is to be rendered now comes from only one place, the initialized `ObservableQuery` instance. --- src/Query.tsx | 75 ++++++------------- test/client/Query.test.tsx | 28 ++++--- .../mutations/recycled-queries.test.tsx | 22 +----- .../client/graphql/queries/lifecycle.test.tsx | 12 --- 4 files changed, 44 insertions(+), 93 deletions(-) diff --git a/src/Query.tsx b/src/Query.tsx index d5bf1bffcd..33a37370d5 100644 --- a/src/Query.tsx +++ b/src/Query.tsx @@ -7,6 +7,7 @@ import ApolloClient, { NetworkStatus, FetchMoreOptions, FetchMoreQueryOptions, + ApolloCurrentResult } from 'apollo-client'; import { DocumentNode } from 'graphql'; import { ZenObservable } from 'zen-observable-ts'; @@ -115,7 +116,6 @@ export default class Query extends // only delete queryObservable when we unmount the component. private queryObservable?: ObservableQuery | null; private querySubscription?: ZenObservable.Subscription; - private previousData: any = {}; private refetcherQueue?: { args: any; resolve: (value?: any | PromiseLike) => void; @@ -124,8 +124,7 @@ export default class Query extends private hasMounted: boolean = false; private operation?: IDocumentDefinition; - private lastResult: ApolloQueryResult | null = null; - private lastRenderIsDifferent: boolean = false; + private lastRenderedResult: ApolloQueryResult | null = null; constructor(props: QueryProps, context: QueryContext) { super(props, context); @@ -176,7 +175,7 @@ export default class Query extends this.hasMounted = true; if (this.props.skip) return; - this.startQuerySubscription(true); + this.startQuerySubscription(); if (this.refetcherQueue) { const { args, resolve, reject } = this.refetcherQueue; this.queryObservable!.refetch(args) @@ -188,6 +187,7 @@ export default class Query extends componentWillReceiveProps(nextProps: QueryProps, nextContext: QueryContext) { // the next render wants to skip if (nextProps.skip && !this.props.skip) { + this.queryObservable!.resetLastResults(); this.removeQuerySubscription(); return; } @@ -202,11 +202,10 @@ export default class Query extends this.client = nextClient; this.removeQuerySubscription(); this.queryObservable = null; - this.previousData = {}; - this.updateQuery(nextProps); } if (this.props.query !== nextProps.query) { + this.queryObservable!.resetLastResults(); this.removeQuerySubscription(); } @@ -233,14 +232,7 @@ export default class Query extends render(): React.ReactNode { const { context } = this; - const finish = () => { - const result = this.getQueryResult(); - this.lastRenderIsDifferent = - !this.lastResult || - this.lastResult.loading !== result.loading || - this.lastResult.networkStatus !== result.networkStatus; - return this.props.children(result); - }; + const finish = () => this.props.children(this.getQueryResult()); if (context && context.renderPromises) { return context.renderPromises.addQueryPromise(this, finish); } @@ -308,53 +300,28 @@ export default class Query extends .catch(() => null); } - private startQuerySubscription = (justMounted: boolean = false) => { + private startQuerySubscription = () => { // When the `Query` component receives new props, or when we explicitly // re-subscribe to a query using `resubscribeToQuery`, we start a new // subscription in this method. To avoid un-necessary re-renders when // receiving new props or re-subscribing, we track the full last // observable result so it can be compared against incoming new data. // We only trigger a re-render if the incoming result is different than - // the stored `lastResult`. - // - // It's important to note that when a component is first mounted, - // the `startQuerySubscription` method is also triggered. During a first - // mount, we don't want to store or use the last result, as we always - // need the first render to happen, even if there was a previous last - // result (which can happen when the same component is mounted, unmounted, - // and mounted again). - if (!justMounted) { - this.lastResult = this.queryObservable!.getLastResult(); - } + // the stored `lastRenderedResult`. if (this.querySubscription) return; - // store the initial renders worth of result - let initial: QueryResult | undefined = this.getQueryResult(); - this.querySubscription = this.queryObservable!.subscribe({ - next: ({ loading, networkStatus, data }) => { - // to prevent a quick second render from the subscriber - // we compare to see if the original started finished (from cache) and is unchanged - if (initial && initial.networkStatus === 7 && shallowEqual(initial.data, data)) { - initial = undefined; - return; - } - + next: (result) => { if ( - !this.lastRenderIsDifferent && - this.lastResult && - this.lastResult.loading === loading && - this.lastResult.networkStatus === networkStatus && - shallowEqual(this.lastResult.data, data) + this.lastRenderedResult && + this.lastRenderedResult.loading === result.loading && + this.lastRenderedResult.networkStatus === result.networkStatus && + shallowEqual(this.lastRenderedResult.data, result.data) ) { return; } - initial = undefined; - if (this.lastResult) { - this.lastResult = this.queryObservable!.getLastResult(); - } this.updateCurrentData(); }, error: error => { @@ -368,8 +335,8 @@ export default class Query extends private removeQuerySubscription = () => { if (this.querySubscription) { - this.lastResult = this.queryObservable!.getLastResult(); this.querySubscription.unsubscribe(); + delete this.lastRenderedResult; delete this.querySubscription; } }; @@ -396,8 +363,6 @@ export default class Query extends // after a network based Query result has been received. this.handleErrorOrCompleted(); - this.lastRenderIsDifferent = false; - // Force a rerender that goes through shouldComponentUpdate. if (this.hasMounted) this.forceUpdate(); }; @@ -429,7 +394,6 @@ export default class Query extends loading: false, }; } else { - // Fetch the current result (if any) from the store. const currentResult = this.queryObservable!.currentResult(); const { loading, partial, networkStatus, errors } = currentResult; let { error } = currentResult; @@ -443,8 +407,11 @@ export default class Query extends const { fetchPolicy } = this.queryObservable!.options; Object.assign(data, { loading, networkStatus, error }); + const previousData = + this.lastRenderedResult ? this.lastRenderedResult.data : {}; + if (loading) { - Object.assign(data.data, this.previousData, currentResult.data); + Object.assign(data.data, previousData, currentResult.data); } else if (error) { Object.assign(data, { data: (this.queryObservable!.getLastResult() || {}).data, @@ -455,11 +422,12 @@ export default class Query extends ) { // Make sure data pulled in by a `no-cache` query is preserved // when the components parent tree is re-rendered. - data.data = this.previousData; + data.data = previousData; } else { const { partialRefetch } = this.props; if ( partialRefetch && + typeof currentResult.data === 'object' && Object.keys(currentResult.data).length === 0 && partial && fetchPolicy !== 'cache-only' @@ -474,11 +442,11 @@ export default class Query extends // this we'll attempt to refetch the `Query` data. Object.assign(data, { loading: true, networkStatus: NetworkStatus.loading }); data.refetch(); + this.lastRenderedResult = data; return data; } Object.assign(data.data, currentResult.data); - this.previousData = currentResult.data; } } @@ -524,6 +492,7 @@ export default class Query extends }); data.client = this.client; + this.lastRenderedResult = data; return data; }; } diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx index ddf2a376a2..fb81fd22b9 100644 --- a/test/client/Query.test.tsx +++ b/test/client/Query.test.tsx @@ -524,14 +524,18 @@ describe('Query component', () => { describe('props allow', () => { it('custom fetch-policy', done => { + let count = 0; const Component = () => ( {result => { - catchAsyncError(done, () => { - expect(result.loading).toBeFalsy(); - expect(result.networkStatus).toBe(NetworkStatus.ready); - done(); - }); + if (count === 0) { + catchAsyncError(done, () => { + expect(result.loading).toBeFalsy(); + expect(result.networkStatus).toBe(NetworkStatus.ready); + done(); + }); + } + count += 1; return null; }} @@ -545,14 +549,18 @@ describe('Query component', () => { }); it('default fetch-policy', done => { + let count = 0; const Component = () => ( {result => { - catchAsyncError(done, () => { - expect(result.loading).toBeFalsy(); - expect(result.networkStatus).toBe(NetworkStatus.ready); - done(); - }); + if (count === 0) { + catchAsyncError(done, () => { + expect(result.loading).toBeFalsy(); + expect(result.networkStatus).toBe(NetworkStatus.ready); + done(); + }); + } + count += 1; return null; }} diff --git a/test/client/graphql/mutations/recycled-queries.test.tsx b/test/client/graphql/mutations/recycled-queries.test.tsx index 5366b2412f..02924181c1 100644 --- a/test/client/graphql/mutations/recycled-queries.test.tsx +++ b/test/client/graphql/mutations/recycled-queries.test.tsx @@ -128,7 +128,7 @@ describe('graphql(mutation) update queries', () => { render() { try { - switch (queryRenderCount++) { + switch (queryRenderCount) { case 0: expect(this.props.data!.loading).toBeTruthy(); expect(this.props.data!.todo_list).toBeFalsy(); @@ -141,21 +141,6 @@ describe('graphql(mutation) update queries', () => { }); break; case 2: - expect(queryMountCount).toBe(1); - expect(queryUnmountCount).toBe(0); - expect(stripSymbols(this.props.data!.todo_list)).toEqual({ - id: '123', - title: 'how to apollo', - tasks: [ - { - id: '99', - text: 'This one was created with a mutation.', - completed: true, - }, - ], - }); - break; - case 3: expect(queryMountCount).toBe(2); expect(queryUnmountCount).toBe(1); expect(stripSymbols(this.props.data!.todo_list)).toEqual({ @@ -175,7 +160,7 @@ describe('graphql(mutation) update queries', () => { ], }); break; - case 4: + case 3: expect(stripSymbols(this.props.data!.todo_list)).toEqual({ id: '123', title: 'how to apollo', @@ -196,6 +181,7 @@ describe('graphql(mutation) update queries', () => { default: throw new Error('Rendered too many times'); } + queryRenderCount += 1; } catch (error) { reject(error); } @@ -247,7 +233,7 @@ describe('graphql(mutation) update queries', () => { expect(todoUpdateQueryCount).toBe(2); expect(queryMountCount).toBe(2); expect(queryUnmountCount).toBe(2); - expect(queryRenderCount).toBe(4); + expect(queryRenderCount).toBe(3); resolve(); } catch (error) { reject(error); diff --git a/test/client/graphql/queries/lifecycle.test.tsx b/test/client/graphql/queries/lifecycle.test.tsx index b244ce6827..eaf733acc2 100644 --- a/test/client/graphql/queries/lifecycle.test.tsx +++ b/test/client/graphql/queries/lifecycle.test.tsx @@ -581,20 +581,8 @@ describe('[queries] lifecycle', () => { { loading: false, a: 7, b: 8, c: 9 }, // Load 6 - - // The first render is caused by the component having its state updated - // when switching the client. The 2nd and 3rd renders are caused by the - // component re-subscribing to get data from Apollo Client. - { loading: false, a: 1, b: 2, c: 3 }, - { loading: false, a: 1, b: 2, c: 3 }, { loading: false, a: 1, b: 2, c: 3 }, - - { loading: false, a: 4, b: 5, c: 6 }, { loading: false, a: 4, b: 5, c: 6 }, - { loading: false, a: 4, b: 5, c: 6 }, - - { loading: false, a: 7, b: 8, c: 9 }, - { loading: false, a: 7, b: 8, c: 9 }, { loading: false, a: 7, b: 8, c: 9 }, ]); }); From 0fd0985d49fdb88153331f43bf2a23f9ff420099 Mon Sep 17 00:00:00 2001 From: hwillson Date: Fri, 21 Jun 2019 11:49:23 -0400 Subject: [PATCH 4/5] Code review tweaks --- src/Query.tsx | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/Query.tsx b/src/Query.tsx index 33a37370d5..7574b1a784 100644 --- a/src/Query.tsx +++ b/src/Query.tsx @@ -379,16 +379,16 @@ export default class Query extends } private getQueryResult = (): QueryResult => { - let data = { data: Object.create(null) as TData } as any; + let result = { data: Object.create(null) as TData } as any; // Attach bound methods - Object.assign(data, observableQueryFields(this.queryObservable!)); + Object.assign(result, observableQueryFields(this.queryObservable!)); // When skipping a query (ie. we're not querying for data but still want // to render children), make sure the `data` is cleared out and // `loading` is set to `false` (since we aren't loading anything). if (this.props.skip) { - data = { - ...data, + result = { + ...result, data: undefined, error: undefined, loading: false, @@ -405,15 +405,15 @@ export default class Query extends } const { fetchPolicy } = this.queryObservable!.options; - Object.assign(data, { loading, networkStatus, error }); + Object.assign(result, { loading, networkStatus, error }); const previousData = this.lastRenderedResult ? this.lastRenderedResult.data : {}; if (loading) { - Object.assign(data.data, previousData, currentResult.data); + Object.assign(result.data, previousData, currentResult.data); } else if (error) { - Object.assign(data, { + Object.assign(result, { data: (this.queryObservable!.getLastResult() || {}).data, }); } else if ( @@ -422,11 +422,12 @@ export default class Query extends ) { // Make sure data pulled in by a `no-cache` query is preserved // when the components parent tree is re-rendered. - data.data = previousData; + result.data = previousData; } else { const { partialRefetch } = this.props; if ( partialRefetch && + currentResult.data !== null && typeof currentResult.data === 'object' && Object.keys(currentResult.data).length === 0 && partial && @@ -440,13 +441,13 @@ export default class Query extends // the original `Query` component are expecting certain data values to // exist, and they're all of a sudden stripped away. To help avoid // this we'll attempt to refetch the `Query` data. - Object.assign(data, { loading: true, networkStatus: NetworkStatus.loading }); - data.refetch(); - this.lastRenderedResult = data; - return data; + Object.assign(result, { loading: true, networkStatus: NetworkStatus.loading }); + result.refetch(); + this.lastRenderedResult = result; + return result; } - Object.assign(data.data, currentResult.data); + Object.assign(result.data, currentResult.data); } } @@ -470,9 +471,9 @@ export default class Query extends // always hit the network with refetch, since the components data will be // updated and a network request is not currently active. if (!this.querySubscription) { - const oldRefetch = (data as QueryControls).refetch; + const oldRefetch = (result as QueryControls).refetch; - (data as QueryControls).refetch = args => { + (result as QueryControls).refetch = args => { if (this.querySubscription) { return oldRefetch(args); } else { @@ -491,8 +492,8 @@ export default class Query extends this.queryObservable!.resetQueryStoreErrors(); }); - data.client = this.client; - this.lastRenderedResult = data; - return data; + result.client = this.client; + this.lastRenderedResult = result; + return result; }; } From c6428cfc7c6704e42598e3b3cb45c9359ae0051d Mon Sep 17 00:00:00 2001 From: hwillson Date: Fri, 21 Jun 2019 11:51:43 -0400 Subject: [PATCH 5/5] Changelog update --- Changelog.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index 40309c90ff..52f57fee2f 100644 --- a/Changelog.md +++ b/Changelog.md @@ -31,9 +31,11 @@ responses couldn't be handled). The `Query` component can now handle an error in a response, then continue to handle a valid response afterwards.
[@hwillson](https://github.com/hwillson) in [#3107](https://github.com/apollographql/react-apollo/pull/3107) -- Reorder `Subscription` component code to avoid setting state on unmounted +- Reorder `Subscription` component code to avoid setting state on unmounted component.
[@jasonpaulos](https://github.com/jasonpaulos) in [#3139](https://github.com/apollographql/react-apollo/pull/3139) +- Fix component stuck in `loading` state for `network-only` fetch policy.
+ [@jasonpaulos](https://github.com/jasonpaulos) in [#3126](https://github.com/apollographql/react-apollo/pull/3126) ## 2.5.6 (2019-05-22)