From 9ef66f5f5d92d0e8494d5b8ac279b3058f30e69b Mon Sep 17 00:00:00 2001 From: hwillson Date: Fri, 30 Aug 2019 08:15:29 -0400 Subject: [PATCH] Fix problematic `fetchMore` `notifyOnNetworkStatusChange` renders Prior to this commit, when `notifyOnNetworkStatusChange` is true calls to `fetchMore` do not wait for `updateQuery` to finish, before triggering a re-render. This means that after a `fetchMore` call, components will flip to `loading: true`, then `loading: false`, then un-necessarily re-render once more as `loading: false` with the result from `updateQuery`. This commit adds a check to see if `fetchMore` has been called and then holds off on re-rendering the `loading: false` state, until `updateQuery` has fully finished. Fixes #3333 --- Changelog.md | 4 +- package.json | 2 +- .../hoc/src/__tests__/queries/skip.test.tsx | 32 +-- .../hooks/src/__tests__/useQuery.test.tsx | 205 +++++++++++++++++- packages/hooks/src/data/QueryData.ts | 33 ++- 5 files changed, 250 insertions(+), 26 deletions(-) diff --git a/Changelog.md b/Changelog.md index 167cba41ad..55ed0425ee 100644 --- a/Changelog.md +++ b/Changelog.md @@ -10,6 +10,8 @@ [@n1ru4l](https://github.com/n1ru4l) in [#3356](https://github.com/apollographql/react-apollo/pull/3356) - Makes sure `refetch`, `fetchMore`, `updateQuery`, `startPolling`, `stopPolling`, and `subscribeToMore` maintain a stable identity when they're passed back alongside query results.
[@hwillson](https://github.com/hwillson) in [#3422](https://github.com/apollographql/react-apollo/pull/3422) +- Fixed problematic re-renders that were caused by using `fetchMore.updateQuery` with `notifyOnNetworkStatusChange` set to true. When `notifyOnNetworkStatusChange` is true, re-renders will now wait until `updateQuery` has completed, to make sure the updated data is used during the render.
+ [@hwillson](https://github.com/hwillson) in [#3433](https://github.com/apollographql/react-apollo/pull/3433) - Documentation fixes.
[@SeanRoberts](https://github.com/SeanRoberts) in [#3380](https://github.com/apollographql/react-apollo/pull/3380) @@ -74,6 +76,7 @@ Consult the [Hooks migration guide](https://www.apollographql.com/docs/react/hoo ```js import * as compose from 'lodash.flowright'; ``` + - Render prop components (`Query`, `Mutation` and `Subscription`) can no longer be extended. In other words, this is no longer possible: ```js @@ -94,7 +97,6 @@ Consult the [Hooks migration guide](https://www.apollographql.com/docs/react/hoo ); ``` - ## 2.5.7 (2019-06-21) ### Improvements diff --git a/package.json b/package.json index 81bc28c6fa..3ff1be5f41 100644 --- a/package.json +++ b/package.json @@ -92,7 +92,7 @@ { "name": "@apollo/react-hooks", "path": "./packages/hooks/lib/react-hooks.cjs.min.js", - "maxSize": "3.98 kB" + "maxSize": "4 kB" }, { "name": "@apollo/react-ssr", diff --git a/packages/hoc/src/__tests__/queries/skip.test.tsx b/packages/hoc/src/__tests__/queries/skip.test.tsx index 58442e3e67..76dbed3ce2 100644 --- a/packages/hoc/src/__tests__/queries/skip.test.tsx +++ b/packages/hoc/src/__tests__/queries/skip.test.tsx @@ -1,5 +1,5 @@ import React from 'react'; -import { render, cleanup } from '@testing-library/react'; +import { render, cleanup, wait } from '@testing-library/react'; import gql from 'graphql-tag'; import ApolloClient from 'apollo-client'; import { ApolloLink } from 'apollo-link'; @@ -555,7 +555,7 @@ describe('[queries] skip', () => { ); }); - it('allows you to skip then unskip a query with opts syntax', done => { + it('allows you to skip then unskip a query with opts syntax', async () => { const query: DocumentNode = gql` query people { allPeople(first: 1) { @@ -627,7 +627,6 @@ describe('[queries] skip', () => { break; case 4: expect(this.props.data!.loading).toBeFalsy(); - done(); break; default: } @@ -654,9 +653,13 @@ describe('[queries] skip', () => { ); + + await wait(() => { + expect(count).toEqual(5); + }); }); - it('removes the injected props if skip becomes true', done => { + it('removes the injected props if skip becomes true', async () => { let count = 0; const query: DocumentNode = gql` query people($first: Int) { @@ -697,17 +700,12 @@ describe('[queries] skip', () => { class extends React.Component> { componentDidUpdate() { const { data } = this.props; - try { - // loading is true, but data still there - if (count === 0) - expect(stripSymbols(data!.allPeople)).toEqual(data1.allPeople); - if (count === 1) expect(data).toBeUndefined(); - if (count === 2 && !data!.loading) { - expect(stripSymbols(data!.allPeople)).toEqual(data3.allPeople); - done(); - } - } catch (error) { - done.fail(error); + // loading is true, but data still there + if (count === 0) + expect(stripSymbols(data!.allPeople)).toEqual(data1.allPeople); + if (count === 1) expect(data).toBeUndefined(); + if (count === 2 && !data!.loading) { + expect(stripSymbols(data!.allPeople)).toEqual(data3.allPeople); } } render() { @@ -740,6 +738,10 @@ describe('[queries] skip', () => { ); + + await wait(() => { + expect(count).toEqual(2); + }); }); it('allows you to unmount a skipped query', done => { diff --git a/packages/hooks/src/__tests__/useQuery.test.tsx b/packages/hooks/src/__tests__/useQuery.test.tsx index 4c23e9e1dd..7f43893619 100644 --- a/packages/hooks/src/__tests__/useQuery.test.tsx +++ b/packages/hooks/src/__tests__/useQuery.test.tsx @@ -346,8 +346,6 @@ describe('useQuery Hook', () => { } ); - console.log('ns', networkStatus); - switch (renderCount) { case 0: expect(loading).toBeTruthy(); @@ -437,4 +435,207 @@ describe('useQuery Hook', () => { ); }); }); + + describe('Pagination', () => { + it( + 'should render `fetchMore.updateQuery` updated results with proper ' + + 'loading status, when `notifyOnNetworkStatusChange` is true', + async () => { + const carQuery: DocumentNode = gql` + query cars($limit: Int) { + cars(limit: $limit) { + id + make + model + vin + __typename + } + } + `; + + const carResults = { + cars: [ + { + id: 1, + make: 'Audi', + model: 'RS8', + vin: 'DOLLADOLLABILL', + __typename: 'Car' + } + ] + }; + + const moreCarResults = { + cars: [ + { + id: 2, + make: 'Audi', + model: 'eTron', + vin: 'TREESRGOOD', + __typename: 'Car' + } + ] + }; + + const mocks = [ + { + request: { query: carQuery, variables: { limit: 1 } }, + result: { data: carResults } + }, + { + request: { query: carQuery, variables: { limit: 1 } }, + result: { data: moreCarResults } + } + ]; + + let renderCount = 0; + function App() { + const { loading, data, fetchMore } = useQuery(carQuery, { + variables: { limit: 1 }, + notifyOnNetworkStatusChange: true + }); + + switch (renderCount) { + case 0: + expect(loading).toBeTruthy(); + break; + case 1: + expect(loading).toBeFalsy(); + expect(data).toEqual(carResults); + fetchMore({ + variables: { + limit: 1 + }, + updateQuery: (prev, { fetchMoreResult }) => ({ + cars: [...prev.cars, ...fetchMoreResult.cars] + }) + }); + break; + case 2: + expect(loading).toBeTruthy(); + break; + case 3: + expect(loading).toBeFalsy(); + expect(data).toEqual({ + cars: [carResults.cars[0], moreCarResults.cars[0]] + }); + break; + default: + } + + renderCount += 1; + return null; + } + + render( + + + + ); + + await wait(() => { + expect(renderCount).toBe(4); + }); + } + ); + + it( + 'should render `fetchMore.updateQuery` updated results with no ' + + 'loading status, when `notifyOnNetworkStatusChange` is false', + async () => { + const carQuery: DocumentNode = gql` + query cars($limit: Int) { + cars(limit: $limit) { + id + make + model + vin + __typename + } + } + `; + + const carResults = { + cars: [ + { + id: 1, + make: 'Audi', + model: 'RS8', + vin: 'DOLLADOLLABILL', + __typename: 'Car' + } + ] + }; + + const moreCarResults = { + cars: [ + { + id: 2, + make: 'Audi', + model: 'eTron', + vin: 'TREESRGOOD', + __typename: 'Car' + } + ] + }; + + const mocks = [ + { + request: { query: carQuery, variables: { limit: 1 } }, + result: { data: carResults } + }, + { + request: { query: carQuery, variables: { limit: 1 } }, + result: { data: moreCarResults } + } + ]; + + let renderCount = 0; + function App() { + const { loading, data, fetchMore } = useQuery(carQuery, { + variables: { limit: 1 }, + notifyOnNetworkStatusChange: false + }); + + switch (renderCount) { + case 0: + expect(loading).toBeTruthy(); + break; + case 1: + expect(loading).toBeFalsy(); + expect(data).toEqual(carResults); + fetchMore({ + variables: { + limit: 1 + }, + updateQuery: (prev, { fetchMoreResult }) => ({ + cars: [...prev.cars, ...fetchMoreResult.cars] + }) + }); + break; + case 2: + expect(loading).toBeFalsy(); + expect(data).toEqual({ + cars: [carResults.cars[0], moreCarResults.cars[0]] + }); + break; + default: + } + + renderCount += 1; + return null; + } + + render( + + + + ); + + await wait(() => { + expect(renderCount).toBe(3); + }); + } + ); + }); }); diff --git a/packages/hooks/src/data/QueryData.ts b/packages/hooks/src/data/QueryData.ts index ad79d4f214..bffc33f3cf 100644 --- a/packages/hooks/src/data/QueryData.ts +++ b/packages/hooks/src/data/QueryData.ts @@ -261,13 +261,32 @@ export class QueryData extends OperationData { const obsQuery = this.currentObservable.query!; this.currentObservable.subscription = obsQuery.subscribe({ next: ({ loading, networkStatus, data }) => { - if ( - this.previousData.result && - this.previousData.result.loading === loading && - this.previousData.result.networkStatus === networkStatus && - isEqual(this.previousData.result.data, data) - ) { - return; + const previousResult = this.previousData.result; + + if (previousResult) { + // Calls to `ObservableQuery.fetchMore` return a result before the + // `updateQuery` function fully finishes. This can lead to an + // extra un-necessary re-render since the initially returned data is + // the same as data that has already been rendered. We'll + // prevent the un-necessary render from happening, making sure + // `fetchMore` results are only rendered when `updateQuery` has + // completed. + if ( + previousResult.loading && + previousResult.networkStatus === NetworkStatus.fetchMore && + isEqual(previousResult.data, data) + ) { + return; + } + + // Make sure we're not attempting to re-render similar results + if ( + previousResult.loading === loading && + previousResult.networkStatus === networkStatus && + isEqual(previousResult.data, data) + ) { + return; + } } this.forceUpdate();