Make dataId parameter of cache.modify optional.

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 idomatic 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/client@3.0.0-beta.34 by PR #5909, so it has never been out of beta testing, so breaking changes are still fair game.
apollographql · Apr 21, 2020 · f7213a0 · f7213a0
1 parent 41901e2
commit f7213a0
Show file tree

Hide file tree

Showing 6 changed files with 43 additions and 37 deletions.
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();