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

@@ -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. <br/>
   [@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. <br/>
+- **[beta-BREAKING]** Please note that the `cache.evict` method now requires `Cache.EvictOptions`, though it previously supported positional arguments as well. <br/>
   [@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. <br/>
   [@benjamn](https://github.com/benjamn) in [#6288](https://github.com/apollographql/apollo-client/pull/6288)

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);

src/cache/core/cache.ts

@@ -28,14 +28,6 @@ export abstract class ApolloCache<TSerialized> 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<string, any>,
-  ): boolean;
-
   // intializer / offline / ssr API
   /**
    * Replaces existing state in the cache (if any) with the values expressed by

src/cache/core/types/Cache.ts

@@ -28,7 +28,7 @@ export namespace Cache {
   }
 
   export interface EvictOptions {
-    id: string;
+    id?: string;
     fieldName?: string;
     args?: Record<string, any>;
     broadcast?: boolean;

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(

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"}',

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;
   }
 

src/cache/inmemory/inMemoryCache.ts

@@ -236,28 +236,24 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
     return this.policies.identify(object)[0];
   }
 
-  public evict(
-    idOrOptions: string | Cache.EvictOptions,
-    fieldName?: string,
-    args?: Record<string, any>,
-  ): 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();
       }
     }