From 2b7073b870dcae0bd5f54968d741fb6d6ec0d5e5 Mon Sep 17 00:00:00 2001 From: Jason Paulos Date: Fri, 21 Jun 2019 09:28:47 -0700 Subject: [PATCH] Fix component stuck in loading state for network-only fetch policy (#3126) * Add ability for Query to detect when lastResult is not accurate * Add regression test for #2899 * 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. * Code review tweaks * Changelog update --- Changelog.md | 4 +- src/Query.tsx | 91 ++++++--------- test/client/Query.test.tsx | 28 +++-- .../mutations/recycled-queries.test.tsx | 22 +--- .../client/graphql/queries/lifecycle.test.tsx | 12 -- test/client/graphql/queries/loading.test.tsx | 107 ++++++++++++++++++ 6 files changed, 168 insertions(+), 96 deletions(-) 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) diff --git a/src/Query.tsx b/src/Query.tsx index 2fd7642abb..7574b1a784 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,7 +124,7 @@ export default class Query extends private hasMounted: boolean = false; private operation?: IDocumentDefinition; - private lastResult: ApolloQueryResult | null = null; + private lastRenderedResult: ApolloQueryResult | null = null; constructor(props: QueryProps, context: QueryContext) { super(props, context); @@ -175,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) @@ -187,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; } @@ -201,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(); } @@ -300,52 +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.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 => { @@ -359,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; } }; @@ -403,22 +379,21 @@ 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, }; } else { - // Fetch the current result (if any) from the store. const currentResult = this.queryObservable!.currentResult(); const { loading, partial, networkStatus, errors } = currentResult; let { error } = currentResult; @@ -430,12 +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, this.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 ( @@ -444,11 +422,13 @@ 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; + result.data = previousData; } else { const { partialRefetch } = this.props; if ( partialRefetch && + currentResult.data !== null && + typeof currentResult.data === 'object' && Object.keys(currentResult.data).length === 0 && partial && fetchPolicy !== 'cache-only' @@ -461,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(); - return data; + Object.assign(result, { loading: true, networkStatus: NetworkStatus.loading }); + result.refetch(); + this.lastRenderedResult = result; + return result; } - Object.assign(data.data, currentResult.data); - this.previousData = currentResult.data; + Object.assign(result.data, currentResult.data); } } @@ -491,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 { @@ -512,7 +492,8 @@ export default class Query extends this.queryObservable!.resetQueryStoreErrors(); }); - data.client = this.client; - return data; + result.client = this.client; + this.lastRenderedResult = result; + return result; }; } 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 }, ]); }); 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( + + + , + ); + }); });