Merge pull request #9742 from apollographql/issue-9735-DeepMerger-pro…

…d-only-mismerge

Restrict `DeepMerger` mutable object reuse to fix subtle production-only bug

CHANGELOG.md

@@ -11,6 +11,9 @@
 - Fix bug where `onCompleted()` and `onError()` are stale for `useMutation()`. <br/>
   [@charle692](https://github.com/charle692) in [#9740](https://github.com/apollographql/apollo-client/pull/9740)
 
+- Limit scope of `DeepMerger` object reuse, and avoid using `Object.isFrozen`, which can introduce differences between development and production if objects that were frozen using `Object.freeze` in development are left unfrozen in production. <br/>
+  [@benjamn](https://github.com/benjamn) in [#9742](https://github.com/apollographql/apollo-client/pull/9742)
+
 ## Apollo Client 3.6.4 (2022-05-16)
 
 ### Bug Fixes

src/cache/inmemory/readFromStore.ts

@@ -22,9 +22,10 @@ import {
   getFragmentDefinitions,
   getMainDefinition,
   getQueryDefinition,
-  DeepMerger,
   getFragmentFromSelection,
   maybeDeepFreeze,
+  mergeDeepArray,
+  DeepMerger,
   isNonNullObject,
   canUseWeakMap,
   compact,
@@ -49,8 +50,6 @@ interface ReadContext extends ReadMergeModifyContext {
   policies: Policies;
   canonizeResults: boolean;
   fragmentMap: FragmentMap;
-  // General-purpose deep-merge function for use during reads.
-  merge<T>(existing: T, incoming: T): T;
 };
 
 export type ExecResult<R = any> = {
@@ -233,7 +232,6 @@ export class StoreReader {
     };
 
     const rootRef = makeReference(rootId);
-    const merger = new DeepMerger;
     const execResult = this.executeSelectionSet({
       selectionSet: getMainDefinition(query).selectionSet,
       objectOrReference: rootRef,
@@ -246,15 +244,6 @@ export class StoreReader {
         varString: canonicalStringify(variables),
         canonizeResults,
         fragmentMap: createFragmentMap(getFragmentDefinitions(query)),
-        merge(a, b) {
-          // We use the same DeepMerger instance throughout the read, so any
-          // merged objects created during this read can be updated later in the
-          // read using in-place/destructive property assignments. Once the read
-          // is finished, these objects will be frozen, but in the meantime it's
-          // good for performance and memory usage if we avoid allocating a new
-          // object for every merged property.
-          return merger.merge(a, b);
-        },
       },
     });
 
@@ -325,21 +314,22 @@ export class StoreReader {
     const { variables, policies, store } = context;
     const typename = store.getFieldValue<string>(objectOrReference, "__typename");
 
-    let result: any = {};
+    const objectsToMerge: Record<string, any>[] = [];
     let missing: MissingTree | undefined;
+    const missingMerger = new DeepMerger();
 
     if (this.config.addTypename &&
         typeof typename === "string" &&
         !policies.rootIdsByTypename[typename]) {
       // Ensure we always include a default value for the __typename
       // field, if we have one, and this.config.addTypename is true. Note
       // that this field can be overridden by other merged objects.
-      result = { __typename: typename };
+      objectsToMerge.push({ __typename: typename });
     }
 
     function handleMissing<T>(result: ExecResult<T>, resultName: string): T {
       if (result.missing) {
-        missing = context.merge(missing, { [resultName]: result.missing });
+        missing = missingMerger.merge(missing, { [resultName]: result.missing });
       }
       return result.result;
     }
@@ -363,7 +353,7 @@ export class StoreReader {
 
         if (fieldValue === void 0) {
           if (!addTypenameToDocument.added(selection)) {
-            missing = context.merge(missing, {
+            missing = missingMerger.merge(missing, {
               [resultName]: `Can't find field '${
                 selection.name.value
               }' on ${
@@ -404,7 +394,7 @@ export class StoreReader {
         }
 
         if (fieldValue !== void 0) {
-          result = context.merge(result, { [resultName]: fieldValue });
+          objectsToMerge.push({ [resultName]: fieldValue });
         }
 
       } else {
@@ -419,6 +409,7 @@ export class StoreReader {
       }
     });
 
+    const result = mergeDeepArray(objectsToMerge);
     const finalResult: ExecResult = { result, missing };
     const frozen = context.canonizeResults
       ? this.canon.admit(finalResult)
@@ -443,10 +434,11 @@ export class StoreReader {
     context,
   }: ExecSubSelectedArrayOptions): ExecResult {
     let missing: MissingTree | undefined;
+    let missingMerger = new DeepMerger<MissingTree[]>();
 
     function handleMissing<T>(childResult: ExecResult<T>, i: number): T {
       if (childResult.missing) {
-        missing = context.merge(missing, { [i]: childResult.missing });
+        missing = missingMerger.merge(missing, { [i]: childResult.missing });
       }
       return childResult.result;
     }

src/utilities/common/mergeDeep.ts

@@ -101,21 +101,17 @@ export class DeepMerger<TContextArgs extends any[]> {
 
   public shallowCopyForMerge<T>(value: T): T {
     if (isNonNullObject(value)) {
-      if (this.pastCopies.has(value)) {
-        // In order to reuse a past copy, it must be mutable, but copied objects
-        // can sometimes be frozen while this DeepMerger is still active.
-        if (!Object.isFrozen(value)) return value;
-        this.pastCopies.delete(value);
-      }
-      if (Array.isArray(value)) {
-        value = (value as any).slice(0);
-      } else {
-        value = {
-          __proto__: Object.getPrototypeOf(value),
-          ...value,
-        };
+      if (!this.pastCopies.has(value)) {
+        if (Array.isArray(value)) {
+          value = (value as any).slice(0);
+        } else {
+          value = {
+            __proto__: Object.getPrototypeOf(value),
+            ...value,
+          };
+        }
+        this.pastCopies.add(value);
       }
-      this.pastCopies.add(value);
     }
     return value;
   }