From e187866fdfbbd1e1e30646f289367fb4b5afb3c3 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Tue, 13 Jun 2023 17:34:21 +0200 Subject: [PATCH] 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` 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` 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 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 --- .changeset/afraid-zebras-punch.md | 24 +++++ .changeset/cold-tips-accept.md | 9 ++ src/cache/core/__tests__/cache.ts | 141 +++++++++++++++++++++++++- src/cache/core/cache.ts | 2 +- src/cache/core/types/Cache.ts | 6 +- src/cache/core/types/common.ts | 39 +++++-- src/cache/inmemory/__tests__/cache.ts | 104 +++++++++++++------ src/cache/inmemory/entityStore.ts | 13 ++- src/cache/inmemory/inMemoryCache.ts | 2 +- src/cache/inmemory/types.ts | 4 +- 10 files changed, 295 insertions(+), 49 deletions(-) create mode 100644 .changeset/afraid-zebras-punch.md create mode 100644 .changeset/cold-tips-accept.md diff --git a/.changeset/afraid-zebras-punch.md b/.changeset/afraid-zebras-punch.md new file mode 100644 index 00000000000..8869d5b5934 --- /dev/null +++ b/.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({ + 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`. + }, + }, +}); +``` diff --git a/.changeset/cold-tips-accept.md b/.changeset/cold-tips-accept.md new file mode 100644 index 00000000000..0e8775403a9 --- /dev/null +++ b/.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/). diff --git a/src/cache/core/__tests__/cache.ts b/src/cache/core/__tests__/cache.ts index 5f07bb28e97..48660ffb715 100644 --- 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 { 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(); + return field; + }, + prop2(field) { + expectTypeOf(field).toEqualTypeOf(); + return field; + }, + child(field) { + expectTypeOf(field).toEqualTypeOf<{ someObject: true } | Reference>(); + return field; + }, + children(field) { + expectTypeOf(field).toEqualTypeOf<(ReadonlyArray<{ anotherObject: false }>) | ReadonlyArray>(); + 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({ + id: 'foo', + fields(field) { + expectTypeOf(field).toEqualTypeOf(); + return field; + } + }) + // without reference + cache.modify({ + id: 'foo', + fields(field) { + expectTypeOf(field).toEqualTypeOf(); + 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({ + id: 'foo', + fields(field) { + expectTypeOf(field).toEqualTypeOf(); + return field; + } + }) + // without reference + cache.modify({ + id: 'foo', + fields(field) { + expectTypeOf(field).toEqualTypeOf(); + 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(); + 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(); + return field; + } + } + }); + }); + }); +}); diff --git a/src/cache/core/cache.ts b/src/cache/core/cache.ts index 30069aa89be..e48f12ccaec 100644 --- a/src/cache/core/cache.ts +++ b/src/cache/core/cache.ts @@ -120,7 +120,7 @@ export abstract class ApolloCache implements DataProxy { return []; } - public modify(options: Cache.ModifyOptions): boolean { + public modify = Record>(options: Cache.ModifyOptions): boolean { return false; } diff --git a/src/cache/core/types/Cache.ts b/src/cache/core/types/Cache.ts index cb89169930b..47504c287c7 100644 --- 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,9 +57,9 @@ export namespace Cache { discardWatches?: boolean; } - export interface ModifyOptions { + export interface ModifyOptions = Record> { id?: string; - fields: Modifiers | Modifier; + fields: Modifiers | AllFieldsModifier; optimistic?: boolean; broadcast?: boolean; } diff --git a/src/cache/core/types/common.ts b/src/cache/core/types/common.ts index 4e45b84ad82..3a1cdcf4df6 100644 --- a/src/cache/core/types/common.ts +++ b/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 = (value: T, details: ModifierDetails) => T; - -export type Modifiers = { - [fieldName: string]: Modifier; -}; +export type Modifier = ( + value: T, + details: ModifierDetails +) => T | DeleteModifier | InvalidateModifier; + +type StoreObjectValueMaybeReference = + StoreVal extends Record[] + ? Readonly | readonly Reference[] + : StoreVal extends Record + ? StoreVal | Reference + : StoreVal; + +export type AllFieldsModifier< + Entity extends Record +> = Modifier> + : never>; + +export type Modifiers< + T extends Record = Record +> = Partial<{ + [FieldName in keyof T]: Modifier< + StoreObjectValueMaybeReference> + >; +}>; diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts index 470247db539..cfc0d86afd8 100644 --- a/src/cache/inmemory/__tests__/cache.ts +++ b/src/cache/inmemory/__tests__/cache.ts @@ -1,4 +1,5 @@ import gql, { disableFragmentWarnings } from 'graphql-tag'; +import { expectTypeOf } from 'expect-type' import { cloneDeep } from '../../../utilities/common/cloneDeep'; import { makeReference, Reference, makeVar, TypedDocumentNode, isReference, DocumentNode } from '../../../core'; @@ -2817,7 +2818,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 +2903,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, }); @@ -3905,18 +3907,43 @@ describe('TypedDocumentNode', () => { } `; - it('should determine Data and Variables types of {write,read}{Query,Fragment}', () => { - const cache = new InMemoryCache({ + // We need to define these objects separately from calling writeQuery, + // because passing them directly to writeQuery will trigger excess property + // warnings due to the extra __typename and isbn fields. Internally, we + // almost never pass object literals to writeQuery or writeFragment, so + // excess property checks should not be a problem in practice. + const jcmAuthor = { + __typename: "Author", + name: "John C. Mitchell", + }; + + const ffplBook = { + __typename: "Book", + isbn: "0262133210", + title: "Foundations for Programming Languages", + author: jcmAuthor, + }; + + const ffplVariables = { + isbn: "0262133210", + }; + + function getBookCache() { + return new InMemoryCache({ typePolicies: { Query: { fields: { book(existing, { args, toReference }) { - return existing ?? (args && toReference({ - __typename: "Book", - isbn: args.isbn, - })); - } - } + return ( + existing ?? + (args && + toReference({ + __typename: "Book", + isbn: args.isbn, + })) + ); + }, + }, }, Book: { @@ -3928,27 +3955,10 @@ describe('TypedDocumentNode', () => { }, }, }); + } - // We need to define these objects separately from calling writeQuery, - // because passing them directly to writeQuery will trigger excess property - // warnings due to the extra __typename and isbn fields. Internally, we - // almost never pass object literals to writeQuery or writeFragment, so - // excess property checks should not be a problem in practice. - const jcmAuthor = { - __typename: "Author", - name: "John C. Mitchell", - }; - - const ffplBook = { - __typename: "Book", - isbn: "0262133210", - title: "Foundations for Programming Languages", - author: jcmAuthor, - }; - - const ffplVariables = { - isbn: "0262133210", - }; + it("should determine Data and Variables types of {write,read}{Query,Fragment}", () => { + const cache = getBookCache(); cache.writeQuery({ query, @@ -4028,4 +4038,40 @@ describe('TypedDocumentNode', () => { }, }); }); + + it.skip("should infer the types of modifier fields", () => { + const cache = getBookCache(); + + cache.writeQuery({ + query, + variables: ffplVariables, + data: { + book: ffplBook, + }, + }); + + cache.modify({ + id: cache.identify(ffplBook), + fields: { + isbn: (value) => { + expectTypeOf(value).toEqualTypeOf(); + return value; + }, + title: (value, { INVALIDATE }) => { + expectTypeOf(value).toEqualTypeOf(); + return INVALIDATE; + }, + author: (value, { DELETE, isReference }) => { + expectTypeOf(value).toEqualTypeOf(); + if (isReference(value)) { + expectTypeOf(value).toEqualTypeOf(); + } else { + expectTypeOf(value).toEqualTypeOf(); + } + + return DELETE; + }, + }, + }); + }); }); diff --git a/src/cache/inmemory/entityStore.ts b/src/cache/inmemory/entityStore.ts index 9631ca615ef..a7dd96be059 100644 --- a/src/cache/inmemory/entityStore.ts +++ b/src/cache/inmemory/entityStore.ts @@ -27,11 +27,14 @@ import type { ReadFieldOptions, ToReferenceFunction, CanReadFunction, + InvalidateModifier, + DeleteModifier, + ModifierDetails, } from '../core/types/common'; -const DELETE: any = Object.create(null); +const DELETE: DeleteModifier = Object.create(null); const delModifier: Modifier = () => DELETE; -const INVALIDATE: any = Object.create(null); +const INVALIDATE: InvalidateModifier = Object.create(null); export abstract class EntityStore implements NormalizedCache { protected data: NormalizedCacheObject = Object.create(null); @@ -192,7 +195,7 @@ export abstract class EntityStore implements NormalizedCache { public modify( dataId: string, - fields: Modifier | Modifiers, + fields: Modifier | Modifiers>, ): boolean { const storeObject = this.lookup(dataId); @@ -217,13 +220,13 @@ export abstract class EntityStore implements NormalizedCache { } : fieldNameOrOptions, { store: this }, ), - }; + } satisfies Partial; Object.keys(storeObject).forEach(storeFieldName => { const fieldName = fieldNameFromStoreName(storeFieldName); let fieldValue = storeObject[storeFieldName]; if (fieldValue === void 0) return; - const modify: Modifier = typeof fields === "function" + const modify: Modifier | undefined = typeof fields === "function" ? fields : fields[storeFieldName] || fields[fieldName]; if (modify) { diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index aa4d42713f3..49b33c59397 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -214,7 +214,7 @@ export class InMemoryCache extends ApolloCache { } } - public modify(options: Cache.ModifyOptions): boolean { + public modify = Record>(options: Cache.ModifyOptions): 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 diff --git a/src/cache/inmemory/types.ts b/src/cache/inmemory/types.ts index 31a06caa20a..286649c8239 100644 --- a/src/cache/inmemory/types.ts +++ b/src/cache/inmemory/types.ts @@ -15,10 +15,10 @@ import type { FieldMergeFunction, } from './policies'; import type { - Modifier, Modifiers, ToReferenceFunction, CanReadFunction, + AllFieldsModifier, } from '../core/types/common'; import type { FragmentRegistryAPI } from './fragmentRegistry'; @@ -49,7 +49,7 @@ export interface NormalizedCache { merge(olderId: string, newerObject: StoreObject): void; merge(olderObject: StoreObject, newerId: string): void; - modify(dataId: string, fields: Modifiers | Modifier): boolean; + modify>(dataId: string, fields: Modifiers | AllFieldsModifier): boolean; delete(dataId: string, fieldName?: string): boolean; clear(): void;