Skip to content

Commit

Permalink
Avoid full reobservation for broadcasts from optimistic transactions.
Browse files Browse the repository at this point in the history
I first attempted to solve this bug in #6419, but that approach was
flawed, and we ultimately reverted it in #6493. Both of these changes
happened shortly before the AC3 launch (rc.3 and rc.9, respectively).

The key to this solution is that diff.fromOptimisticTransaction is only
ever set by the InMemoryCache broadcast code, when we know that we've just
performed an optimistic transaction, and we're broadcasting to a query
watcher that requested optimistic data. The QueryInfo class receives this
broadcast, and uses diff.fromOptimisticTransaction to decide whether to do
a full reapplication of the chosen fetch policy by calling oq.reobserve(),
or simply to deliver a single cache result by calling oq.observe().
  • Loading branch information
benjamn committed Aug 17, 2020
1 parent 1dd9301 commit 97f3caf
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 9 deletions.
3 changes: 3 additions & 0 deletions src/__tests__/optimistic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1937,6 +1937,7 @@ describe('optimistic mutation results', () => {
expect(optimisticDiffs).toEqual([
{
complete: true,
fromOptimisticTransaction: true,
result: {
items: manualItems,
},
Expand Down Expand Up @@ -2099,12 +2100,14 @@ describe('optimistic mutation results', () => {
expect(optimisticDiffs).toEqual([
{
complete: true,
fromOptimisticTransaction: true,
result: {
items: manualItems,
},
},
{
complete: true,
fromOptimisticTransaction: true,
result: {
items: [...manualItems, optimisticItem],
},
Expand Down
1 change: 1 addition & 0 deletions src/cache/core/types/DataProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export namespace DataProxy {
result?: T;
complete?: boolean;
missing?: MissingFieldError[];
fromOptimisticTransaction?: boolean;
}
}

Expand Down
31 changes: 23 additions & 8 deletions src/cache/inmemory/inMemoryCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -278,11 +278,14 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
}
};

let fromOptimisticTransaction = false;

if (typeof optimisticId === 'string') {
// Note that there can be multiple layers with the same optimisticId.
// When removeOptimistic(id) is called for that id, all matching layers
// will be removed, and the remaining layers will be reapplied.
this.optimisticData = this.optimisticData.addLayer(optimisticId, perform);
fromOptimisticTransaction = true;
} else if (optimisticId === null) {
// Ensure both this.data and this.optimisticData refer to the root
// (non-optimistic) layer of the cache during the transaction. Note
Expand All @@ -297,7 +300,7 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
}

// This broadcast does nothing if this.txCount > 0.
this.broadcastWatches();
this.broadcastWatches(fromOptimisticTransaction);
}

public transformDocument(document: DocumentNode): DocumentNode {
Expand All @@ -316,14 +319,17 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
return document;
}

protected broadcastWatches() {
protected broadcastWatches(fromOptimisticTransaction?: boolean) {
if (!this.txCount) {
this.watches.forEach(c => this.maybeBroadcastWatch(c));
this.watches.forEach(c => this.maybeBroadcastWatch(c, fromOptimisticTransaction));
}
}

private maybeBroadcastWatch = wrap((c: Cache.WatchOptions) => {
return this.broadcastWatch.call(this, c);
private maybeBroadcastWatch = wrap((
c: Cache.WatchOptions,
fromOptimisticTransaction?: boolean,
) => {
return this.broadcastWatch.call(this, c, !!fromOptimisticTransaction);
}, {
makeCacheKey: (c: Cache.WatchOptions) => {
// Return a cache key (thus enabling result caching) only if we're
Expand Down Expand Up @@ -354,7 +360,10 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
// simpler to check for changes after recomputing a result but before
// broadcasting it, but this wrapping approach allows us to skip both
// the recomputation and the broadcast, in most cases.
private broadcastWatch(c: Cache.WatchOptions) {
private broadcastWatch(
c: Cache.WatchOptions,
fromOptimisticTransaction: boolean,
) {
// First, invalidate any other maybeBroadcastWatch wrapper functions
// currently depending on this Cache.WatchOptions object (including
// the one currently calling broadcastWatch), so they will be included
Expand All @@ -372,10 +381,16 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
// changed since they were previously delivered.
this.watchDep(c);

c.callback(this.diff({
const diff = this.diff<any>({
query: c.query,
variables: c.variables,
optimistic: c.optimistic,
}));
});

if (c.optimistic && fromOptimisticTransaction) {
diff.fromOptimisticTransaction = true;
}

c.callback(diff);
}
}
13 changes: 12 additions & 1 deletion src/core/QueryInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,18 @@ export class QueryInfo {

if (oq) {
oq["queryInfo"] = this;
this.listeners.add(this.oqListener = () => oq.reobserve());
this.listeners.add(this.oqListener = () => {
// If this.diff came from an optimistic transaction, deliver the
// current cache data to the ObservableQuery, but don't perform a
// full reobservation, since oq.reobserve might make a network
// request, and we don't want to trigger network requests for
// optimistic updates.
if (this.getDiff().fromOptimisticTransaction) {
oq.observe();
} else {
oq.reobserve();
}
});
} else {
delete this.oqListener;
}
Expand Down

0 comments on commit 97f3caf

Please sign in to comment.