Skip to content

Commit

Permalink
Warn when clobbering non-normalized data in the cache.
Browse files Browse the repository at this point in the history
One consequence of #5603 is that replacing non-normalized data in the
cache can result in loss of useful data. In almost every case, the right
solution is to make sure the data can be normalized, or (if that isn't
possible) to define a custom merge function for the replaced field, within
the parent TypePolicy.

It turns out we can give a very detailed warning in all such situations,
so that's what this commit does. Looking at the output for our test suite,
every warning is legitimate and worth fixing. I will resolve the warnings
and test failures that our test suite generates in subsequent commits.

TODO Update the documentation URLs after #5677 is merged.
  • Loading branch information
benjamn committed Jan 23, 2020
1 parent 171b6c3 commit 86a2bba
Showing 1 changed file with 63 additions and 5 deletions.
68 changes: 63 additions & 5 deletions src/cache/inmemory/entityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -450,18 +450,17 @@ const storeObjectReconciler: ReconcilerFunction<[EntityStore]> = function (
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;
}

if (process.env.NODE_ENV !== 'production') {
warnAboutDataLoss(existingObject, incomingObject, property, store);
}
}

// In all other cases, incoming replaces existing without any effort to
Expand All @@ -470,6 +469,65 @@ const storeObjectReconciler: ReconcilerFunction<[EntityStore]> = function (
return incoming;
}

function warnAboutDataLoss(
existingObject: Record<string | number, any>,
incomingObject: Record<string | number, any>,
property: string | number,
store: EntityStore,
) {
const existing = existingObject[property];
const incoming = incomingObject[property];

if (!isReference(existing) &&
!isReference(incoming) &&
!Object.keys(existing).every(key => hasOwn.call(incoming, key))) {
const fieldName = fieldNameFromStoreName(String(property));

const parentType =
getTypenameFromStoreObject(store, existingObject) ||
getTypenameFromStoreObject(store, incomingObject);

const childTypenames: string[] = [];
// Arrays 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 = getTypenameFromStoreObject(store, child);
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 ${
parentType
}.${
fieldName
} 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://deploy-preview-5677--apollo-client-docs.netlify.com/docs/react/v3.0-beta/caching/cache-configuration/#generating-unique-identifiers
* Defining custom merge functions: https://deploy-preview-5677--apollo-client-docs.netlify.com/docs/react/v3.0-beta/caching/cache-field-behavior/#merging-non-normalized-objects
`
);
}
}

export function supportsResultCaching(store: any): store is EntityStore {
// When result caching is disabled, store.depend will be null.
return !!(store instanceof EntityStore && store.group.caching);
Expand Down

0 comments on commit 86a2bba

Please sign in to comment.