cache.modify: less strict types & dev runtime warnings

.changeset/weak-worms-fetch.md

@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+`cache.modify`: Less strict types & new dev runtime warnings.

.size-limit.cjs

@@ -10,7 +10,7 @@ const checks = [
   {
     path: "dist/index.js",
     import: "{ ApolloClient, InMemoryCache, HttpLink }",
-    limit: "32044",
+    limit: "32067",
   },
   ...[
     "ApolloProvider",

src/cache/core/__tests__/cache.ts

@@ -374,7 +374,7 @@ describe.skip("Cache type tests", () => {
           },
           children(field) {
             expectTypeOf(field).toEqualTypeOf<
-              ReadonlyArray<{ anotherObject: false }> | ReadonlyArray<Reference>
+              ReadonlyArray<{ anotherObject: false } | Reference>
             >();
             return field;
           },
@@ -440,7 +440,7 @@ describe.skip("Cache type tests", () => {
         id: "foo",
         fields(field) {
           expectTypeOf(field).toEqualTypeOf<
-            boolean | symbol | readonly OtherChildEntry[] | readonly Reference[]
+            boolean | symbol | ReadonlyArray<OtherChildEntry | Reference>
           >();
           return field;
         },
@@ -477,7 +477,7 @@ describe.skip("Cache type tests", () => {
         id: "foo",
         fields(field) {
           expectTypeOf(field).toEqualTypeOf<
-            boolean | symbol | readonly OtherChildEntry[] | readonly Reference[]
+            boolean | symbol | ReadonlyArray<OtherChildEntry | Reference>
           >();
           return field;
         },

src/cache/core/types/common.ts

@@ -5,6 +5,7 @@ import type {
   StoreObject,
   StoreValue,
   isReference,
+  AsStoreObject,
 } from "../../../utilities/index.js";
 
 import type { StorageType } from "../../inmemory/policies.js";
@@ -104,13 +105,12 @@ export type Modifier<T> = (
   details: ModifierDetails
 ) => T | DeleteModifier | InvalidateModifier;
 
-type StoreObjectValueMaybeReference<StoreVal> = StoreVal extends Record<
-  string,
-  any
->[]
-  ? Readonly<StoreVal> | readonly Reference[]
+type StoreObjectValueMaybeReference<StoreVal> = StoreVal extends Array<
+  infer Item extends Record<string, any>
+>
+  ? ReadonlyArray<AsStoreObject<Item> | Reference>
   : StoreVal extends Record<string, any>
-  ? StoreVal | Reference
+  ? AsStoreObject<StoreVal> | Reference
   : StoreVal;
 
 export type AllFieldsModifier<Entity extends Record<string, any>> = Modifier<

src/cache/inmemory/__tests__/cache.ts

@@ -18,6 +18,7 @@ import { StoreReader } from "../readFromStore";
 import { StoreWriter } from "../writeToStore";
 import { ObjectCanon } from "../object-canon";
 import { TypePolicies } from "../policies";
+import { spyOnConsole } from "../../../testing/internal";
 
 disableFragmentWarnings();
 
@@ -3454,6 +3455,225 @@ describe("InMemoryCache#modify", () => {
 
     expect(cache.extract()).toEqual(snapshot);
   });
+
+  it("warns if `modify` returns a mixed array of objects and references", () => {
+    const cache = new InMemoryCache();
+    const query = gql`
+      query {
+        me {
+          id
+          books {
+            id
+            title
+          }
+        }
+      }
+    `;
+
+    interface Book {
+      __typename: "Book";
+      id: string;
+      title: string;
+    }
+
+    const book1: Book = { __typename: "Book", id: "1", title: "1984" };
+    const book2: Book = { __typename: "Book", id: "2", title: "The Odyssey" };
+    const book3: Book = { __typename: "Book", id: "3", title: "The Hobbit" };
+    const book4: Book = { __typename: "Book", id: "4", title: "The Swarm" };
+
+    cache.writeQuery({
+      query,
+      data: {
+        me: {
+          __typename: "User",
+          id: "42",
+          books: [book1, book2, book3],
+        },
+      },
+    });
+
+    expect(cache.readQuery({ query })).toEqual({
+      me: {
+        __typename: "User",
+        books: [book1, book2, book3],
+        id: "42",
+      },
+    });
+
+    {
+      using consoleSpy = spyOnConsole("warn");
+      cache.modify<{ books: Book[] }>({
+        id: cache.identify({ __typename: "User", id: "42" }),
+        fields: {
+          books(existingBooks, { toReference }) {
+            return [toReference(existingBooks[2])!, book4];
+          },
+        },
+      });
+      expect(consoleSpy.warn).toHaveBeenLastCalledWith(
+        "cache.modify: Writing an array with a mix of both References and Objects will not result in the Objects being normalized correctly.\n" +
+          "Please convert the object instance %o to a Reference before writing it to the cache by calling `toReference(object, true)`.",
+        book4
+      );
+    }
+  });
+
+  it("warns if `modify` returns a Reference that is not part of the store as part of an array", () => {
+    const cache = new InMemoryCache();
+    const query = gql`
+      query {
+        me {
+          id
+          books {
+            id
+            title
+          }
+        }
+      }
+    `;
+
+    type Book = {
+      __typename: "Book";
+      id: string;
+      title: string;
+    };
+
+    const book1: Book = { __typename: "Book", id: "1", title: "1984" };
+    const book2: Book = { __typename: "Book", id: "2", title: "The Odyssey" };
+    const book3: Book = { __typename: "Book", id: "3", title: "The Hobbit" };
+    const book4: Book = { __typename: "Book", id: "4", title: "The Swarm" };
+
+    cache.writeQuery({
+      query,
+      data: {
+        me: {
+          __typename: "User",
+          id: "42",
+          books: [book1, book2, book3],
+        },
+      },
+    });
+
+    expect(cache.readQuery({ query })).toEqual({
+      me: {
+        __typename: "User",
+        books: [book1, book2, book3],
+        id: "42",
+      },
+    });
+
+    {
+      using consoleSpy = spyOnConsole("warn");
+      cache.modify<{ books: Book[] }>({
+        id: cache.identify({ __typename: "User", id: "42" }),
+        fields: {
+          books(existingBooks, { toReference }) {
+            return [...existingBooks, toReference(book4)!];
+          },
+        },
+      });
+      expect(consoleSpy.warn).toHaveBeenLastCalledWith(
+        "cache.modify: You are trying to write a Reference that is not part of the store: %o\n" +
+          "Please make sure to set the `mergeIntoStore` parameter to `true` when creating a Reference that is not part of the store yet:\n" +
+          "`toReference(object, true)`",
+        { __ref: "Book:4" }
+      );
+    }
+
+    // reading the cache *looks* good to the user
+    expect(cache.readQuery({ query })).toEqual({
+      me: {
+        __typename: "User",
+        // this is what we're warning about - book 4 is not in the store
+        books: [book1, book2, book3],
+        id: "42",
+      },
+    });
+    expect(cache.extract()).toEqual({
+      ROOT_QUERY: { __typename: "Query", me: { __ref: "User:42" } },
+      "Book:1": book1,
+      "Book:2": book2,
+      "Book:3": book3,
+      // no Book:4
+      "User:42": {
+        __typename: "User",
+        id: "42",
+        // Book:4 here is a dead ref
+        books: [
+          { __ref: "Book:1" },
+          { __ref: "Book:2" },
+          { __ref: "Book:3" },
+          { __ref: "Book:4" },
+        ],
+      },
+    });
+  });
+
+  it("warns if `modify` returns a Reference that is not part of the store", () => {
+    const cache = new InMemoryCache();
+    const query = gql`
+      query {
+        me {
+          id
+        }
+      }
+    `;
+
+    type User = {
+      __typename: string;
+      id: string;
+    };
+
+    cache.writeQuery({
+      query,
+      data: {
+        me: {
+          __typename: "User",
+          id: "42",
+        },
+      },
+    });
+
+    expect(cache.readQuery({ query })).toEqual({
+      me: {
+        __typename: "User",
+        id: "42",
+      },
+    });
+
+    {
+      using consoleSpy = spyOnConsole("warn");
+      cache.modify<{ me: User }>({
+        id: "ROOT_QUERY",
+        fields: {
+          me(existingUser, { toReference }) {
+            return toReference({
+              __typename: "User",
+              id: "43",
+            })!;
+          },
+        },
+      });
+      expect(consoleSpy.warn).toHaveBeenLastCalledWith(
+        "cache.modify: You are trying to write a Reference that is not part of the store: %o\n" +
+          "Please make sure to set the `mergeIntoStore` parameter to `true` when creating a Reference that is not part of the store yet:\n" +
+          "`toReference(object, true)`",
+        { __ref: "User:43" }
+      );
+    }
+
+    // reading the cache returns `null`
+    expect(cache.readQuery({ query })).toEqual(null);
+    expect(cache.extract()).toEqual({
+      // User:43 is a dead ref
+      ROOT_QUERY: { __typename: "Query", me: { __ref: "User:43" } },
+      "User:42": {
+        __typename: "User",
+        id: "42",
+      },
+      // no User:43
+    });
+  });
 });
 
 describe("ReactiveVar and makeVar", () => {

src/cache/inmemory/entityStore.ts

@@ -257,6 +257,51 @@ export abstract class EntityStore implements NormalizedCache {
               changedFields[storeFieldName] = newValue;
               needToMerge = true;
               fieldValue = newValue;
+
+              if (__DEV__) {
+                const checkReference = (ref: Reference) => {
+                  if (this.lookup(ref.__ref) === undefined) {
+                    invariant.warn(
+                      "cache.modify: You are trying to write a Reference that is not part of the store: %o\n" +
+                        "Please make sure to set the `mergeIntoStore` parameter to `true` when creating a Reference that is not part of the store yet:\n" +
+                        "`toReference(object, true)`",
+                      ref
+                    );
+                    return true;
+                  }
+                };
+                if (isReference(newValue)) {
+                  checkReference(newValue);
+                } else if (Array.isArray(newValue)) {
+                  // Warn about writing "mixed" arrays of Reference and non-Reference objects
+                  let seenReference: boolean = false;
+                  let someNonReference: unknown;
+                  for (const value of newValue) {
+                    if (isReference(value)) {
+                      seenReference = true;
+                      if (checkReference(value)) break;
+                    } else {
+                      // Do not warn on primitive values, since those could never be represented
+                      // by a reference. This is a valid (albeit uncommon) use case.
+                      if (typeof value === "object" && !!value) {
+                        const [id] = this.policies.identify(value);
+                        // check if object could even be referenced, otherwise we are not interested in it for this warning
+                        if (id) {
+                          someNonReference = value;
+                        }
+                      }
+                    }
+                    if (seenReference && someNonReference !== undefined) {
+                      invariant.warn(
+                        "cache.modify: Writing an array with a mix of both References and Objects will not result in the Objects being normalized correctly.\n" +
+                          "Please convert the object instance %o to a Reference before writing it to the cache by calling `toReference(object, true)`.",
+                        someNonReference
+                      );
+                      break;
+                    }
+                  }
+                }
+              }
             }
           }
         }

src/utilities/graphql/storeUtils.ts

@@ -55,6 +55,24 @@ export interface StoreObject {
   [storeFieldName: string]: StoreValue;
 }
 
+/**
+ * Workaround for a TypeScript quirk:
+ * types per default have an implicit index signature that makes them
+ * assignable to `StoreObject`.
+ * interfaces do not have that implicit index signature, so they cannot
+ * be assigned to `StoreObject`.
+ * This type just maps over a type or interface that is passed in,
+ * implicitly adding the index signature.
+ * That way, the result can be assigned to `StoreObject`.
+ *
+ * This is important if some user-defined interface is used e.g.
+ * in cache.modify, where the `toReference` method expects a
+ * `StoreObject` as input.
+ */
+export type AsStoreObject<T extends { __typename?: string }> = {
+  [K in keyof T]: T[K];
+};
+
 export function isDocumentNode(value: any): value is DocumentNode {
   return (
     isNonNullObject(value) &&

src/utilities/index.ts

@@ -39,6 +39,7 @@ export { print } from "./graphql/print.js";
 
 export type {
   StoreObject,
+  AsStoreObject,
   Reference,
   StoreValue,
   Directives,