Make dataId parameter of cache.modify optional. (#6178)

Most importantly, this change allows callers of cache.modify to avoid passing "ROOT_QUERY" as the ID for modifications of root Query data. Making the dataId parameter optional and moving it after the modifiers parameter felt much more idiomatic and understandable than than trying to support two conflicting type signatures for the modify method, even though that extra effort could have avoided the breaking change. Please remember that the cache.modify API was added in @apollo/[email protected] by PR #5909, so it has never been out of beta testing, so breaking changes are still fair game.
apollographql · Apr 21, 2020 · 6e5819b · 6e5819b
1 parent 41901e2
commit 6e5819b
Show file tree

Hide file tree

Showing 7 changed files with 46 additions and 39 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -73,14 +73,15 @@
 
 - `InMemoryCache` now has a method called `modify` which can be used to update the value of a specific field within a specific entity object:
   ```ts
-  cache.modify(cache.identify(post), {
+  cache.modify({
     comments(comments: Reference[], { readField }) {
       return comments.filter(comment => idToRemove !== readField("id", comment));
     },
-  });
+  }, cache.identify(post));
   ```
   This API gracefully handles cases where multiple field values are associated with a single field name, and also removes the need for updating the cache by reading a query or fragment, modifying the result, and writing the modified result back into the cache. Behind the scenes, the `cache.evict` method is now implemented in terms of `cache.modify`. <br/>
   [@benjamn](https://github.com/benjamn) in [#5909](https://github.com/apollographql/apollo-client/pull/5909)
+  and [#6178](https://github.com/apollographql/apollo-client/pull/6178)
 
 - `InMemoryCache` provides a new API for storing client state that can be updated from anywhere:
   ```ts

diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts
@@ -2994,7 +2994,7 @@ describe('@connection', () => {
     checkLastResult(abResults, a456bOyez);
     const cSee = checkLastResult(cResults, { c: "see" });
 
-    cache.modify("ROOT_QUERY", {
+    cache.modify({
       c(value) {
         expect(value).toBe("see");
         return "saw";

diff --git a/src/__tests__/local-state/resolvers.ts b/src/__tests__/local-state/resolvers.ts
@@ -553,12 +553,12 @@ describe('Writing cache data from resolvers', () => {
               },
             });
 
-            cache.modify('Object:uniqueId', {
+            cache.modify({
               field(value) {
                 expect(value).toBe(1);
                 return 2;
               },
-            })
+            }, 'Object:uniqueId');
 
             return { start: true };
           },

diff --git a/src/cache/core/cache.ts b/src/cache/core/cache.ts
@@ -70,9 +70,9 @@ export abstract class ApolloCache<TSerialized> implements DataProxy {
   }
 
   public modify(
-    dataId: string,
     modifiers: Modifier<any> | Modifiers,
-    optimistic = false,
+    dataId?: string,
+    optimistic?: boolean,
   ): boolean {
     return false;
   }

diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts
@@ -1556,7 +1556,7 @@ describe("InMemoryCache#modify", () => {
     const resultBeforeModify = cache.readQuery({ query });
     expect(resultBeforeModify).toEqual({ a: 0, b: 0, c: 0 });
 
-    cache.modify("ROOT_QUERY", (value, { fieldName }) => {
+    cache.modify((value, { fieldName }) => {
       switch (fieldName) {
         case "a": return value + 1;
         case "b": return value - 1;
@@ -1600,7 +1600,7 @@ describe("InMemoryCache#modify", () => {
     expect(resultBeforeModify).toEqual({ a: 0, b: 0, c: 0 });
 
     let checkedTypename = false;
-    cache.modify("ROOT_QUERY", {
+    cache.modify({
       a(value) { return value + 1 },
       b(value) { return value - 1 },
       __typename(t: string, { readField }) {
@@ -1693,11 +1693,11 @@ describe("InMemoryCache#modify", () => {
     const authorId = cache.identify(currentlyReading.author);
     expect(authorId).toBe('Author:{"name":"Ezra Klein"}');
 
-    cache.modify(authorId, {
+    cache.modify({
       yearOfBirth(yob) {
         return yob + 1;
       },
-    });
+    }, authorId);
 
     const yobResult = cache.readFragment({
       id: authorId,
@@ -1713,22 +1713,22 @@ describe("InMemoryCache#modify", () => {
 
     // Modifying the Book in order to modify the Author is fancier than
     // necessary, but we want fancy use cases to work, too.
-    cache.modify(bookId, {
+    cache.modify({
       author(author: Reference, { readField }) {
         expect(readField("title")).toBe("Why We're Polarized");
         expect(readField("name", author)).toBe("Ezra Klein");
-        cache.modify(cache.identify({
-          __typename: readField("__typename", author),
-          name: readField("name", author),
-        }), {
+        cache.modify({
           yearOfBirth(yob, { DELETE }) {
             expect(yob).toBe(1984);
             return DELETE;
           },
-        });
+        }, cache.identify({
+          __typename: readField("__typename", author),
+          name: readField("name", author),
+        }));
         return author;
       }
-    });
+    }, bookId);
 
     const snapshotWithoutYOB = cache.extract();
     expect(snapshotWithoutYOB[authorId].yearOfBirth).toBeUndefined();
@@ -1756,7 +1756,7 @@ describe("InMemoryCache#modify", () => {
     });
 
     // Delete the whole Book.
-    cache.modify(bookId, (_, { DELETE }) => DELETE);
+    cache.modify((_, { DELETE }) => DELETE, bookId);
 
     const snapshotWithoutBook = cache.extract();
     expect(snapshotWithoutBook[bookId]).toBeUndefined();
@@ -1775,10 +1775,10 @@ describe("InMemoryCache#modify", () => {
     });
 
     // Delete all fields of the Author, which also removes the object.
-    cache.modify(authorId, {
+    cache.modify({
       __typename(_, { DELETE }) { return DELETE },
       name(_, { DELETE }) { return DELETE },
-    });
+    }, authorId);
 
     const snapshotWithoutAuthor = cache.extract();
     expect(snapshotWithoutAuthor[authorId]).toBeUndefined();
@@ -1792,7 +1792,7 @@ describe("InMemoryCache#modify", () => {
       },
     });
 
-    cache.modify("ROOT_QUERY", (_, { DELETE }) => DELETE);
+    cache.modify((_, { DELETE }) => DELETE);
     expect(cache.extract()).toEqual({});
   });
 
@@ -1904,10 +1904,7 @@ describe("InMemoryCache#modify", () => {
       },
     });
 
-    cache.modify(cache.identify({
-      __typename: "Thread",
-      tid: 123,
-    }), {
+    cache.modify({
       comments(comments: Reference[], { readField }) {
         debugger;
         expect(Object.isFrozen(comments)).toBe(true);
@@ -1918,7 +1915,10 @@ describe("InMemoryCache#modify", () => {
         expect(filtered.length).toBe(2);
         return filtered;
       },
-    });
+    }, cache.identify({
+      __typename: "Thread",
+      tid: 123,
+    }));
 
     expect(cache.gc()).toEqual(['Comment:{"id":"c1"}']);
 
@@ -1965,12 +1965,12 @@ describe("InMemoryCache#modify", () => {
       })
     }, "transaction");
 
-    cache.modify("ROOT_QUERY", {
+    cache.modify({
       b(value, { DELETE }) {
         expect(value).toBe(2);
         return DELETE;
       },
-    }, true);
+    }, "ROOT_QUERY", true);
 
     expect(cache.extract(true)).toEqual({
       ROOT_QUERY: {
@@ -1980,11 +1980,11 @@ describe("InMemoryCache#modify", () => {
       },
     });
 
-    cache.modify("ROOT_QUERY", (value, { fieldName }) => {
+    cache.modify((value, { fieldName }) => {
       expect(fieldName).not.toBe("b");
       if (fieldName === "a") expect(value).toBe(1);
       if (fieldName === "c") expect(value).toBe(3);
-    }, true);
+    }, "ROOT_QUERY", true);
 
     cache.removeOptimistic("transaction");
 
@@ -2079,22 +2079,22 @@ describe("InMemoryCache#modify", () => {
     const aId = cache.identify({ __typename: "A", id: 1 });
     const bId = cache.identify({ __typename: "B", id: 1 });
 
-    cache.modify(aId, {
+    cache.modify({
       value(x: number) {
         return x + 1;
       },
-    });
+    }, aId);
 
     const a124 = makeResult("A", 124);
 
     expect(aResults).toEqual([a123, a124]);
     expect(bResults).toEqual([b321]);
 
-    cache.modify(bId, {
+    cache.modify({
       value(x: number) {
         return x + 1;
       },
-    });
+    }, bId);
 
     const b322 = makeResult("B", 322);
 
@@ -2180,7 +2180,7 @@ describe("InMemoryCache#modify", () => {
     function check(isbnToDelete?: string) {
       let bookCount = 0;
 
-      cache.modify("ROOT_QUERY", {
+      cache.modify({
         book(book: Reference, {
           fieldName,
           storeFieldName,

diff --git a/src/cache/inmemory/__tests__/entityStore.ts b/src/cache/inmemory/__tests__/entityStore.ts
@@ -1623,7 +1623,7 @@ describe('EntityStore', () => {
       },
     });
 
-    cache.modify(cuckoosCallingId, {
+    cache.modify({
       title(title: string, {
         isReference,
         toReference,
@@ -1656,7 +1656,7 @@ describe('EntityStore', () => {
         // Typography matters!
         return title.split("'").join("’");
       },
-    });
+    }, cuckoosCallingId);
 
     expect(cache.extract()).toEqual({
       ...threeBookSnapshot,

diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts
@@ -151,10 +151,16 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
   }
 
   public modify(
-    dataId: string,
     modifiers: Modifier<any> | Modifiers,
+    dataId = "ROOT_QUERY",
     optimistic = false,
   ): boolean {
+    if (typeof modifiers === "string") {
+      // In beta testing of Apollo Client 3, the dataId parameter used to
+      // come before the modifiers. The type system should complain about
+      // this, but it doesn't have to be fatal if we fix it here.
+      [modifiers, dataId] = [dataId as any, modifiers];
+    }
     const store = optimistic ? this.optimisticData : this.data;
     if (store.modify(dataId, modifiers)) {
       this.broadcastWatches();