diff --git a/CHANGELOG.md b/CHANGELOG.md index d3c316a6e3f..00c2cd8bd6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,9 @@ - Cache methods that would normally trigger a broadcast, like `cache.evict`, `cache.writeQuery`, and `cache.writeFragment`, can now be called with a named options object, which supports a `broadcast: boolean` property that can be used to silence the broadcast, for situations where you want to update the cache multiple times without triggering a broadcast each time.
[@benjamn](https://github.com/benjamn) in [#6288](https://github.com/apollographql/apollo-client/pull/6288) +- `InMemoryCache` now `console.warn`s in development whenever non-normalized data is dangerously overwritten, with helpful links to documentation about normalization and custom `merge` functions.
+ [@benjamn](https://github.com/benjamn) in [#6372](https://github.com/apollographql/apollo-client/pull/6372) + - The contents of the `@apollo/react-hooks` package have been merged into `@apollo/client`, enabling the following all-in-one `import`: ```ts import { ApolloClient, ApolloProvider, useQuery } from '@apollo/client'; diff --git a/src/__tests__/ApolloClient.ts b/src/__tests__/ApolloClient.ts index 8268ec5d0b3..44ce8ff0c10 100644 --- a/src/__tests__/ApolloClient.ts +++ b/src/__tests__/ApolloClient.ts @@ -668,7 +668,21 @@ describe('ApolloClient', () => { it('will write some deeply nested data to the store', () => { const client = new ApolloClient({ link: ApolloLink.empty(), - cache: new InMemoryCache(), + cache: new InMemoryCache({ + typePolicies: { + Query: { + fields: { + d: { + // Silence "Cache data may be lost..." warnings by + // unconditionally favoring the incoming data. + merge(_, incoming) { + return incoming; + }, + }, + }, + }, + }, + }), }); client.writeQuery({ @@ -1171,6 +1185,20 @@ describe('ApolloClient', () => { return new ApolloClient({ link, cache: new InMemoryCache({ + typePolicies: { + Person: { + fields: { + friends: { + // Deliberately silence "Cache data may be lost..." + // warnings by preferring the incoming data, rather + // than (say) concatenating the arrays together. + merge(_, incoming) { + return incoming; + }, + }, + }, + }, + }, dataIdFromObject: result => { if (result.id && result.__typename) { return result.__typename + result.id; diff --git a/src/__tests__/optimistic.ts b/src/__tests__/optimistic.ts index 283450789d4..360f0c81913 100644 --- a/src/__tests__/optimistic.ts +++ b/src/__tests__/optimistic.ts @@ -110,6 +110,20 @@ describe('optimistic mutation results', () => { const client = new ApolloClient({ link, cache: new InMemoryCache({ + typePolicies: { + TodoList: { + fields: { + todos: { + // Deliberately silence "Cache data may be lost..." + // warnings by favoring the incoming data, rather than + // (say) concatenating the arrays together. + merge(_, incoming) { + return incoming; + }, + }, + }, + }, + }, dataIdFromObject: (obj: any) => { if (obj.id && obj.__typename) { return obj.__typename + obj.id; diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts index 1de403b5ae4..d4451c3f62d 100644 --- a/src/cache/inmemory/__tests__/cache.ts +++ b/src/cache/inmemory/__tests__/cache.ts @@ -28,9 +28,8 @@ describe('Cache', () => { ]; cachesList.forEach((caches, i) => { - it(message + ` (${i + 1}/${cachesList.length})`, () => - callback(...caches), - ); + it(`${message} (${i + 1}/${cachesList.length})`, + () => callback(...caches)); }); } @@ -869,106 +868,118 @@ describe('Cache', () => { }); }); - itWithInitialData( - 'will write some deeply nested data to the store', - [{}], - proxy => { - proxy.writeQuery({ - data: { a: 1, d: { e: 4 } }, - query: gql` - { - a - d { - e - } + it('will write some deeply nested data to the store', () => { + const cache = new InMemoryCache({ + typePolicies: { + Query: { + fields: { + d: { + // Deliberately silence "Cache data may be lost..." + // warnings by unconditionally favoring the incoming data. + merge(_, incoming) { + return incoming; + }, + }, + }, + }, + }, + }); + + cache.writeQuery({ + data: { a: 1, d: { e: 4 } }, + query: gql` + { + a + d { + e } - `, - }); + } + `, + }); - expect((proxy as InMemoryCache).extract()).toEqual({ - ROOT_QUERY: { - __typename: "Query", - a: 1, - d: { - e: 4, - }, + expect((cache as InMemoryCache).extract()).toEqual({ + ROOT_QUERY: { + __typename: "Query", + a: 1, + d: { + e: 4, }, - }); + }, + }); - proxy.writeQuery({ - data: { a: 1, d: { h: { i: 7 } } }, - query: gql` - { - a - d { - h { - i - } + cache.writeQuery({ + data: { a: 1, d: { h: { i: 7 } } }, + query: gql` + { + a + d { + h { + i } } - `, - }); + } + `, + }); - expect((proxy as InMemoryCache).extract()).toEqual({ - ROOT_QUERY: { - __typename: "Query", - a: 1, - // The new value for d overwrites the old value, since there - // is no custom merge function defined for Query.d. - d: { - h: { - i: 7, - }, + expect((cache as InMemoryCache).extract()).toEqual({ + ROOT_QUERY: { + __typename: "Query", + a: 1, + // The new value for d overwrites the old value, since there + // is no custom merge function defined for Query.d. + d: { + h: { + i: 7, }, }, - }); + }, + }); - proxy.writeQuery({ - data: { - a: 1, - b: 2, - c: 3, - d: { e: 4, f: 5, g: 6, h: { i: 7, j: 8, k: 9 } }, - }, - query: gql` - { - a - b - c - d { - e - f - g - h { - i - j - k - } + cache.writeQuery({ + data: { + a: 1, + b: 2, + c: 3, + d: { e: 4, f: 5, g: 6, h: { i: 7, j: 8, k: 9 } }, + }, + query: gql` + { + a + b + c + d { + e + f + g + h { + i + j + k } } - `, - }); + } + `, + }); - expect((proxy as InMemoryCache).extract()).toEqual({ - ROOT_QUERY: { - __typename: "Query", - a: 1, - b: 2, - c: 3, - d: { - e: 4, - f: 5, - g: 6, - h: { - i: 7, - j: 8, - k: 9, - }, + expect((cache as InMemoryCache).extract()).toEqual({ + ROOT_QUERY: { + __typename: "Query", + a: 1, + b: 2, + c: 3, + d: { + e: 4, + f: 5, + g: 6, + h: { + i: 7, + j: 8, + k: 9, }, }, - }); - }, - ); + }, + }); + }); itWithInitialData( 'will write some data to the store with variables', diff --git a/src/cache/inmemory/__tests__/writeToStore.ts b/src/cache/inmemory/__tests__/writeToStore.ts index 2f1fb476212..45464c376fc 100644 --- a/src/cache/inmemory/__tests__/writeToStore.ts +++ b/src/cache/inmemory/__tests__/writeToStore.ts @@ -1331,7 +1331,48 @@ describe('writing to the store', () => { } } `; - const expectedStoreWithoutId = defaultNormalizedCacheFactory({ + + const cache = new InMemoryCache({ + typePolicies: { + Query: { + fields: { + author: { + // Silence "Cache data may be lost..." warnings by always + // preferring the incoming value. + merge(existing, incoming, { readField, isReference }) { + if (existing) { + expect(isReference(existing)).toBe(false); + expect(readField({ + fieldName: "__typename", + from: existing, + })).toBe("Author"); + + expect(isReference(incoming)).toBe(true); + expect(readField({ + fieldName: "__typename", + from: incoming, + })).toBe("Author"); + } + + return incoming; + }, + }, + }, + }, + }, + dataIdFromObject(object: any) { + if (object.__typename && object.id) { + return object.__typename + '__' + object.id; + } + }, + }); + + cache.writeQuery({ + query: queryWithoutId, + data: dataWithoutId, + }); + + expect(cache.extract()).toEqual({ ROOT_QUERY: { __typename: 'Query', author: { @@ -1341,7 +1382,13 @@ describe('writing to the store', () => { }, }, }); - const expectedStoreWithId = defaultNormalizedCacheFactory({ + + cache.writeQuery({ + query: queryWithId, + data: dataWithId, + }); + + expect(cache.extract()).toEqual({ Author__129: { firstName: 'John', id: '129', @@ -1352,21 +1399,6 @@ describe('writing to the store', () => { author: makeReference('Author__129'), }, }); - - const storeWithoutId = writeQueryToStore({ - writer, - result: dataWithoutId, - query: queryWithoutId, - }); - expect(storeWithoutId.toObject()).toEqual(expectedStoreWithoutId.toObject()); - - const storeWithId = writeQueryToStore({ - writer, - result: dataWithId, - query: queryWithId, - store: storeWithoutId, - }); - expect(storeWithId.toObject()).toEqual(expectedStoreWithId.toObject()); }); it('should allow a union of objects of a different type, when overwriting a generated id with a real id', () => { @@ -1400,7 +1432,48 @@ describe('writing to the store', () => { } } `; - const expStoreWithPlaceholder = defaultNormalizedCacheFactory({ + + let mergeCount = 0; + const cache = new InMemoryCache({ + typePolicies: { + Query: { + fields: { + author: { + merge(existing, incoming, { isReference, readField }) { + switch (++mergeCount) { + case 1: + expect(existing).toBeUndefined(); + expect(isReference(incoming)).toBe(false); + expect(incoming).toEqual(dataWithPlaceholder.author); + break; + case 2: + expect(existing).toEqual(dataWithPlaceholder.author); + expect(isReference(incoming)).toBe(true); + expect(readField("__typename", incoming)).toBe("Author"); + break; + case 3: + expect(isReference(existing)).toBe(true); + expect(readField("__typename", existing)).toBe("Author"); + expect(incoming).toEqual(dataWithPlaceholder.author); + break; + default: + fail("unreached"); + } + return incoming; + }, + }, + }, + }, + }, + }); + + // write the first object, without an ID, placeholder + cache.writeQuery({ + query, + data: dataWithPlaceholder, + }); + + expect(cache.extract()).toEqual({ ROOT_QUERY: { __typename: 'Query', author: { @@ -1409,8 +1482,15 @@ describe('writing to the store', () => { }, }, }); - const expStoreWithAuthor = defaultNormalizedCacheFactory({ - Author__129: { + + // replace with another one of different type with ID + cache.writeQuery({ + query, + data: dataWithAuthor, + }); + + expect(cache.extract()).toEqual({ + "Author:129": { firstName: 'John', lastName: 'Smith', id: '129', @@ -1418,40 +1498,33 @@ describe('writing to the store', () => { }, ROOT_QUERY: { __typename: 'Query', - author: makeReference('Author__129'), + author: makeReference('Author:129'), }, }); - // write the first object, without an ID, placeholder - const store = writeQueryToStore({ - writer, - result: dataWithPlaceholder, - query, - }); - expect(store.toObject()).toEqual(expStoreWithPlaceholder.toObject()); - - // replace with another one of different type with ID - writeQueryToStore({ - writer, - result: dataWithAuthor, - query, - store, - }); - expect(store.toObject()).toEqual(expStoreWithAuthor.toObject()); - // and go back to the original: - writeQueryToStore({ - writer, - result: dataWithPlaceholder, + cache.writeQuery({ query, - store, + data: dataWithPlaceholder, }); + // Author__129 will remain in the store, // but will not be referenced by any of the fields, // hence we combine, and in that very order - expect(store.toObject()).toEqual({ - ...expStoreWithAuthor.toObject(), - ...expStoreWithPlaceholder.toObject(), + expect(cache.extract()).toEqual({ + "Author:129": { + firstName: 'John', + lastName: 'Smith', + id: '129', + __typename: 'Author', + }, + ROOT_QUERY: { + __typename: 'Query', + author: { + hello: 'Foo', + __typename: 'Placeholder', + }, + }, }); }); diff --git a/src/cache/inmemory/entityStore.ts b/src/cache/inmemory/entityStore.ts index 847afd04e7b..763d77d5f2f 100644 --- a/src/cache/inmemory/entityStore.ts +++ b/src/cache/inmemory/entityStore.ts @@ -513,7 +513,7 @@ class Layer extends EntityStore { function storeObjectReconciler( existingObject: StoreObject, incomingObject: StoreObject, - property: string | number, + property: string, ): StoreValue { const existingValue = existingObject[property]; const incomingValue = incomingObject[property]; diff --git a/src/cache/inmemory/writeToStore.ts b/src/cache/inmemory/writeToStore.ts index 774f53bb480..e0238d15272 100644 --- a/src/cache/inmemory/writeToStore.ts +++ b/src/cache/inmemory/writeToStore.ts @@ -1,5 +1,5 @@ import { SelectionSetNode, FieldNode, DocumentNode } from 'graphql'; -import { InvariantError } from 'ts-invariant'; +import { invariant, InvariantError } from 'ts-invariant'; import { createFragmentMap, @@ -29,7 +29,7 @@ import { cloneDeep } from '../../utilities/common/cloneDeep'; import { ReadMergeContext } from './policies'; import { NormalizedCache } from './types'; -import { makeProcessedFieldsMerger, FieldValueToBeMerged } from './helpers'; +import { makeProcessedFieldsMerger, FieldValueToBeMerged, fieldNameFromStoreName } from './helpers'; import { StoreReader } from './readFromStore'; import { InMemoryCache } from './inMemoryCache'; @@ -271,6 +271,22 @@ export class StoreWriter { mergedFields = policies.applyMerges(entityRef, mergedFields, context); } + if (process.env.NODE_ENV !== "production") { + Object.keys(mergedFields).forEach(storeFieldName => { + const fieldName = fieldNameFromStoreName(storeFieldName); + // If a merge function was defined for this field, trust that it + // did the right thing about (not) clobbering data. + if (!policies.hasMergeFunction(typename, fieldName)) { + warnAboutDataLoss( + entityRef, + mergedFields, + storeFieldName, + context.store, + ); + } + }); + } + context.store.merge(dataId, mergedFields); return entityRef; @@ -304,3 +320,81 @@ export class StoreWriter { }); } } + +const warnings = new Set(); + +// Note that this function is unused in production, and thus should be +// pruned by any well-configured minifier. +function warnAboutDataLoss( + existingRef: Reference, + incomingObj: StoreObject, + storeFieldName: string, + store: NormalizedCache, +) { + const getChild = (objOrRef: StoreObject | Reference): StoreObject | false => { + const child = store.getFieldValue(objOrRef, storeFieldName); + return typeof child === "object" && child; + }; + + const existing = getChild(existingRef); + if (!existing) return; + + const incoming = getChild(incomingObj); + 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 => store.getFieldValue(incoming, key) !== void 0)) { + return; + } + + const parentType = + store.getFieldValue(existingRef, "__typename") || + store.getFieldValue(incomingObj, "__typename"); + const fieldName = fieldNameFromStoreName(storeFieldName); + const typeDotName = `${parentType}.${fieldName}`; + // Avoid warning more than once for the same type and field name. + 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 = store.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 all objects of type " + + childTypenames.join(" and ") + " have IDs, or " + : "" +}define a custom merge function for the ${ + typeDotName +} field, so 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 +`); +} diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts index 53994f1aff7..e407fd6d700 100644 --- a/src/core/__tests__/QueryManager/index.ts +++ b/src/core/__tests__/QueryManager/index.ts @@ -8,7 +8,7 @@ import { DocumentNode, GraphQLError } from 'graphql'; import { Observable, Observer } from '../../../utilities/observables/Observable'; import { ApolloLink } from '../../../link/core/ApolloLink'; import { GraphQLRequest, FetchResult } from '../../../link/core/types'; -import { InMemoryCache } from '../../../cache/inmemory/inMemoryCache'; +import { InMemoryCache, InMemoryCacheConfig } from '../../../cache/inmemory/inMemoryCache'; import { ApolloReducerConfig, NormalizedCacheObject @@ -64,7 +64,7 @@ describe('QueryManager', () => { clientAwareness = {}, }: { link: ApolloLink; - config?: ApolloReducerConfig; + config?: Partial; clientAwareness?: { [key: string]: string }; }) => { return new QueryManager({ @@ -2210,6 +2210,7 @@ describe('QueryManager', () => { }, }; + let mergeCount = 0; const queryManager = createQueryManager({ link: mockSingleLink({ request: { query: queryWithoutId }, @@ -2218,6 +2219,34 @@ describe('QueryManager', () => { request: { query: queryWithId }, result: { data: dataWithId }, }).setOnError(reject), + config: { + typePolicies: { + Query: { + fields: { + author: { + merge(existing, incoming, { isReference, readField }) { + switch (++mergeCount) { + case 1: + expect(existing).toBeUndefined(); + expect(isReference(incoming)).toBe(false); + expect(incoming).toEqual(dataWithoutId.author); + break; + case 2: + expect(existing).toEqual(dataWithoutId.author); + expect(isReference(incoming)).toBe(true); + expect(readField("id", incoming)).toBe("129"); + expect(readField("name", incoming)).toEqual(dataWithId.author.name); + break; + default: + fail("unreached"); + } + return incoming; + }, + }, + }, + }, + }, + }, }); const observableWithId = queryManager.watchQuery({ diff --git a/src/utilities/graphql/storeUtils.ts b/src/utilities/graphql/storeUtils.ts index 7076017e08d..31c9513fb8d 100644 --- a/src/utilities/graphql/storeUtils.ts +++ b/src/utilities/graphql/storeUtils.ts @@ -30,7 +30,7 @@ export function makeReference(id: string): Reference { } export function isReference(obj: any): obj is Reference { - return obj && typeof obj === 'object' && typeof obj.__ref === 'string'; + return Boolean(obj && typeof obj === 'object' && typeof obj.__ref === 'string'); } export type StoreValue =