Skip to content

Commit

Permalink
Avoid network after incomplete optimistic cache results.
Browse files Browse the repository at this point in the history
This fixes an issue found by @darkbasic while working with optimistic
updates: #6183 (comment)

The cache-first FetchPolicy is important not just because it's the default
policy, but also because both cache-and-network and network-only turn into
cache-first after the first network request (#6353).

Once the cache-first policy is in effect for a query, any changes to the
cache that cause the query to begin reading incomplete data will generally
trigger a network request.

However, if the source of the changes is an optimistic update for a
mutation, it seems reasonable to avoid the network request during the
mutation, since there's a good chance the incompleteness of the optimistic
data is only temporary, and the client might read a complete result after
the optimistic update is rolled back, removing the need to do a network
request.

Of course, if the non-optimistic read following the rollback is
incomplete, a network request will be triggered, so skipping the network
request during optimistic updates does not mean ignoring legitimate
incompleteness forever.

Note: we already avoid sending network requests for queries that are
currently in flight, but in this case it's the mutation that's in flight,
so this commit introduces a way to prevent other affected queries (which
are not currently in flight, themselves) from hitting the network.
  • Loading branch information
benjamn committed Jun 9, 2020
1 parent 6f8d1b7 commit 37b898f
Show file tree
Hide file tree
Showing 8 changed files with 146 additions and 2 deletions.
1 change: 1 addition & 0 deletions src/cache/core/types/DataProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export namespace DataProxy {
result?: T;
complete?: boolean;
missing?: MissingFieldError[];
optimistic?: boolean;
}
}

Expand Down
9 changes: 8 additions & 1 deletion src/cache/inmemory/__tests__/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1460,6 +1460,7 @@ describe("InMemoryCache#broadcastWatches", function () {
},
},
complete: true,
optimistic: false,
}];

expect(receivedCallbackResults).toEqual([
Expand All @@ -1481,6 +1482,7 @@ describe("InMemoryCache#broadcastWatches", function () {
},
},
complete: true,
optimistic: false,
}];

expect(receivedCallbackResults).toEqual([
Expand All @@ -1502,6 +1504,7 @@ describe("InMemoryCache#broadcastWatches", function () {
},
},
complete: true,
optimistic: false,
}];

const received4 = [id4, 1, {
Expand All @@ -1511,6 +1514,7 @@ describe("InMemoryCache#broadcastWatches", function () {
},
},
complete: true,
optimistic: false,
}];

expect(receivedCallbackResults).toEqual([
Expand All @@ -1531,6 +1535,7 @@ describe("InMemoryCache#broadcastWatches", function () {
},
},
complete: true,
optimistic: false,
}];

expect(receivedCallbackResults).toEqual([
Expand Down Expand Up @@ -2108,10 +2113,12 @@ describe("InMemoryCache#modify", () => {
function makeResult(
__typename: string,
value: number,
complete: boolean = true,
complete = true,
optimistic = false,
) {
return {
complete,
optimistic,
result: {
[__typename.toLowerCase()]: {
__typename,
Expand Down
5 changes: 5 additions & 0 deletions src/cache/inmemory/__tests__/entityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1084,6 +1084,7 @@ describe('EntityStore', () => {
},
})).toEqual({
complete: false,
optimistic: false,
result: {
authorOfBook: tedWithoutHobby,
},
Expand Down Expand Up @@ -1653,6 +1654,7 @@ describe('EntityStore', () => {
})).toEqual({
complete: false,
missing,
optimistic: true,
result: {
book: {
__typename: "Book",
Expand Down Expand Up @@ -1684,6 +1686,7 @@ describe('EntityStore', () => {
})).toEqual({
complete: false,
missing,
optimistic: false,
result: {
book: {
__typename: "Book",
Expand All @@ -1700,6 +1703,7 @@ describe('EntityStore', () => {
returnPartialData: true,
})).toEqual({
complete: true,
optimistic: false,
result: {
book: {
__typename: "Book",
Expand Down Expand Up @@ -1900,6 +1904,7 @@ describe('EntityStore', () => {

expect(cuckoosCallingDiffResult).toEqual({
complete: false,
optimistic: false,
result: {
book: {
__typename: "Book",
Expand Down
4 changes: 4 additions & 0 deletions src/cache/inmemory/__tests__/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,7 @@ describe("type policies", function () {
}],
},
complete: false,
optimistic: false,
missing: [
makeMissingError(1),
makeMissingError(2),
Expand Down Expand Up @@ -1103,6 +1104,7 @@ describe("type policies", function () {
}],
},
complete: false,
optimistic: false,
missing: [
makeMissingError(1),
makeMissingError(3),
Expand Down Expand Up @@ -1160,6 +1162,7 @@ describe("type policies", function () {
}],
},
complete: false,
optimistic: false,
missing: [
makeMissingError(1),
makeMissingError(3),
Expand Down Expand Up @@ -1194,6 +1197,7 @@ describe("type policies", function () {
}],
},
complete: true,
optimistic: false,
});

expect(cache.readQuery({ query })).toEqual({
Expand Down
6 changes: 6 additions & 0 deletions src/cache/inmemory/__tests__/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,7 @@ describe('reading from the store', () => {
},
},
complete: true,
optimistic: false,
};

// We already have one diff because of the immediate:true above.
Expand All @@ -1045,6 +1046,7 @@ describe('reading from the store', () => {
},
},
complete: true,
optimistic: false,
};

expect(diffs).toEqual([
Expand Down Expand Up @@ -1072,6 +1074,7 @@ describe('reading from the store', () => {
},
},
complete: true,
optimistic: false,
};

expect(diffs).toEqual([
Expand Down Expand Up @@ -1113,6 +1116,7 @@ describe('reading from the store', () => {

const diffWithChildrenOfZeus = {
complete: true,
optimistic: false,
result: {
...diffWithoutDevouredSons.result,
ruler: {
Expand Down Expand Up @@ -1149,6 +1153,7 @@ describe('reading from the store', () => {

const diffWithZeusAsRuler = {
complete: true,
optimistic: false,
result: {
ruler: {
__typename: "Deity",
Expand Down Expand Up @@ -1327,6 +1332,7 @@ describe('reading from the store', () => {

const diffWithApolloAsRuler = {
complete: true,
optimistic: false,
result: apolloRulerResult,
};

Expand Down
3 changes: 2 additions & 1 deletion src/cache/inmemory/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import {
ReadQueryOptions,
NormalizedCache,
} from './types';
import { supportsResultCaching } from './entityStore';
import { supportsResultCaching, EntityStore } from './entityStore';
import { getTypenameFromStoreObject } from './helpers';
import { Policies, ReadMergeContext } from './policies';
import { InMemoryCache } from './inMemoryCache';
Expand Down Expand Up @@ -159,6 +159,7 @@ export class StoreReader {
result: execResult.result,
missing: execResult.missing,
complete: !hasMissingFields,
optimistic: !(store instanceof EntityStore.Root),
};
}

Expand Down
6 changes: 6 additions & 0 deletions src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,12 @@ export class QueryManager<TStore> {
];
}

if (diff.optimistic) {
return returnPartialData ? [
resultsFromCache(diff, queryInfo.markReady()),
] : [];
}

if (returnPartialData) {
return [
resultsFromCache(diff),
Expand Down
114 changes: 114 additions & 0 deletions src/core/__tests__/fetchPolicies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,120 @@ describe('no-cache', () => {
});
});

describe('cache-first', () => {
itAsync('does not trigger network request during optimistic update', (resolve, reject) => {
const results: any[] = [];
const client = new ApolloClient({
link: new ApolloLink((operation, forward) => {
return forward(operation).map(result => {
results.push(result);
return result;
});
}).concat(createMutationLink(reject)),
cache: new InMemoryCache,
});

let inOptimisticTransaction = false;

subscribeAndCount(reject, client.watchQuery({
query,
fetchPolicy: "cache-and-network",
returnPartialData: true,
}), (count, { data, loading, networkStatus }) => {
if (count === 1) {
expect(results.length).toBe(0);
expect(loading).toBe(true);
expect(networkStatus).toBe(NetworkStatus.loading);
expect(data).toEqual({});
} else if (count === 2) {
expect(results.length).toBe(1);
expect(loading).toBe(false);
expect(networkStatus).toBe(NetworkStatus.ready);
expect(data).toEqual({
author: {
__typename: "Author",
id: 1,
firstName: "John",
lastName: "Smith",
},
});

inOptimisticTransaction = true;
client.cache.recordOptimisticTransaction(cache => {
cache.writeQuery({
query,
data: {
author: {
__typename: "Bogus",
},
},
});
}, "bogus");

} else if (count === 3) {
expect(results.length).toBe(1);
expect(loading).toBe(false);
expect(networkStatus).toBe(NetworkStatus.ready);
expect(data).toEqual({
author: {
__typename: "Bogus",
},
});

setTimeout(() => {
inOptimisticTransaction = false;
client.cache.removeOptimistic("bogus");
}, 100);

} else if (count === 4) {
// A network request should not be triggered until after the bogus
// optimistic transaction has been removed.
expect(inOptimisticTransaction).toBe(false);
expect(results.length).toBe(1);
expect(loading).toBe(false);
expect(networkStatus).toBe(NetworkStatus.ready);
expect(data).toEqual({
author: {
__typename: "Author",
id: 1,
firstName: "John",
lastName: "Smith",
},
});

client.cache.writeQuery({
query,
data: {
author: {
__typename: "Author",
id: 2,
firstName: "Chinua",
lastName: "Achebe",
},
},
});

} else if (count === 5) {
expect(inOptimisticTransaction).toBe(false);
expect(results.length).toBe(1);
expect(loading).toBe(false);
expect(networkStatus).toBe(NetworkStatus.ready);
expect(data).toEqual({
author: {
__typename: "Author",
id: 2,
firstName: "Chinua",
lastName: "Achebe",
},
});
setTimeout(resolve, 100);
} else {
reject(new Error("unreached"));
}
});
});
});

describe('cache-and-network', function() {
itAsync('gives appropriate networkStatus for refetched queries', (resolve, reject) => {
const client = new ApolloClient({
Expand Down

0 comments on commit 37b898f

Please sign in to comment.