From 995e8553983a8053601ebeb283347b6d5c190c07 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 11 Oct 2019 17:19:30 -0400 Subject: [PATCH] Unify Field{Read,Merge}Options as much as possible, for consistency. Although it was tempting to make the read function feel like a GraphQL resolver function (parentObject, args, context, info), that signature would not have been appropriate for the merge function, and having to remember two totally different signatures for reading and merging did not seem ideal. Now, the only difference between the signatures is that the merge function takes incoming data, to be merged with the existing data (if any): read(existing, { args, parentObject, field, variables }) merge(existing, incoming, { args, parentObject, field, variables }) It would have been nice to use named options for everything, including the existing and incoming parameters, but it's important for those parameters to be positional so that the developer can specify their types, without also having to provide a new type for the entire options object. --- .../inmemory/__tests__/diffAgainstStore.ts | 2 +- src/cache/inmemory/__tests__/policies.ts | 18 ++-- src/cache/inmemory/policies.ts | 102 ++++++++++-------- src/cache/inmemory/readFromStore.ts | 13 +-- src/cache/inmemory/writeToStore.ts | 9 +- src/core/__tests__/QueryManager/links.ts | 2 +- 6 files changed, 72 insertions(+), 74 deletions(-) diff --git a/src/cache/inmemory/__tests__/diffAgainstStore.ts b/src/cache/inmemory/__tests__/diffAgainstStore.ts index 341deb17525..1d5507e78d9 100644 --- a/src/cache/inmemory/__tests__/diffAgainstStore.ts +++ b/src/cache/inmemory/__tests__/diffAgainstStore.ts @@ -959,7 +959,7 @@ describe('diffing queries against the store', () => { typePolicies: { Query: { fields: { - person(rootQuery, args) { + person(_, { args, parentObject: rootQuery }) { expect(typeof args.id).toBe('number'); const id = this.identify({ __typename: 'Person', id: args.id }); expect(id).toBe(`Person:${JSON.stringify({ id: args.id })}`); diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts index 859c8bba4a7..196e0d5abf9 100644 --- a/src/cache/inmemory/__tests__/policies.ts +++ b/src/cache/inmemory/__tests__/policies.ts @@ -1,6 +1,7 @@ import gql from "graphql-tag"; import { InMemoryCache } from "../inMemoryCache"; import { StoreValue } from "../../../utilities"; +import { FieldPolicy } from "../policies"; describe("type policies", function () { const bookQuery = gql` @@ -396,7 +397,7 @@ describe("type policies", function () { Person: { keyFields: ["firstName", "lastName"], fields: { - fullName(person) { + fullName(_, { parentObject: person }) { return `${person.firstName} ${person.lastName}`; }, }, @@ -534,23 +535,20 @@ describe("type policies", function () { todos: { keyArgs: [], - read(person, args, { storeFieldValue }) { - return (storeFieldValue as any[]).slice( + read(existing: any[], { args }) { + return existing.slice( args.offset, args.offset + args.limit, ); }, - merge({ - args, - existingValue = [], - incomingValue, - }): StoreValue[] { + merge(existing: any[], incoming: any[], { args }) { + const copy = existing ? existing.slice(0) : []; const limit = args.offset + args.limit; for (let i = args.offset; i < limit; ++i) { - existingValue[i] = incomingValue[i - args.offset]; + copy[i] = incoming[i - args.offset]; } - return existingValue as any; + return copy; } }, }, diff --git a/src/cache/inmemory/policies.ts b/src/cache/inmemory/policies.ts index 9f13240bc9f..1befc62534b 100644 --- a/src/cache/inmemory/policies.ts +++ b/src/cache/inmemory/policies.ts @@ -62,7 +62,9 @@ type TypePolicy = { subscriptionType?: true, fields?: { - [fieldName: string]: FieldPolicy | FieldReadFunction; + [fieldName: string]: + | FieldPolicy + | FieldReadFunction; } }; @@ -75,29 +77,32 @@ type KeyArgsFunction = ( }, ) => ReturnType; -type FieldPolicy = { +export type FieldPolicy = { keyArgs?: KeySpecifier | KeyArgsFunction; read?: FieldReadFunction; merge?: FieldMergeFunction; }; -interface CommonFieldFunctionOptions { - typename: string; +interface FieldFunctionOptions { + args: Record; + parentObject: Readonly; field: FieldNode; - variables: Record; -} - -interface FieldReadOptions extends CommonFieldFunctionOptions { - storeFieldName: string; - storeFieldValue: Readonly; + variables?: Record; } -interface FieldReadFunction { +interface FieldReadFunction { (this: Policies, - storeObject: Readonly, - args: Record, - options: FieldReadOptions, - ): StoreValue; + // When reading a field, one often needs to know about any existing + // value stored for that field. If the field is read before any value + // has been written to the cache, this existing parameter will be + // undefined, which makes it easy to use a default parameter expression + // to supply the initial value. This parameter is positional (rather + // than one of the named options) because that makes it possible for + // the developer to annotate it with a type, without also having to + // provide a whole new type for the options object. + existing: Readonly | undefined, + options: FieldFunctionOptions, + ): TResult; // The TypeScript typings for Function.prototype.call are much too generic // to enforce the type safety we need here, for reasons discussed in this @@ -107,21 +112,26 @@ interface FieldReadFunction { // https://github.com/microsoft/TypeScript/pull/27028 call( self: Policies, - storeObject: Readonly, - args: Record, - options: FieldReadOptions, - ): StoreValue; + existing: Readonly | undefined, + options: FieldFunctionOptions, + ): TResult; } -interface FieldMergeOptions extends CommonFieldFunctionOptions { - incomingValue: Readonly; - existingValue?: Readonly; - args: Record; -} +interface FieldMergeFunction { + (this: Policies, + existing: Readonly | undefined, + // The incoming parameter needs to be positional as well, for the same + // reasons discussed in FieldReadFunction above. + incoming: Readonly, + options: FieldFunctionOptions, + ): TExisting; -interface FieldMergeFunction { - (this: Policies, options: FieldMergeOptions): TValue; - call(self: Policies, options: FieldMergeOptions): TValue; + call( + self: Policies, + existing: Readonly | undefined, + incoming: Readonly, + options: FieldFunctionOptions, + ): TExisting; } export function defaultDataIdFromObject(object: StoreObject) { @@ -358,46 +368,43 @@ export class Policies { } public readFieldFromStoreObject( - typename: string, - storeObject: Readonly, + parentObject: Readonly, field: FieldNode, - variables: Record, + typename = parentObject.__typename, + variables?: Record, ): StoreValue { const storeFieldName = this.getStoreFieldName(typename, field, variables); - const storeFieldValue = storeObject[storeFieldName]; + const existing = parentObject[storeFieldName]; const policy = this.getFieldPolicy(typename, field.name.value, false); if (policy && policy.read) { - // TODO Avoid recomputing this. - const args = argumentsObjectFromField(field, variables); - return policy.read.call(this, storeObject, args, { - typename, + return policy.read.call(this, existing, { + // TODO Avoid recomputing this. + args: argumentsObjectFromField(field, variables), + parentObject, field, variables, - storeFieldName, - storeFieldValue, }); } - return storeFieldValue; + return existing; } public getFieldMergeFunction( typename: string, field: FieldNode, - variables: Record, + variables?: Record, ): StoreValueMergeFunction { const policy = this.getFieldPolicy(typename, field.name.value, false); if (policy && policy.merge) { return ( - existingValue: StoreValue, - incomingValue: StoreValue, - ) => policy.merge.call(this, { - typename, - field, - variables, - incomingValue, - existingValue, + existing: StoreValue, + incoming: StoreValue, + parentObject: Readonly, + ) => policy.merge.call(this, existing, incoming, { // TODO Avoid recomputing this. args: argumentsObjectFromField(field, variables), + parentObject, + field, + variables, }); } } @@ -406,6 +413,7 @@ export class Policies { export type StoreValueMergeFunction = ( existing: StoreValue, incoming: StoreValue, + parentObject: Readonly, ) => StoreValue; function keyArgsFnFromSpecifier( diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index bdd55216bdf..8600083b87b 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -12,7 +12,6 @@ import { isField, isInlineFragment, resultKeyNameFromField, - StoreValue, Reference, isReference, makeReference, @@ -336,16 +335,8 @@ export class StoreReader { policies, } = context; - let fieldValue: StoreValue | undefined; - - if (object) { - fieldValue = policies.readFieldFromStoreObject( - typename, - object, - field, - variables, - ); - } + const fieldValue = object && + policies.readFieldFromStoreObject(object, field, typename, variables); const readStoreResult = typeof fieldValue === "undefined" ? { result: fieldValue, diff --git a/src/cache/inmemory/writeToStore.ts b/src/cache/inmemory/writeToStore.ts index a6d22c57eda..eb6edce1636 100644 --- a/src/cache/inmemory/writeToStore.ts +++ b/src/cache/inmemory/writeToStore.ts @@ -368,10 +368,11 @@ function walkWithMergeOverrides( const existingValue: any = existingObject && existingObject[name]; const incomingValue: any = incomingObject && incomingObject[name]; if (typeof override === "function") { - return incomingObject[name] = override(existingValue, incomingValue); - } - if (override && typeof override === "object") { - return walkWithMergeOverrides(existingValue, incomingValue, override); + incomingObject[name] = override(existingValue, incomingValue, existingObject); + } else if (override && typeof override === "object") { + // StoreObjects can have multiple layers of nested objects/arrays, + // each layer with its own nested fields and override functions. + walkWithMergeOverrides(existingValue, incomingValue, override); } }); } diff --git a/src/core/__tests__/QueryManager/links.ts b/src/core/__tests__/QueryManager/links.ts index 36f88683e33..ea2a8bee20b 100644 --- a/src/core/__tests__/QueryManager/links.ts +++ b/src/core/__tests__/QueryManager/links.ts @@ -330,7 +330,7 @@ describe('Link interactions', () => { typePolicies: { Query: { fields: { - book(rootQuery, args) { + book(_, { parentObject: rootQuery, args }) { const id = this.identify({ __typename: "Book", id: args.id }); expect(id).toEqual(`Book:${args.id}`); const found = (rootQuery.books as Reference[]).find(