Default to current object for readField only when from omitted. (#…

…8508)

Should help with #8499.

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`. <br/>
   [@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. <br/>
+  [@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. <br/>

src/__tests__/__snapshots__/exports.ts.snap

@@ -312,9 +312,12 @@ Array [
   "itAsync",
   "mockObservableLink",
   "mockSingleLink",
+  "stringifyForDisplay",
   "stripSymbols",
   "subscribeAndCount",
   "withErrorSpy",
+  "withLogSpy",
+  "withWarningSpy",
 ]
 `;
 

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\\":<undefined>}]",
+    ],
+    Array [
+      "Undefined 'from' passed to readField with arguments [\\"lastName\\",<undefined>]",
+    ],
+  ],
+  "results": Array [
+    Object {
+      "type": "return",
+      "value": undefined,
+    },
+    Object {
+      "type": "return",
+      "value": undefined,
+    },
+  ],
+}
+`;
+
 exports[`type policies support inheritance 1`] = `
 Object {
   "Cobra:{\\"tagId\\":\\"Egypt30BC\\"}": Object {

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<string>({
+                  fieldName: "firstName",
+                })
+              } ${
+                readField<string>("lastName")
+              }`;
+            },
+
+            fullNameWithVoids(_, { readField }) {
+              return `${
+                readField<string>({
+                  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<string>("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: {

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) {

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';

src/utilities/testing/mocking/mockLink.ts

@@ -15,17 +15,13 @@ import {
   removeClientSetsFromDocument,
   removeConnectionDirectiveFromDocument,
   cloneDeep,
-  makeUniqueId,
 } from '../../../utilities';
 
-export type ResultFunction<T> = () => 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("<undefined>");
-}
+export type ResultFunction<T> = () => T;
 
 export interface MockedResponse<TData = Record<string, any>> {
   request: GraphQLRequest;

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("<undefined>");
+}

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);
+}