From d6cb128206b5bbf7db7430a87cef3b08d4cc0a2c Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 29 May 2020 16:32:10 -0400 Subject: [PATCH 1/2] Require Cache.EvictOptions when calling cache.evict. We've been finding lately that named options APIs are much easier to work with (and later to extend). Since we're approaching a major new version of Apollo Client (3.0), and the cache.evict method did nothing in AC2 anyway, there's no sense preserving the alternate API with positional arguments. --- src/__tests__/client.ts | 4 +- src/cache/core/cache.ts | 8 --- src/cache/core/types/Cache.ts | 2 +- src/cache/inmemory/__tests__/entityStore.ts | 65 +++++++++++++++------ src/cache/inmemory/__tests__/policies.ts | 4 +- src/cache/inmemory/entityStore.ts | 22 +++---- src/cache/inmemory/inMemoryCache.ts | 26 ++++----- 7 files changed, 77 insertions(+), 54 deletions(-) diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts index 8be5e5b6796..26b69a2b5b6 100644 --- a/src/__tests__/client.ts +++ b/src/__tests__/client.ts @@ -2913,7 +2913,7 @@ describe('@connection', () => { expect(checkLastResult(abResults, a456bOyez)).toBe(a456bOyez); // Now invalidate the ROOT_QUERY.a field. - client.cache.evict("ROOT_QUERY", "a"); + client.cache.evict({ fieldName: "a" }); await wait(); // The results are structurally the same, but the result objects have @@ -2957,7 +2957,7 @@ describe('@connection', () => { checkLastResult(abResults, a456bOyez); checkLastResult(cResults, { c: "saw" }); - client.cache.evict("ROOT_QUERY", "c"); + client.cache.evict({ fieldName: "c" }); await wait(); checkLastResult(aResults, a456); diff --git a/src/cache/core/cache.ts b/src/cache/core/cache.ts index acd20c2288a..7df44dfa5f1 100644 --- a/src/cache/core/cache.ts +++ b/src/cache/core/cache.ts @@ -28,14 +28,6 @@ export abstract class ApolloCache implements DataProxy { // removed from the cache. public abstract evict(options: Cache.EvictOptions): boolean; - // For backwards compatibility, evict can also take positional - // arguments. Please prefer the Cache.EvictOptions style (above). - public abstract evict( - id: string, - field?: string, - args?: Record, - ): boolean; - // intializer / offline / ssr API /** * Replaces existing state in the cache (if any) with the values expressed by diff --git a/src/cache/core/types/Cache.ts b/src/cache/core/types/Cache.ts index ea18b2ab627..2ef3e025df6 100644 --- a/src/cache/core/types/Cache.ts +++ b/src/cache/core/types/Cache.ts @@ -27,7 +27,7 @@ export namespace Cache { } export interface EvictOptions { - id: string; + id?: string; fieldName?: string; args?: Record; broadcast?: boolean; diff --git a/src/cache/inmemory/__tests__/entityStore.ts b/src/cache/inmemory/__tests__/entityStore.ts index 99fad8e91de..75130b44a06 100644 --- a/src/cache/inmemory/__tests__/entityStore.ts +++ b/src/cache/inmemory/__tests__/entityStore.ts @@ -620,7 +620,7 @@ describe('EntityStore', () => { }, }); - expect(cache.evict("Author:J.K. Rowling")).toBe(false); + expect(cache.evict({ id: "Author:J.K. Rowling" })).toBe(false); const bookAuthorFragment = gql` fragment BookAuthor on Book { @@ -689,7 +689,7 @@ describe('EntityStore', () => { expect(cache.gc()).toEqual([]); - expect(cache.evict("Author:Robert Galbraith")).toBe(true); + expect(cache.evict({ id: "Author:Robert Galbraith" })).toBe(true); expect(cache.gc()).toEqual([]); @@ -754,9 +754,27 @@ describe('EntityStore', () => { 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. - expect(cache.evict("ROOT_QUERY")).toBe(true); + function checkFalsyEvictId(id: any) { + expect(id).toBeFalsy(); + expect(cache.evict({ + // Accidentally passing a falsy/undefined options.id to + // cache.evict (perhaps because cache.identify failed) should + // *not* cause the ROOT_QUERY object to be evicted! In order for + // cache.evict to default to ROOT_QUERY, the options.id property + // must be *absent* (not just undefined). + id, + })).toBe(false); + } + checkFalsyEvictId(void 0); + checkFalsyEvictId(null); + checkFalsyEvictId(false); + checkFalsyEvictId(0); + checkFalsyEvictId(""); + + // In other words, this is how you evict the entire ROOT_QUERY + // object. If you're ever tempted to do this, you probably want to use + // cache.clear() instead, but evicting the ROOT_QUERY should work. + expect(cache.evict({})).toBe(true); expect(cache.extract(true)).toEqual({ "Book:031648637X": { @@ -1177,7 +1195,10 @@ describe('EntityStore', () => { }, }); - cache.evict('ROOT_QUERY', 'authorOfBook', { isbn: "1" }); + cache.evict({ + fieldName: 'authorOfBook', + args: { isbn: "1" }, + }); expect(cache.extract()).toEqual({ ROOT_QUERY: { @@ -1195,7 +1216,10 @@ describe('EntityStore', () => { }, }); - cache.evict('ROOT_QUERY', 'authorOfBook', { isbn: '3' }); + cache.evict({ + fieldName: 'authorOfBook', + args: { isbn: '3' }, + }); expect(cache.extract()).toEqual({ ROOT_QUERY: { @@ -1213,7 +1237,10 @@ describe('EntityStore', () => { }, }); - cache.evict('ROOT_QUERY', 'authorOfBook', {}); + cache.evict({ + fieldName: 'authorOfBook', + args: {}, + }); expect(cache.extract()).toEqual({ ROOT_QUERY: { @@ -1226,7 +1253,9 @@ describe('EntityStore', () => { }, }); - cache.evict('ROOT_QUERY', 'authorOfBook');; + cache.evict({ + fieldName: 'authorOfBook', + }); expect(cache.extract()).toEqual({ ROOT_QUERY: { @@ -1506,12 +1535,14 @@ describe('EntityStore', () => { query: queryWithoutAliases, })).toBe(resultWithoutAliases); - cache.evict(cache.identify({ - __typename: "ABCs", - a: "ay", - b: "bee", - c: "see", - })!); + cache.evict({ + id: cache.identify({ + __typename: "ABCs", + a: "ay", + b: "bee", + c: "see", + }), + }); expect(cache.extract()).toEqual({ ROOT_QUERY: { @@ -1590,7 +1621,7 @@ describe('EntityStore', () => { id: 2, })!; - expect(cache.evict(authorId)).toBe(true); + expect(cache.evict({ id: authorId })).toBe(true); expect(cache.extract(true)).toEqual({ "Book:1": { @@ -1604,7 +1635,7 @@ describe('EntityStore', () => { }, }); - expect(cache.evict(authorId)).toBe(false); + expect(cache.evict({ id: authorId })).toBe(false); const missing = [ new MissingFieldError( diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts index 89ac93e61e6..c2a7b7b8c2f 100644 --- a/src/cache/inmemory/__tests__/policies.ts +++ b/src/cache/inmemory/__tests__/policies.ts @@ -2513,7 +2513,9 @@ describe("type policies", function () { expect(cache.gc()).toEqual([]); - expect(cache.evict("ROOT_QUERY", "book")).toBe(true); + expect(cache.evict({ + fieldName: "book", + })).toBe(true); expect(cache.gc().sort()).toEqual([ 'Book:{"isbn":"0393354326"}', diff --git a/src/cache/inmemory/entityStore.ts b/src/cache/inmemory/entityStore.ts index 1c9909e51f0..6823671cfe0 100644 --- a/src/cache/inmemory/entityStore.ts +++ b/src/cache/inmemory/entityStore.ts @@ -207,17 +207,19 @@ export abstract class EntityStore implements NormalizedCache { public evict(options: Cache.EvictOptions): boolean { let evicted = false; - if (hasOwn.call(this.data, options.id)) { - evicted = this.delete(options.id, options.fieldName, options.args); - } - if (this instanceof Layer) { - evicted = this.parent.evict(options) || evicted; + if (options.id) { + if (hasOwn.call(this.data, options.id)) { + evicted = this.delete(options.id, options.fieldName, options.args); + } + if (this instanceof Layer) { + evicted = this.parent.evict(options) || evicted; + } + // Always invalidate the field to trigger rereading of watched + // queries, even if no cache data was modified by the eviction, + // because queries may depend on computed fields with custom read + // functions, whose values are not stored in the EntityStore. + this.group.dirty(options.id, options.fieldName || "__exists"); } - // Always invalidate the field to trigger rereading of watched - // queries, even if no cache data was modified by the eviction, - // because queries may depend on computed fields with custom read - // functions, whose values are not stored in the EntityStore. - this.group.dirty(options.id, options.fieldName || "__exists"); return evicted; } diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index e1c2693b194..18486d524d8 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -237,28 +237,24 @@ export class InMemoryCache extends ApolloCache { return this.policies.identify(object)[0]; } - public evict( - idOrOptions: string | Cache.EvictOptions, - fieldName?: string, - args?: Record, - ): boolean { + public evict(options: Cache.EvictOptions): boolean { + if (!options.id) { + if (hasOwn.call(options, "id")) { + // See comment in modify method about why we return false when + // options.id exists but is falsy/undefined. + return false; + } + options = { ...options, id: "ROOT_QUERY" }; + } try { // It's unlikely that the eviction will end up invoking any other // cache update operations while it's running, but {in,de}crementing // this.txCount still seems like a good idea, for uniformity with // the other update methods. ++this.txCount; - return this.optimisticData.evict( - typeof idOrOptions === "string" ? { - id: idOrOptions, - fieldName, - args, - } : idOrOptions, - ); + return this.optimisticData.evict(options); } finally { - if (!--this.txCount && - (typeof idOrOptions === "string" || - idOrOptions.broadcast !== false)) { + if (!--this.txCount && options.broadcast !== false) { this.broadcastWatches(); } } From 2fff015da1d63870f48db640cffcc7e5bc34f0cc Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 29 May 2020 17:17:15 -0400 Subject: [PATCH 2/2] Mention that cache.evict now requires Cache.EvictOptions in CHANGELOG.md. --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae299f2fbd6..d3c316a6e3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,8 +59,9 @@ - `InMemoryCache` now supports tracing garbage collection and eviction. Note that the signature of the `evict` method has been simplified in a potentially backwards-incompatible way.
[@benjamn](https://github.com/benjamn) in [#5310](https://github.com/apollographql/apollo-client/pull/5310) -- The `cache.evict` method can optionally take an arguments object as its third parameter (following the entity ID and field name), to delete only those field values with specific arguments.
+- **[beta-BREAKING]** Please note that the `cache.evict` method now requires `Cache.EvictOptions`, though it previously supported positional arguments as well.
[@danReynolds](https://github.com/danReynolds) in [#6141](https://github.com/apollographql/apollo-client/pull/6141) + [@benjamn](https://github.com/benjamn) in [#6364](https://github.com/apollographql/apollo-client/pull/6364) - Cache methods that would normally trigger a broadcast, like `cache.evict`, `cache.writeQuery`, and `cache.writeFragment`, can now be called with a named options object, which supports a `broadcast: boolean` property that can be used to silence the broadcast, for situations where you want to update the cache multiple times without triggering a broadcast each time.
[@benjamn](https://github.com/benjamn) in [#6288](https://github.com/apollographql/apollo-client/pull/6288)