code review adjustments

apollographql · Jun 12, 2023 · 6973cd4 · 6973cd4
1 parent 014f17d
commit 6973cd4
Show file tree

Hide file tree

Showing 6 changed files with 159 additions and 17 deletions.
diff --git a/src/cache/core/__tests__/cache.ts b/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;
+          }
+        }
+      });
+    });
+  });
+});
diff --git a/src/cache/core/cache.ts b/src/cache/core/cache.ts
@@ -120,7 +120,7 @@ export abstract class ApolloCache<TSerialized> implements DataProxy {
     return [];
   }
 
-  public modify<Entity = Record<string, any>>(options: Cache.ModifyOptions<Entity>): boolean {
+  public modify<Entity extends Record<string, any> = Record<string, any>>(options: Cache.ModifyOptions<Entity>): boolean {
     return false;
   }
 

diff --git a/src/cache/core/types/Cache.ts b/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,11 +57,9 @@ export namespace Cache {
     discardWatches?: boolean;
   }
 
-  export interface ModifyOptions<Entity = Record<string, any>> {
+  export interface ModifyOptions<Entity extends Record<string, any> = Record<string, any>> {
     id?: string;
-    fields: Entity extends Record<string, unknown>
-      ? Modifiers<Entity> | Modifier<Entity>
-      : Modifier<Entity>;
+    fields: Modifiers<Entity> | AllFieldsModifier<Entity>;
     optimistic?: boolean;
     broadcast?: boolean;
   }

diff --git a/src/cache/core/types/common.ts b/src/cache/core/types/common.ts
@@ -98,17 +98,21 @@ export type Modifier<T> = (
   details: ModifierDetails
 ) => T | DeleteModifier | InvalidateModifier;
 
-type StoreObjectValueMaybeReference<StoreVal> = StoreVal extends Record<
-  string,
-  unknown
->[]
-  ? StoreVal | Reference[]
-  : StoreVal extends Record<string, unknown>
+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, unknown> = Record<string, unknown>
+  T extends Record<string, any> = Record<string, unknown>
 > = Partial<{
   [FieldName in keyof T]: Modifier<
     StoreObjectValueMaybeReference<Exclude<T[FieldName], undefined>>

diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts
@@ -2,7 +2,7 @@ import gql, { disableFragmentWarnings } from 'graphql-tag';
 
 import { cloneDeep } from '../../../utilities/common/cloneDeep';
 import { makeReference, Reference, makeVar, TypedDocumentNode, isReference, DocumentNode } from '../../../core';
-import { Cache, Modifiers } from '../../../cache';
+import { Cache } from '../../../cache';
 import { InMemoryCache } from '../inMemoryCache';
 import { InMemoryCacheConfig } from '../types';
 
@@ -2817,7 +2817,7 @@ describe("InMemoryCache#modify", () => {
 
     cache.modify({
       fields: {
-        comments(comments: Reference[], { readField }) {
+        comments(comments: readonly Reference[], { readField }) {
           expect(Object.isFrozen(comments)).toBe(true);
           expect(comments.length).toBe(3);
           const filtered = comments.filter(comment => {
@@ -2902,6 +2902,7 @@ describe("InMemoryCache#modify", () => {
         expect(fieldName).not.toBe("b");
         if (fieldName === "a") expect(value).toBe(1);
         if (fieldName === "c") expect(value).toBe(3);
+        return value;
       },
       optimistic: true,
     });

diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts
@@ -214,7 +214,7 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
     }
   }
 
-  public modify<Entity = Record<string, any>>(options: Cache.ModifyOptions<Entity>): boolean {
+  public modify<Entity extends Record<string, any> = Record<string, any>>(options: Cache.ModifyOptions<Entity>): boolean {
     if (hasOwn.call(options, "id") && !options.id) {
       // To my knowledge, TypeScript does not currently provide a way to
       // enforce that an optional property?:type must *not* be undefined