Improve TypeScript type-safety of cache.modify (#10895)

* feat(cache): accept DELETE and INVALIDATE modifiers in the return type

Problem: The `DELETE` and `INVALIDATE` modifiers type is `any`.
Returning them from the `Modifier<T>` function triggers the
`@typescript-eslint/no-unsafe-return` ESLint rule [0],
since the modifier is then returning a value with type `any`.

Solution: Set the types of these modifiers to two different `unique
symbol`s. These serve as unique opaque types. The consuming code should
not peek into them. These 2 types are permitted in the return type of
the `Modifier<T>` function, since they are not `any` anymore, and thus,
are not assignable to `T`.

This change only affects TypeScript types. It has no runtime impact.

[0]: https://typescript-eslint.io/rules/no-unsafe-return/

* feat(cache): type inference for cache `Modifiers` type

Problem: The `fields` property in `cache.modify` did not offer ways to
infer the field names or values based on the field type. It accepted any
field names and the field values were `any`.

Solution: Add a generic type parameter for the object type to the
`Modifiers` type. The code can now use the `satisfies Modifiers<...>`
operator to inform TypeScript about the possible field names and values.

Field values include `Reference`s for objects and arrays. The consuming
code is then responsible for checking (or asserting) that the field
value is or is not a reference.

* refactor(cache): add default value for Modifiers generic parameter

Makes the change backwards-compatible.

* refactor(cache): use interfaces for invalidate and delete modifier types

Interfaces retain their names in error messages and hover dialogs. Type
aliases did not.

* refactor(cache): add generic parameter for ModifyOptions

A generic parameter is easier to discover and set than
`fields: { ... } satisfies Modifiers<...>`

* refactor(cache): use Record<string, any> as the default Entity type

Use a more appropriate real-life default type for `cache.modify`.

* code review adjustments

* refactor(cache-test): replace assertType with expect-type library

Use an already installed library instead of introducing a new utility
function.

* chore: use generic parameter instead of satisfies in changeset

Promote the use of a generic parameter of `cache.modify` over using
`satisfies` which requires importing the `Modifers` type.

* fix(cache): use AllFieldsModifier type in cache.modify parameter

---------

Co-authored-by: Lenz Weber-Tronic <[email protected]>

.changeset/afraid-zebras-punch.md

@@ -0,0 +1,24 @@
+---
+"@apollo/client": minor
+---
+
+Add generic type parameter for the entity modified in `cache.modify`. Improves
+TypeScript type inference for that type's fields and values of those fields.
+
+Example:
+
+```ts
+cache.modify<Book>({
+  id: cache.identify(someBook),
+  fields: {
+    title: (title) => {
+      // title has type `string`.
+      // It used to be `any`.
+    },
+    author: (author) => {
+      // author has type `Reference | Book["author"]`.
+      // It used to be `any`.
+    },
+  },
+});
+```

.changeset/cold-tips-accept.md

@@ -0,0 +1,9 @@
+---
+"@apollo/client": minor
+---
+
+Use unique opaque types for the `DELETE` and `INVALIDATE` Apollo cache modifiers.
+
+This increases type safety, since these 2 modifiers no longer have the `any` type.
+Moreover, it no longer triggers [the `@typescript-eslint/no-unsafe-return`
+rule](https://typescript-eslint.io/rules/no-unsafe-return/).

src/cache/core/__tests__/cache.ts

@@ -2,7 +2,7 @@ import gql from 'graphql-tag';
 import { ApolloCache } from '../cache';
 import { Cache, DataProxy } from '../..';
 import { Reference } from '../../../utilities/graphql/storeUtils';
-
+import { expectTypeOf } from 'expect-type'
 class TestCache extends ApolloCache<unknown> {
   constructor() {
     super();
@@ -308,3 +308,142 @@ describe('abstract cache', () => {
     });
   });
 });
+
+describe.skip('Cache type tests', () => {
+  describe('modify', () => {
+    test('field types are inferred correctly from passed entity type', () => {
+      const cache = new TestCache();
+      cache.modify<{
+        prop1: string;
+        prop2: number;
+        child: {
+          someObject: true
+        },
+        children: {
+          anotherObject: false
+        }[]
+      }>({
+        fields: {
+          prop1(field) {
+            expectTypeOf(field).toEqualTypeOf<string>();
+            return field;
+          },
+          prop2(field) {
+            expectTypeOf(field).toEqualTypeOf<number>();
+            return field;
+          },
+          child(field) {
+            expectTypeOf(field).toEqualTypeOf<{ someObject: true } | Reference>();
+            return field;
+          },
+          children(field) {
+            expectTypeOf(field).toEqualTypeOf<(ReadonlyArray<{ anotherObject: false }>) | ReadonlyArray<Reference>>();
+            return field;
+          }
+        }
+      })
+    })
+    test('field method needs to return a value of the correct type', () => {
+      const cache = new TestCache();
+      cache.modify<{ p1: string, p2: string, p3: string, p4: string, p5: string }>({
+        fields: {
+          p1() { return "" },
+          // @ts-expect-error returns wrong type
+          p2() { return 1 },
+          // @ts-expect-error needs return statement
+          p3() {},
+          p4(_, { DELETE }) { return DELETE },
+          p5(_, { INVALIDATE }) { return INVALIDATE },
+        }
+      })
+    })
+    test('passing a function as `field` should infer all entity properties as possible input (interfaces)', () => {
+      interface ParentEntity {
+        prop1: string;
+        prop2: number;
+        child: ChildEntity;
+      }
+      interface ChildEntity {
+        prop1: boolean;
+        prop2: symbol;
+        children: OtherChildEntry[];
+      }
+      interface OtherChildEntry {
+        foo: false
+      }
+
+      const cache = new TestCache();
+      // with reference
+      cache.modify<ParentEntity>({
+        id: 'foo',
+        fields(field) {
+          expectTypeOf(field).toEqualTypeOf<string | number | ChildEntity | Reference>();
+          return field;
+        }
+      })
+      // without reference
+      cache.modify<ChildEntity>({
+        id: 'foo',
+        fields(field) {
+          expectTypeOf(field).toEqualTypeOf<boolean | symbol | readonly OtherChildEntry[] | readonly Reference[]>();
+          return field;
+        }
+      })
+    })
+    test('passing a function as `field` should infer all entity properties as possible input (types)', () => {
+      type ParentEntity = {
+        prop1: string;
+        prop2: number;
+        child: ChildEntity;
+      }
+      type ChildEntity = {
+        prop1: boolean;
+        prop2: symbol;
+        children: OtherChildEntry[];
+      }
+      type OtherChildEntry = {
+        foo: false
+      }
+
+      const cache = new TestCache();
+      // with reference
+      cache.modify<ParentEntity>({
+        id: 'foo',
+        fields(field) {
+          expectTypeOf(field).toEqualTypeOf<string | number | ChildEntity | Reference>();
+          return field;
+        }
+      })
+      // without reference
+      cache.modify<ChildEntity>({
+        id: 'foo',
+        fields(field) {
+          expectTypeOf(field).toEqualTypeOf<boolean | symbol | readonly OtherChildEntry[] | readonly Reference[]>();
+          return field;
+        }
+      })
+    })
+    test('passing a function as `field` w/o specifying an entity type', () => {
+      const cache = new TestCache();
+      cache.modify({
+        id: 'foo',
+        fields(field) {
+          expectTypeOf(field).toEqualTypeOf<any>();
+          return field;
+        }
+      });
+    });
+    test('passing a function as `field` property w/o specifying an entity type', () => {
+      const cache = new TestCache();
+      cache.modify({
+        id: 'foo',
+        fields: {
+          p1(field) {
+            expectTypeOf(field).toEqualTypeOf<any>();
+            return field;
+          }
+        }
+      });
+    });
+  });
+});

src/cache/core/cache.ts

@@ -120,7 +120,7 @@ export abstract class ApolloCache<TSerialized> implements DataProxy {
     return [];
   }
 
-  public modify(options: Cache.ModifyOptions): boolean {
+  public modify<Entity extends Record<string, any> = Record<string, any>>(options: Cache.ModifyOptions<Entity>): boolean {
     return false;
   }
 

src/cache/core/types/Cache.ts

@@ -1,5 +1,5 @@
 import { DataProxy } from './DataProxy';
-import type { Modifier, Modifiers } from './common';
+import type { AllFieldsModifier, Modifiers } from './common';;
 import type { ApolloCache } from '../cache';
 
 export namespace Cache {
@@ -57,9 +57,9 @@ export namespace Cache {
     discardWatches?: boolean;
   }
 
-  export interface ModifyOptions {
+  export interface ModifyOptions<Entity extends Record<string, any> = Record<string, any>> {
     id?: string;
-    fields: Modifiers | Modifier<any>;
+    fields: Modifiers<Entity> | AllFieldsModifier<Entity>;
     optimistic?: boolean;
     broadcast?: boolean;
   }

src/cache/core/types/common.ts

@@ -76,9 +76,14 @@ export type ToReferenceFunction = (
 
 export type CanReadFunction = (value: StoreValue) => boolean;
 
+declare const _deleteModifier: unique symbol;
+export interface DeleteModifier { [_deleteModifier]: true }
+declare const _invalidateModifier: unique symbol;
+export interface InvalidateModifier { [_invalidateModifier]: true}
+
 export type ModifierDetails = {
-  DELETE: any;
-  INVALIDATE: any;
+  DELETE: DeleteModifier;
+  INVALIDATE: InvalidateModifier;
   fieldName: string;
   storeFieldName: string;
   readField: ReadFieldFunction;
@@ -88,8 +93,28 @@ export type ModifierDetails = {
   storage: StorageType;
 }
 
-export type Modifier<T> = (value: T, details: ModifierDetails) => T;
-
-export type Modifiers = {
-  [fieldName: string]: Modifier<any>;
-};
+export type Modifier<T> = (
+  value: T,
+  details: ModifierDetails
+) => T | DeleteModifier | InvalidateModifier;
+
+type StoreObjectValueMaybeReference<StoreVal> = 
+  StoreVal extends Record<string, any>[]
+  ? Readonly<StoreVal> | readonly Reference[]
+  : StoreVal extends Record<string, any>
+  ? StoreVal | Reference
+  : StoreVal;
+
+export type AllFieldsModifier<
+  Entity extends Record<string, any>
+> = Modifier<Entity[keyof Entity] extends infer Value ? 
+  StoreObjectValueMaybeReference<Exclude<Value, undefined>>
+  : never>;
+
+export type Modifiers<
+  T extends Record<string, any> = Record<string, unknown>
+> = Partial<{
+  [FieldName in keyof T]: Modifier<
+    StoreObjectValueMaybeReference<Exclude<T[FieldName], undefined>>
+  >;
+}>;