From f8b01c04d9df3c34c46bb1dee209a40169258b0c Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 16 Jul 2021 15:32:50 -0400 Subject: [PATCH 1/2] Default to current object for `readField` only when `from` omitted. Should help with #8499. --- src/__tests__/__snapshots__/exports.ts.snap | 3 + .../__tests__/__snapshots__/policies.ts.snap | 23 +++++++ src/cache/inmemory/__tests__/policies.ts | 66 ++++++++++++++++++- src/cache/inmemory/policies.ts | 35 ++++++++-- src/utilities/testing/index.ts | 3 +- src/utilities/testing/mocking/mockLink.ts | 12 ++-- src/utilities/testing/stringifyForDisplay.ts | 8 +++ src/utilities/testing/withConsoleSpy.ts | 49 ++++++++++++++ src/utilities/testing/withErrorSpy.ts | 21 ------ 9 files changed, 183 insertions(+), 37 deletions(-) create mode 100644 src/utilities/testing/stringifyForDisplay.ts create mode 100644 src/utilities/testing/withConsoleSpy.ts delete mode 100644 src/utilities/testing/withErrorSpy.ts diff --git a/src/__tests__/__snapshots__/exports.ts.snap b/src/__tests__/__snapshots__/exports.ts.snap index f43c18e7b25..90edc0545ba 100644 --- a/src/__tests__/__snapshots__/exports.ts.snap +++ b/src/__tests__/__snapshots__/exports.ts.snap @@ -312,9 +312,12 @@ Array [ "itAsync", "mockObservableLink", "mockSingleLink", + "stringifyForDisplay", "stripSymbols", "subscribeAndCount", "withErrorSpy", + "withLogSpy", + "withWarningSpy", ] `; diff --git a/src/cache/inmemory/__tests__/__snapshots__/policies.ts.snap b/src/cache/inmemory/__tests__/__snapshots__/policies.ts.snap index d414466e566..7db3045a6a0 100644 --- a/src/cache/inmemory/__tests__/__snapshots__/policies.ts.snap +++ b/src/cache/inmemory/__tests__/__snapshots__/policies.ts.snap @@ -1389,6 +1389,29 @@ exports[`type policies field policies runs nested merge functions as well as anc } `; +exports[`type policies readField warns if explicitly passed undefined \`from\` option 1`] = ` +[MockFunction] { + "calls": Array [ + Array [ + "Undefined 'from' passed to readField with arguments [{\\"fieldName\\":\\"firstName\\",\\"from\\":}]", + ], + Array [ + "Undefined 'from' passed to readField with arguments [\\"lastName\\",]", + ], + ], + "results": Array [ + Object { + "type": "return", + "value": undefined, + }, + Object { + "type": "return", + "value": undefined, + }, + ], +} +`; + exports[`type policies support inheritance 1`] = ` Object { "Cobra:{\\"tagId\\":\\"Egypt30BC\\"}": Object { diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts index 3fe2c070e51..6ba256d2d7e 100644 --- a/src/cache/inmemory/__tests__/policies.ts +++ b/src/cache/inmemory/__tests__/policies.ts @@ -9,7 +9,7 @@ import { MockLink } from '../../../utilities/testing/mocking/mockLink'; import subscribeAndCount from '../../../utilities/testing/subscribeAndCount'; import { itAsync } from '../../../utilities/testing/itAsync'; import { FieldPolicy, StorageType } from "../policies"; -import { withErrorSpy } from "../../../testing"; +import { withErrorSpy, withWarningSpy } from "../../../testing"; function reverse(s: string) { return s.split("").reverse().join(""); @@ -5051,6 +5051,70 @@ describe("type policies", function () { }); }); + withWarningSpy(it, "readField warns if explicitly passed undefined `from` option", function () { + const cache = new InMemoryCache({ + typePolicies: { + Query: { + fields: { + fullNameWithDefaults(_, { readField }) { + return `${ + readField({ + fieldName: "firstName", + }) + } ${ + readField("lastName") + }`; + }, + + fullNameWithVoids(_, { readField }) { + return `${ + readField({ + fieldName: "firstName", + // If options.from is explicitly passed but undefined, + // readField should not default to reading from the current + // object (see issue #8499). + from: void 0, + }) + } ${ + // Likewise for the shorthand version of readField. + readField("lastName", void 0) + }`; + }, + }, + }, + }, + }); + + const firstNameLastNameQuery = gql` + query { + firstName + lastName + } + `; + + const fullNamesQuery = gql` + query { + fullNameWithVoids + fullNameWithDefaults + } + `; + + cache.writeQuery({ + query: firstNameLastNameQuery, + data: { + firstName: "Alan", + lastName: "Turing", + }, + }); + + expect(cache.readQuery({ + query: fullNamesQuery, + })).toEqual({ + fullNameWithDefaults: "Alan Turing", + fullNameWithVoids: "undefined undefined", + }); + }); + it("can return existing object from merge function (issue #6245)", function () { const cache = new InMemoryCache({ typePolicies: { diff --git a/src/cache/inmemory/policies.ts b/src/cache/inmemory/policies.ts index f32c906ef2a..3a623aa3541 100644 --- a/src/cache/inmemory/policies.ts +++ b/src/cache/inmemory/policies.ts @@ -52,6 +52,7 @@ import { WriteContext } from './writeToStore'; // used by getStoreKeyName. This function is used when computing storeFieldName // strings (when no keyArgs has been configured for a field). import { canonicalStringify } from './object-canon'; +import { stringifyForDisplay } from '../../testing'; getStoreKeyName.setStringify(canonicalStringify); export type TypePolicies = { @@ -882,14 +883,36 @@ function makeFieldFunctionOptions( fieldNameOrOptions: string | ReadFieldOptions, from?: StoreObject | Reference, ) { - const options: ReadFieldOptions = - typeof fieldNameOrOptions === "string" ? { + let options: ReadFieldOptions; + if (typeof fieldNameOrOptions === "string") { + options = { fieldName: fieldNameOrOptions, - from, - } : { ...fieldNameOrOptions }; + // Default to objectOrReference only when no second argument was + // passed for the from parameter, not when undefined is explicitly + // passed as the second argument. + from: arguments.length > 1 ? from : objectOrReference, + }; + } else if (isNonNullObject(fieldNameOrOptions)) { + options = { ...fieldNameOrOptions }; + // Default to objectOrReference only when fieldNameOrOptions.from is + // actually omitted, rather than just undefined. + if (!hasOwn.call(fieldNameOrOptions, "from")) { + options.from = objectOrReference; + } + } else { + invariant.warn(`Unexpected readField arguments: ${ + stringifyForDisplay(Array.from(arguments)) + }`); + // The readField helper function returns undefined for any missing + // fields, so it should also return undefined if the arguments were not + // of a type we expected. + return; + } - if (void 0 === options.from) { - options.from = objectOrReference; + if (__DEV__ && options.from === void 0) { + invariant.warn(`Undefined 'from' passed to readField with arguments ${ + stringifyForDisplay(Array.from(arguments)) + }`); } if (void 0 === options.variables) { diff --git a/src/utilities/testing/index.ts b/src/utilities/testing/index.ts index 668557783e9..c0e5c1daa51 100644 --- a/src/utilities/testing/index.ts +++ b/src/utilities/testing/index.ts @@ -13,4 +13,5 @@ export { createMockClient } from './mocking/mockClient'; export { stripSymbols } from './stripSymbols'; export { default as subscribeAndCount } from './subscribeAndCount'; export { itAsync } from './itAsync'; -export { withErrorSpy } from './withErrorSpy'; +export * from './withConsoleSpy'; +export * from './stringifyForDisplay'; diff --git a/src/utilities/testing/mocking/mockLink.ts b/src/utilities/testing/mocking/mockLink.ts index 7b3a24b49ba..4812177b4b7 100644 --- a/src/utilities/testing/mocking/mockLink.ts +++ b/src/utilities/testing/mocking/mockLink.ts @@ -15,17 +15,13 @@ import { removeClientSetsFromDocument, removeConnectionDirectiveFromDocument, cloneDeep, - makeUniqueId, } from '../../../utilities'; -export type ResultFunction = () => T; +import { + stringifyForDisplay, +} from '../../../testing'; -function stringifyForDisplay(value: any): string { - const undefId = makeUniqueId("stringifyForDisplay"); - return JSON.stringify(value, (key, value) => { - return value === void 0 ? undefId : value; - }).split(JSON.stringify(undefId)).join(""); -} +export type ResultFunction = () => T; export interface MockedResponse> { request: GraphQLRequest; diff --git a/src/utilities/testing/stringifyForDisplay.ts b/src/utilities/testing/stringifyForDisplay.ts new file mode 100644 index 00000000000..4eb8a724238 --- /dev/null +++ b/src/utilities/testing/stringifyForDisplay.ts @@ -0,0 +1,8 @@ +import { makeUniqueId } from "../common/makeUniqueId"; + +export function stringifyForDisplay(value: any): string { + const undefId = makeUniqueId("stringifyForDisplay"); + return JSON.stringify(value, (key, value) => { + return value === void 0 ? undefId : value; + }).split(JSON.stringify(undefId)).join(""); +} diff --git a/src/utilities/testing/withConsoleSpy.ts b/src/utilities/testing/withConsoleSpy.ts new file mode 100644 index 00000000000..171a6eac2a5 --- /dev/null +++ b/src/utilities/testing/withConsoleSpy.ts @@ -0,0 +1,49 @@ +function wrapTestFunction( + fn: (...args: any[]) => any, + consoleMethodName: "log" | "warn" | "error", +) { + return function () { + const args = arguments; + const spy = jest.spyOn(console, consoleMethodName); + spy.mockImplementation(() => {}); + return new Promise(resolve => { + resolve(fn?.apply(this, args)); + }).finally(() => { + expect(spy).toMatchSnapshot(); + spy.mockReset(); + }); + }; +} + +export function withErrorSpy< + TArgs extends any[], + TResult, +>( + it: (...args: TArgs) => TResult, + ...args: TArgs +) { + args[1] = wrapTestFunction(args[1], "error"); + return it(...args); +} + +export function withWarningSpy< + TArgs extends any[], + TResult, +>( + it: (...args: TArgs) => TResult, + ...args: TArgs +) { + args[1] = wrapTestFunction(args[1], "warn"); + return it(...args); +} + +export function withLogSpy< + TArgs extends any[], + TResult, +>( + it: (...args: TArgs) => TResult, + ...args: TArgs +) { + args[1] = wrapTestFunction(args[1], "log"); + return it(...args); +} diff --git a/src/utilities/testing/withErrorSpy.ts b/src/utilities/testing/withErrorSpy.ts deleted file mode 100644 index 2c39e8c1f91..00000000000 --- a/src/utilities/testing/withErrorSpy.ts +++ /dev/null @@ -1,21 +0,0 @@ -export function withErrorSpy< - TArgs extends any[], - TResult, ->( - it: (...args: TArgs) => TResult, - ...args: TArgs -) { - const fn = args[1]; - args[1] = function () { - const args = arguments; - const errorSpy = jest.spyOn(console, 'error'); - errorSpy.mockImplementation(() => {}); - return new Promise(resolve => { - resolve(fn?.apply(this, args)); - }).finally(() => { - expect(errorSpy).toMatchSnapshot(); - errorSpy.mockReset(); - }); - }; - return it(...args); -} From 65aa56124611301925b2a373f5509ac7be97ae0d Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 16 Jul 2021 16:51:27 -0400 Subject: [PATCH 2/2] Mention PR #8508 in CHANGELOG.md. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5080702b183..5a67c6d4616 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,9 @@ - The [`nextFetchPolicy`](https://github.com/apollographql/apollo-client/pull/6893) option for `client.watchQuery` and `useQuery` will no longer be removed from the `options` object after it has been applied, and instead will continue to be applied any time `options.fetchPolicy` is reset to another value, until/unless the `options.nextFetchPolicy` property is removed from `options`.
[@benjamn](https://github.com/benjamn) in [#8465](https://github.com/apollographql/apollo-client/pull/8465) +- Make `readField` default to reading from current object only when the `from` option/argument is actually omitted, not when `from` is passed to `readField` with an undefined value. A warning will be printed when this situation occurs.
+ [@benjamn](https://github.com/benjamn) in [#8508](https://github.com/apollographql/apollo-client/pull/8508) + ### Improvements - `InMemoryCache` now _guarantees_ that any two result objects returned by the cache (from `readQuery`, `readFragment`, etc.) will be referentially equal (`===`) if they are deeply equal. Previously, `===` equality was often achievable for results for the same query, on a best-effort basis. Now, equivalent result objects will be automatically shared among the result trees of completely different queries. This guarantee is important for taking full advantage of optimistic updates that correctly guess the final data, and for "pure" UI components that can skip re-rendering when their input data are unchanged.