Warn when clobbering non-normalized data in the cache.

src/cache/inmemory/__tests__/writeToStore.ts

@@ -1706,64 +1706,6 @@ describe('writing to the store', () => {
     });
   });
 
-  it('throws when trying to write an object without id that was previously queried with id', () => {
-    const store = defaultNormalizedCacheFactory({
-      ROOT_QUERY: {
-        __typename: 'Query',
-        item: makeReference('abcd'),
-      },
-      abcd: {
-        id: 'abcd',
-        __typename: 'Item',
-        stringField: 'This is a string!',
-      },
-    });
-
-    const writer = new StoreWriter({
-      policies: new Policies({
-        dataIdFromObject: getIdField,
-      }),
-    });
-
-    expect(() => {
-      writer.writeQueryToStore({
-        store,
-        result: {
-          item: {
-            __typename: 'Item',
-            stringField: 'This is still a string!',
-          },
-        },
-        query: gql`
-          query Failure {
-            item {
-              stringField
-            }
-          }
-        `,
-      });
-    }).toThrowErrorMatchingSnapshot();
-
-    expect(() => {
-      writer.writeQueryToStore({
-        store,
-        query: gql`
-          query {
-            item {
-              stringField
-            }
-          }
-        `,
-        result: {
-          item: {
-            __typename: 'Item',
-            stringField: 'This is still a string!',
-          },
-        },
-      });
-    }).toThrowError(/contains an id of abcd/g);
-  });
-
   it('properly handles the @connection directive', () => {
     const store = defaultNormalizedCacheFactory();
 

src/cache/inmemory/entityStore.ts

@@ -1,18 +1,11 @@
 import { dep, OptimisticDependencyFunction, KeyTrie } from 'optimism';
-import { invariant } from 'ts-invariant';
 import { equal } from '@wry/equality';
 
 import { isReference, StoreValue } from '../../utilities/graphql/storeUtils';
-import {
-  DeepMerger,
-  ReconcilerFunction,
-} from '../../utilities/common/mergeDeep';
+import { DeepMerger } from '../../utilities/common/mergeDeep';
 import { canUseWeakMap } from '../../utilities/common/canUse';
 import { NormalizedCache, NormalizedCacheObject, StoreObject } from './types';
-import {
-  getTypenameFromStoreObject,
-  fieldNameFromStoreName,
-} from './helpers';
+import { fieldNameFromStoreName } from './helpers';
 
 const hasOwn = Object.prototype.hasOwnProperty;
 
@@ -36,8 +29,11 @@ export abstract class EntityStore implements NormalizedCache {
     return { ...this.data };
   }
 
-  public has(dataId: string): boolean {
-    return this.lookup(dataId, true) !== void 0;
+  public has(dataId: string, fieldName?: string): boolean {
+    const found = fieldName
+      ? this.get(dataId, fieldName)
+      : this.lookup(dataId, true);
+    return found !== void 0;
   }
 
   public get(dataId: string, fieldName: string): StoreValue {
@@ -66,22 +62,25 @@ export abstract class EntityStore implements NormalizedCache {
 
   public merge(dataId: string, incoming: StoreObject): void {
     const existing = this.lookup(dataId);
-    const merged = new DeepMerger(storeObjectReconciler).merge(existing, incoming, this);
+    const merged = new DeepMerger(storeObjectReconciler).merge(existing, incoming);
     if (merged !== existing) {
       this.data[dataId] = merged;
       delete this.refs[dataId];
       if (this.group.caching) {
+        const fieldsToDirty: Record<string, 1> = Object.create(null);
         // If we added a new StoreObject where there was previously none, dirty
         // anything that depended on the existence of this dataId, such as the
         // EntityStore#has method.
-        if (!existing) this.group.dirty(dataId, "__exists");
-        // Now invalidate dependents who called getFieldValue for any
-        // fields that are changing as a result of this merge.
+        if (!existing) fieldsToDirty.__exists = 1;
+        // Now invalidate dependents who called getFieldValue for any fields
+        // that are changing as a result of this merge.
         Object.keys(incoming).forEach(storeFieldName => {
-          if (!existing || incoming[storeFieldName] !== existing[storeFieldName]) {
-            this.group.dirty(dataId, storeFieldName);
+          if (!existing || existing[storeFieldName] !== merged[storeFieldName]) {
+            fieldsToDirty[fieldNameFromStoreName(storeFieldName)] = 1;
           }
         });
+        Object.keys(fieldsToDirty).forEach(
+          fieldName => this.group.dirty(dataId, fieldName));
       }
     }
   }
@@ -417,57 +416,19 @@ class Layer extends EntityStore {
   }
 }
 
-const storeObjectReconciler: ReconcilerFunction<[EntityStore]> = function (
-  existingObject,
-  incomingObject,
-  property,
-  // This parameter comes from the additional argument we pass to the
-  // merge method in context.mergeStoreObjects (see writeQueryToStore).
-  store,
-) {
-  // In the future, reconciliation logic may depend on the type of the parent
-  // StoreObject, not just the values of the given property.
-  const existing = existingObject[property];
-  const incoming = incomingObject[property];
-
-  if (
-    existing !== incoming &&
-    // The DeepMerger class has various helpful utilities that we might as
-    // well reuse here.
-    this.isObject(existing) &&
-    this.isObject(incoming)
-  ) {
-    const eType = getTypenameFromStoreObject(store, existing);
-    const iType = getTypenameFromStoreObject(store, incoming);
-    // If both objects have a typename and the typename is different, let the
-    // incoming object win. The typename can change when a different subtype
-    // of a union or interface is written to the store.
-    if (
-      typeof eType === 'string' &&
-      typeof iType === 'string' &&
-      eType !== iType
-    ) {
-      return incoming;
-    }
-
-    invariant(
-      !isReference(existing) || isReference(incoming),
-      `Store error: the application attempted to write an object with no provided id but the store already contains an id of ${existing.__ref} for this object.`,
-    );
-
-    // It's worth checking deep equality here (even though blindly
-    // returning incoming would be logically correct) because preserving
-    // the referential identity of existing data can prevent needless
-    // rereading and rerendering.
-    if (equal(existing, incoming)) {
-      return existing;
-    }
-  }
-
-  // In all other cases, incoming replaces existing without any effort to
-  // merge them deeply, since custom merge functions have already been
-  // applied to the incoming data by walkWithMergeOverrides.
-  return incoming;
+function storeObjectReconciler(
+  existingObject: StoreObject,
+  incomingObject: StoreObject,
+  property: string | number,
+): StoreValue {
+  const existingValue = existingObject[property];
+  const incomingValue = incomingObject[property];
+  // Wherever there is a key collision, prefer the incoming value, unless
+  // it is deeply equal to the existing value. It's worth checking deep
+  // equality here (even though blindly returning incoming would be
+  // logically correct) because preserving the referential identity of
+  // existing data can prevent needless rereading and rerendering.
+  return equal(existingValue, incomingValue) ? existingValue : incomingValue;
 }
 
 export function supportsResultCaching(store: any): store is EntityStore {

src/cache/inmemory/policies.ts

@@ -594,14 +594,6 @@ export class Policies {
 
       const merge = policy && policy.merge;
       if (merge) {
-        if (process.env.NODE_ENV !== "production") {
-          // It may be tempting to modify existing data directly, for example
-          // by pushing more elements onto an existing array, but merge
-          // functions are expected to be pure, so it's important that we
-          // enforce immutability in development.
-          maybeDeepFreeze(existing);
-        }
-
         // If storage ends up null, that just means no options.storage object
         // has ever been created for a read function for this field before, so
         // there's nothing this merge function could do with options.storage
@@ -613,7 +605,7 @@ export class Policies {
           ? policies.storageTrie.lookupArray(storageKeys)
           : null;
 
-        return merge(existing, applied, {
+        return merge(maybeDeepFreeze(existing), maybeDeepFreeze(applied), {
           args: argumentsObjectFromField(field, variables),
           field,
           fieldName,

src/cache/inmemory/types.ts

@@ -19,7 +19,7 @@ export declare type IdGetter = (
  * StoreObjects from the cache
  */
 export interface NormalizedCache {
-  has(dataId: string): boolean;
+  has(dataId: string, fieldName?: string): boolean;
   get(dataId: string, fieldName: string): StoreValue;
   merge(dataId: string, incoming: StoreObject): void;
   delete(dataId: string, fieldName?: string): boolean;

src/cache/inmemory/writeToStore.ts

@@ -19,6 +19,8 @@ import {
   isField,
   resultKeyNameFromField,
   StoreValue,
+  Reference,
+  isReference,
 } from '../../utilities/graphql/storeUtils';
 
 import { shouldInclude } from '../../utilities/graphql/directives';
@@ -27,7 +29,7 @@ import { cloneDeep } from '../../utilities/common/cloneDeep';
 import { Policies, FieldValueGetter } from './policies';
 import { defaultNormalizedCacheFactory } from './entityStore';
 import { NormalizedCache, StoreObject } from './types';
-import { makeProcessedFieldsMerger } from './helpers';
+import { makeProcessedFieldsMerger, fieldNameFromStoreName } from './helpers';
 
 export type WriteContext = {
   readonly store: NormalizedCache;
@@ -137,21 +139,31 @@ export class StoreWriter {
       // fall back to that.
       context.getFieldValue<string>(entityRef, "__typename");
 
-    store.merge(
-      dataId,
-      policies.applyMerges(
-        entityRef,
-        this.processSelectionSet({
-          result,
-          selectionSet,
-          context,
-          typename,
-        }),
-        context.getFieldValue,
-        context.variables,
-      ),
+    const incoming = policies.applyMerges(
+      entityRef,
+      this.processSelectionSet({
+        result,
+        selectionSet,
+        context,
+        typename,
+      }),
+      context.getFieldValue,
+      context.variables,
     );
 
+    if (process.env.NODE_ENV !== "production") {
+      Object.keys(incoming).forEach(storeFieldName => {
+        warnAboutDataLoss(
+          entityRef,
+          incoming,
+          storeFieldName,
+          context.getFieldValue,
+        );
+      });
+    }
+
+    store.merge(dataId, incoming);
+
     return store;
   }
 
@@ -297,3 +309,84 @@ export class StoreWriter {
     });
   }
 }
+
+const warnings = new Set<string>();
+
+// Note that this function is unused in production, and thus should be pruned
+// by any well-configured minifier.
+function warnAboutDataLoss(
+  existingObject: StoreObject | Reference,
+  incomingObject: StoreObject | Reference,
+  storeFieldName: string,
+  getFieldValue: FieldValueGetter,
+) {
+  const getChild = (objOrRef: StoreObject | Reference): StoreObject => {
+    const child = getFieldValue<StoreObject>(objOrRef, storeFieldName);
+    return typeof child === "object" && child;
+  };
+
+  const existing = getChild(existingObject);
+  if (!existing) return;
+
+  const incoming = getChild(incomingObject);
+  if (!incoming) return;
+
+  // It's always safe to replace a reference, since it refers to data
+  // safely stored elsewhere.
+  if (isReference(existing)) return;
+
+  // If we're replacing every key of the existing object, then the
+  // existing data would be overwritten even if the objects were
+  // normalized, so warning would not be helpful here.
+  if (Object.keys(existing).every(
+    key => getFieldValue(incoming, key) !== void 0)) {
+    return;
+  }
+
+  const parentType =
+    getFieldValue(existingObject, "__typename") ||
+    getFieldValue(incomingObject, "__typename");
+
+  const fieldName = fieldNameFromStoreName(storeFieldName);
+  const typeDotName = `${parentType}.${fieldName}`;
+
+  if (warnings.has(typeDotName)) return;
+  warnings.add(typeDotName);
+
+  const childTypenames: string[] = [];
+  // Arrays do not have __typename fields, and always need a custom merge
+  // function, even if their elements are normalized entities.
+  if (!Array.isArray(existing) &&
+      !Array.isArray(incoming)) {
+    [existing, incoming].forEach(child => {
+      const typename = getFieldValue(child, "__typename");
+      if (typeof typename === "string" &&
+          !childTypenames.includes(typename)) {
+        childTypenames.push(typename);
+      }
+    });
+  }
+
+  invariant.warn(
+`Cache data may be lost when replacing the ${fieldName} field of a ${parentType} object.
+
+To address this problem (which is not a bug in Apollo Client), ${
+  childTypenames.length
+    ? "either ensure that objects of type " +
+        childTypenames.join(" and ") + " have IDs, or "
+    : ""
+}define a custom merge function for the ${
+  typeDotName
+} field, so the InMemoryCache can safely merge these objects:
+
+  existing: ${JSON.stringify(existing).slice(0, 1000)}
+  incoming: ${JSON.stringify(incoming).slice(0, 1000)}
+
+For more information about these options, please refer to the documentation:
+
+  * Ensuring entity objects have IDs: https://go.apollo.dev/c/generating-unique-identifiers
+
+  * Defining custom merge functions: https://go.apollo.dev/c/merging-non-normalized-objects
+`
+  );
+}