Skip to content

Commit

Permalink
Prevent @client @export from firing unnecessary network requests
Browse files Browse the repository at this point in the history
PR #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.
  • Loading branch information
hwillson committed Feb 14, 2020
1 parent 297909d commit f2783bd
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 8 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
{
"name": "apollo-client",
"path": "./dist/apollo-client.cjs.min.js",
"maxSize": "24.1 kB"
"maxSize": "24.2 kB"
}
],
"peerDependencies": {
Expand Down
79 changes: 79 additions & 0 deletions src/__tests__/local-state/export.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
},
});
}
);
});
23 changes: 16 additions & 7 deletions src/core/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -590,34 +590,43 @@ export class ObservableQuery<
iterateObserversSafely(this.observers, 'error', this.lastError = error);
};

const {
hasClientExports,
serverQuery
} = queryManager.transform(this.options.query);

queryManager.observeQuery<TData>(queryId, this.options, {
next: (result: ApolloQueryResult<TData>) => {
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);
}
});
Expand Down

0 comments on commit f2783bd

Please sign in to comment.