From 13ad552d355f640e4b5b2530810e9f4b1df836bd Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 29 May 2020 19:35:18 -0400 Subject: [PATCH] Require Cache.EvictOptions when calling cache.evict. (#6364) We've been finding lately that named options APIs are much easier to work with (and later to extend), because you can think about the different options independently, and future additions of new named options tend to be much less disruptive for existing code. 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, now that we support `Cache.EvictOptions`. In other words, this is definitely a breaking change, but only for those who have been using using the `@apollo/client` betas. Thank you for your patience and understanding; we know changes like this can be annoying if you were using `cache.evict` heavily, but we think the uniformity of always requiring `Cache.EvictOptions` will be simpler and more flexible in the long run. --- CHANGELOG.md | 3 +- 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 ++++----- 8 files changed, 79 insertions(+), 55 deletions(-) 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) 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 f35942a1882..ff7e0764f7c 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 60aad9ea380..21c2535538e 100644 --- a/src/cache/core/types/Cache.ts +++ b/src/cache/core/types/Cache.ts @@ -28,7 +28,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 6b155ae67b2..847afd04e7b 100644 --- a/src/cache/inmemory/entityStore.ts +++ b/src/cache/inmemory/entityStore.ts @@ -214,17 +214,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 71d3a283bf8..1ad6d3bf461 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -236,28 +236,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(); } }