From f2783bd37f602052ac9b68951884e7c9ab7c3907 Mon Sep 17 00:00:00 2001 From: hwillson Date: Fri, 14 Feb 2020 14:37:22 -0500 Subject: [PATCH] Prevent @client @export from firing unnecessary network requests PR https://github.com/apollographql/apollo-client/pull/4604 fixed an issue where changes made to `@client @export` based variables didn't always result in re-running the query that was using the variable. Unfortunately, these changes are a bit too aggressive. They're triggering a `refetch` when an `@client @export` based variable changes (and a few additional conditions are met), and while using the `ObservableQuery.refetch()` method does fix the original issue, it leads to a situation where the query being re-run is always fired over the network, even if the updated query could have been resolved from the cache. `ObservableQuery.refetch()` forces queries to use a network based fetch policy, which means if the query was originally fired with a `cache-first` policy (for example), and has matching data in the cache after taking into consideration the new variable, that data won't be used and instead an extra unnecessary network request will fire. This commit addresses the issue by leveraging `ObservableQuery.setVariables()` instead of automatically refetching. `setVariables` will attempt to resolve the updated query from the cache first, then only fire it over the network if required. --- package.json | 2 +- src/__tests__/local-state/export.ts | 79 +++++++++++++++++++++++++++++ src/core/ObservableQuery.ts | 23 ++++++--- 3 files changed, 96 insertions(+), 8 deletions(-) diff --git a/package.json b/package.json index 591e8db8d59..e8d37fa692b 100644 --- a/package.json +++ b/package.json @@ -48,7 +48,7 @@ { "name": "apollo-client", "path": "./dist/apollo-client.cjs.min.js", - "maxSize": "24.1 kB" + "maxSize": "24.2 kB" } ], "peerDependencies": { diff --git a/src/__tests__/local-state/export.ts b/src/__tests__/local-state/export.ts index a9551dcbfa8..a771e8d424d 100644 --- a/src/__tests__/local-state/export.ts +++ b/src/__tests__/local-state/export.ts @@ -825,4 +825,83 @@ describe('@client @export tests', () => { }); } ); + + it( + 'should NOT attempt to refetch over the network if an @export variable ' + + 'has changed, the current fetch policy is cache-first, and the remote ' + + 'part of the query (that leverages the @export variable) can be fully ' + + 'found in the cache.', + done => { + const query = gql` + query currentAuthorPostCount($authorId: Int!) { + currentAuthorId @client @export(as: "authorId") + postCount(authorId: $authorId) + } + `; + + const testAuthorId1 = 1; + const testPostCount1 = 100; + + const testAuthorId2 = 2; + const testPostCount2 = 200; + + let fetchCount = 0; + const link = new ApolloLink(() => { + fetchCount += 1; + return Observable.of({ + data: { + postCount: testPostCount1 + }, + }); + }); + + const cache = new InMemoryCache(); + const client = new ApolloClient({ + cache, + link, + resolvers: {}, + }); + + client.writeQuery({ + query: gql`{ currentAuthorId }`, + data: { currentAuthorId: testAuthorId1 } + }); + + let resultCount = 0; + const obs = client.watchQuery({ query, fetchPolicy: 'cache-first' }); + obs.subscribe({ + next(result) { + if (resultCount === 0) { + // The initial result is fetched over the network. + expect(fetchCount).toBe(1); + expect(result.data).toMatchObject({ + currentAuthorId: testAuthorId1, + postCount: testPostCount1, + }); + + client.writeQuery({ + query, + variables: { authorId: testAuthorId2 }, + data: { postCount: testPostCount2 } + }); + client.writeQuery({ + query: gql`{ currentAuthorId }`, + data: { currentAuthorId: testAuthorId2 } + }); + } else if (resultCount === 1) { + // The updated result should not have been fetched over the + // network, as it can be found in the cache. + expect(fetchCount).toBe(1); + expect(result.data).toMatchObject({ + currentAuthorId: testAuthorId2, + postCount: testPostCount2, + }); + done(); + } + + resultCount += 1; + }, + }); + } + ); }); diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 51e39d5388a..00a1e82069c 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -590,34 +590,43 @@ export class ObservableQuery< iterateObserversSafely(this.observers, 'error', this.lastError = error); }; + const { + hasClientExports, + serverQuery + } = queryManager.transform(this.options.query); + queryManager.observeQuery(queryId, this.options, { next: (result: ApolloQueryResult) => { if (this.lastError || this.isDifferentFromLastResult(result)) { const previousResult = this.updateLastResult(result); + const { query, variables, fetchPolicy } = this.options; // Before calling `next` on each observer, we need to first see if // the query is using `@client @export` directives, and update // any variables that might have changed. If `@export` variables have - // changed, and the query is calling against both local and remote - // data, a refetch is needed to pull in new data, using the - // updated `@export` variables. - if (queryManager.transform(query).hasClientExports) { + // changed, and the query is requesting both local and remote + // data, `setVariables` is used as a network refetch might be + // needed to pull in new data, using the updated `@export` variables. + if (hasClientExports) { queryManager.getLocalState().addExportedVariables( query, variables, ).then((variables: TVariables) => { const previousVariables = this.variables; - this.variables = this.options.variables = variables; if ( !result.loading && previousResult && fetchPolicy !== 'cache-only' && - queryManager.transform(query).serverQuery && + serverQuery && !equal(previousVariables, variables) ) { - this.refetch(); + this.setVariables(variables).then(updatedResult => { + this.variables = this.options.variables = variables; + iterateObserversSafely(this.observers, 'next', updatedResult) + }); } else { + this.variables = this.options.variables = variables; iterateObserversSafely(this.observers, 'next', result); } });