From c0ec90d9f8be521e1c1cb678002ecc209381da4e Mon Sep 17 00:00:00 2001 From: hwillson Date: Fri, 31 Jul 2020 09:30:27 -0400 Subject: [PATCH] Avoid making network requests when skip is true When skip is set to true but other updated options have been passed into `useQuery` (like updated variables), `useQuery` will still make a network request, even though it will skip the handling of the response. This commit makes sure `useQuery` doesn't make unnecessary `skip` network requests. Fixes #6670 Fixes #6190 Fixes #6572 --- CHANGELOG.md | 7 +++ src/react/data/QueryData.ts | 9 +++ src/react/hoc/__tests__/queries/skip.test.tsx | 43 ++++++++++--- src/react/hooks/__tests__/useQuery.test.tsx | 63 +++++++++++++++++++ 4 files changed, 113 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e050508cc8..9a1636da2b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## Apollo Client 3.1.2 (not yet released) + +## Bug Fixes + +- Avoid making network requests when `skip` is `true`.
+ [@hwillson](https://github.com/hwillson) in [#6752](https://github.com/apollographql/apollo-client/pull/6752) + ## Apollo Client 3.1.1 ## Bug Fixes diff --git a/src/react/data/QueryData.ts b/src/react/data/QueryData.ts index ce3e45f20a2..90a24363f1c 100644 --- a/src/react/data/QueryData.ts +++ b/src/react/data/QueryData.ts @@ -227,6 +227,8 @@ export class QueryData extends OperationData { } private updateObservableQuery() { + if (this.getOptions().skip) return; + // If we skipped initially, we may not have yet created the observable if (!this.currentObservable) { this.initializeObservableQuery(); @@ -326,6 +328,13 @@ export class QueryData extends OperationData { // 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). + // + // NOTE: We no longer think this is the correct behavior. Skipping should + // not automatically set `data` to `undefined`, but instead leave the + // previous data in place. In other words, skipping should not mandate + // that previously received data is all of a sudden removed. Unfortunately, + // changing this is breaking, so we'll have to wait until Apollo Client + // 4.0 to address this. if (options.skip) { result = { ...result, diff --git a/src/react/hoc/__tests__/queries/skip.test.tsx b/src/react/hoc/__tests__/queries/skip.test.tsx index 9f254236e74..24285bbaf54 100644 --- a/src/react/hoc/__tests__/queries/skip.test.tsx +++ b/src/react/hoc/__tests__/queries/skip.test.tsx @@ -596,11 +596,16 @@ describe('[queries] skip', () => { ranQuery++; return f ? f(o) : null; }).concat( - mockSingleLink({ - request: { query }, - result: { data }, - newData: () => ({ data: nextData }) - }) + mockSingleLink( + { + request: { query }, + result: { data }, + }, + { + request: { query }, + result: { data: nextData }, + }, + ) ); const client = new ApolloClient({ @@ -626,13 +631,19 @@ describe('[queries] skip', () => { expect(ranQuery).toBe(1); break; case 2: + // The first batch of data is fetched over the network, and + // verified here, followed by telling the component we want to + // skip running subsequent queries. expect(this.props.data.loading).toBe(false); + expect(this.props.data.allPeople).toEqual(data.allPeople); expect(ranQuery).toBe(1); setTimeout(() => { this.props.setSkip(true); }); break; case 3: + // This render is triggered after setting skip to true. Now + // let's set skip to false to re-trigger the query. expect(this.props.data).toBeUndefined(); expect(ranQuery).toBe(1); setTimeout(() => { @@ -640,12 +651,26 @@ describe('[queries] skip', () => { }); break; case 4: - expect(this.props.data!.loading).toBe(true); - expect(ranQuery).toBe(3); + // Since the `nextFetchPolicy` was set to `cache-first`, our + // query isn't loading as it's able to find the result of the + // query directly from the cache. Let's trigger a refetch + // to manually load the next batch of data. + expect(this.props.data!.loading).toBe(false); + expect(this.props.data.allPeople).toEqual(data.allPeople); + expect(ranQuery).toBe(1); + setTimeout(() => { + this.props.data.refetch(); + }); break; case 5: + expect(this.props.data!.loading).toBe(true); + expect(ranQuery).toBe(2); + break; + case 6: + // The next batch of data has loaded. expect(this.props.data!.loading).toBe(false); - expect(ranQuery).toBe(3); + expect(this.props.data.allPeople).toEqual(nextData.allPeople); + expect(ranQuery).toBe(2); break; default: } @@ -673,7 +698,7 @@ describe('[queries] skip', () => { ); await wait(() => { - expect(count).toEqual(5); + expect(count).toEqual(6); }); }); diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index ce71b578232..462bd4a6e93 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -2162,5 +2162,68 @@ describe('useQuery Hook', () => { expect(renderCount).toBe(3); }).then(resolve, reject); }); + + itAsync('should not make network requests when `skip` is `true`', (resolve, reject) => { + let networkRequestCount = 0; + const link = new ApolloLink((o, f) => { + networkRequestCount += 1; + return f ? f(o) : null; + }).concat(mockSingleLink( + { + request: { + query: CAR_QUERY, + variables: { someVar: true } + }, + result: { data: CAR_RESULT_DATA } + } + )); + + const client = new ApolloClient({ + link, + cache: new InMemoryCache() + }); + + let renderCount = 0; + const Component = () => { + const [skip, setSkip] = useState(false); + const { loading, data } = useQuery(CAR_QUERY, { + fetchPolicy: 'no-cache', + skip, + variables: { someVar: !skip } + }); + + switch (++renderCount) { + case 1: + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + break; + case 2: + expect(loading).toBeFalsy(); + expect(data).toEqual(CAR_RESULT_DATA); + expect(networkRequestCount).toBe(1); + setTimeout(() => setSkip(true)); + break; + case 3: + expect(loading).toBeFalsy(); + expect(data).toBeUndefined(); + expect(networkRequestCount).toBe(1); + break; + default: + reject('too many renders'); + } + + return null; + }; + + render( + + + + ); + + return wait(() => { + expect(renderCount).toBe(3); + }).then(resolve, reject); + }); }); });