Skip to content

Commit

Permalink
Make InMemoryCache#evict remove data from all EntityStore layers.
Browse files Browse the repository at this point in the history
Folks testing AC3 have already reported some confusion about evicted data
seeming to reappear in the context of optimistic mutation updates:
#5746 (comment)

To prevent this confusion, I think we need to honor the force implied by
the word "evict" and remove all traces of the given dataId/fieldName from
all layers the cache, rather than only evicting from the top layer of the
cache (that is, this.optimisticData).
  • Loading branch information
benjamn committed Jan 10, 2020
1 parent 572fade commit ea06ffd
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 18 deletions.
30 changes: 22 additions & 8 deletions src/cache/inmemory/__tests__/entityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -703,10 +703,8 @@ describe('EntityStore', () => {
},
title: "The Cuckoo's Calling",
},
"Author:Robert Galbraith": {
__typename: "Author",
name: "Robert Galbraith",
},
// The Robert Galbraith Author record is no longer here because
// cache.evict evicts data from all EntityStore layers.
});

cache.writeFragment({
Expand All @@ -721,7 +719,25 @@ describe('EntityStore', () => {
},
});

expect(cache.extract(true)).toEqual(snapshotWithBothNames);
expect(cache.extract(true)).toEqual({
ROOT_QUERY: {
__typename: "Query",
book: {
__ref: "Book:031648637X",
},
},
"Book:031648637X": {
__typename: "Book",
author: {
__ref: "Author:J.K. Rowling",
},
title: "The Cuckoo's Calling",
},
"Author:J.K. Rowling": {
__typename: "Author",
name: "J.K. Rowling",
},
});

expect(cache.retain("Author:Robert Galbraith")).toBe(2);

Expand All @@ -730,9 +746,7 @@ describe('EntityStore', () => {
expect(cache.release("Author:Robert Galbraith")).toBe(1);
expect(cache.release("Author:Robert Galbraith")).toBe(0);

expect(cache.gc()).toEqual([
"Author:Robert Galbraith",
]);
expect(cache.gc()).toEqual([]);

// If you're ever tempted to do this, you probably want to use cache.clear()
// instead, but evicting the ROOT_QUERY should work at least.
Expand Down
12 changes: 12 additions & 0 deletions src/cache/inmemory/entityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,20 @@ export abstract class EntityStore implements NormalizedCache {
this.group.dirty(dataId, fieldName);
});
}

return true;
}
}

return false;
}

public evict(dataId: string, fieldName?: string): boolean {
let evicted = this.delete(dataId, fieldName);
if (this instanceof Layer) {
evicted = this.parent.evict(dataId, fieldName) || evicted;
}
return evicted;
}

public clear(): void {
Expand Down
12 changes: 3 additions & 9 deletions src/cache/inmemory/inMemoryCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,15 +202,9 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
}

public evict(dataId: string, fieldName?: string): boolean {
if (this.optimisticData.has(dataId)) {
// Note that this deletion does not trigger a garbage collection, which
// is convenient in cases where you want to evict multiple entities before
// performing a single garbage collection.
this.optimisticData.delete(dataId, fieldName);
this.broadcastWatches();
return !this.optimisticData.has(dataId);
}
return false;
const evicted = this.optimisticData.evict(dataId, fieldName);
if (evicted) this.broadcastWatches();
return evicted;
}

public reset(): Promise<void> {
Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export interface NormalizedCache {
get(dataId: string): StoreObject;
getFieldValue(dataId: string, storeFieldName: string): StoreValue;
merge(dataId: string, incoming: StoreObject): void;
delete(dataId: string, fieldName?: string): void;
delete(dataId: string, fieldName?: string): boolean;
clear(): void;

// non-Map elements:
Expand Down

0 comments on commit ea06ffd

Please sign in to comment.