diff --git a/CHANGELOG.md b/CHANGELOG.md index 93d8dd0f8a1..b8759291608 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,6 +73,9 @@ - The `QueryOptions`, `MutationOptions`, and `SubscriptionOptions` React Apollo interfaces have been renamed to `QueryDataOptions`, `MutationDataOptions`, and `SubscriptionDataOptions` (to avoid conflicting with similarly named and exported Apollo Client interfaces). +- `InMemoryCache` will no longer merge the fields of written objects unless the objects are known to have the same identity, and the values of fields with the same name will not be recursively merged unless a custom `merge` function is defined by a field policy for that field, within a type policy associated with the `__typename` of the parent object.
+ [@benjamn](https://github.com/benjamn) in [#5603](https://github.com/apollographql/apollo-client/pull/5603) + ## Apollo Client (2.6.4) ### Apollo Client (2.6.4) diff --git a/src/__tests__/__snapshots__/ApolloClient.ts.snap b/src/__tests__/__snapshots__/ApolloClient.ts.snap index 007137bce42..e2c039e4166 100644 --- a/src/__tests__/__snapshots__/ApolloClient.ts.snap +++ b/src/__tests__/__snapshots__/ApolloClient.ts.snap @@ -344,7 +344,6 @@ Object { "a": 1, "d": Object { "__typename": "D", - "e": 4, "h": Object { "__typename": "H", "i": 7, diff --git a/src/__tests__/local-state/export.ts b/src/__tests__/local-state/export.ts index 0238226bb4a..a9551dcbfa8 100644 --- a/src/__tests__/local-state/export.ts +++ b/src/__tests__/local-state/export.ts @@ -2,6 +2,7 @@ import gql from 'graphql-tag'; import { print } from 'graphql/language/printer'; import { Observable } from '../../utilities/observables/Observable'; +import { itAsync } from '../../utilities/testing/itAsync'; import { ApolloLink } from '../../link/core/ApolloLink'; import { ApolloClient } from '../..'; import { InMemoryCache } from '../../cache/inmemory/inMemoryCache'; @@ -325,10 +326,10 @@ describe('@client @export tests', () => { }); }); - it( + itAsync( 'should support setting an @client @export variable, loaded from the ' + 'cache, on a virtual field that is combined into a remote query.', - done => { + (resolve, reject) => { const query = gql` query postRequiringReview($reviewerId: Int!) { postRequiringReview { @@ -361,7 +362,7 @@ describe('@client @export tests', () => { reviewerDetails, }, }); - }); + }).setOnError(reject); const cache = new InMemoryCache(); const client = new ApolloClient({ @@ -375,6 +376,7 @@ describe('@client @export tests', () => { postRequiringReview: { loggedInReviewerId, __typename: 'Post', + id: 10, }, }, }); @@ -388,8 +390,7 @@ describe('@client @export tests', () => { }, reviewerDetails, }); - done(); - }); + }).then(resolve, reject); }, ); diff --git a/src/__tests__/local-state/general.ts b/src/__tests__/local-state/general.ts index 407e7b3a8c4..c0f4332a752 100644 --- a/src/__tests__/local-state/general.ts +++ b/src/__tests__/local-state/general.ts @@ -864,7 +864,7 @@ describe('Combining client and server state/operations', () => { }); }); - itAsync('should support nested quering of both server and client fields', (resolve, reject) => { + itAsync('should support nested querying of both server and client fields', (resolve, reject) => { const query = gql` query GetUser { user { @@ -878,7 +878,17 @@ describe('Combining client and server state/operations', () => { const link = new ApolloLink(operation => { expect(operation.operationName).toBe('GetUser'); return Observable.of({ - data: { user: { lastName: 'Doe', __typename: 'User' } }, + data: { + user: { + __typename: 'User', + // We need an id (or a keyFields policy) because, if the User + // object is not identifiable, the call to cache.writeData + // below will simply replace the existing data rather than + // merging the new data with the existing data. + id: 123, + lastName: 'Doe', + }, + }, }); }); @@ -892,13 +902,14 @@ describe('Combining client and server state/operations', () => { data: { user: { __typename: 'User', + id: 123, firstName: 'John', }, }, }); client.watchQuery({ query }).subscribe({ - next: ({ data }: any) => { + next({ data }: any) { const { user } = data; try { expect(user).toMatchObject({ diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts index 7ec01c1b2dc..cb0cc39313b 100644 --- a/src/cache/inmemory/__tests__/cache.ts +++ b/src/cache/inmemory/__tests__/cache.ts @@ -701,8 +701,9 @@ describe('Cache', () => { 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: { - e: 4, h: { i: 7, }, diff --git a/src/cache/inmemory/__tests__/writeToStore.ts b/src/cache/inmemory/__tests__/writeToStore.ts index d7f1bd51e0b..6b02e7ada5f 100644 --- a/src/cache/inmemory/__tests__/writeToStore.ts +++ b/src/cache/inmemory/__tests__/writeToStore.ts @@ -1307,7 +1307,7 @@ describe('writing to the store', () => { }); }); - it('should merge objects when overwriting a generated id with a real id', () => { + it('should not merge unidentified data when replacing with ID reference', () => { const dataWithoutId = { author: { firstName: 'John', @@ -1342,7 +1342,7 @@ describe('writing to the store', () => { } } `; - const expStoreWithoutId = defaultNormalizedCacheFactory({ + const expectedStoreWithoutId = defaultNormalizedCacheFactory({ ROOT_QUERY: { __typename: 'Query', author: { @@ -1352,10 +1352,9 @@ describe('writing to the store', () => { }, }, }); - const expStoreWithId = defaultNormalizedCacheFactory({ + const expectedStoreWithId = defaultNormalizedCacheFactory({ Author__129: { firstName: 'John', - lastName: 'Smith', id: '129', __typename: 'Author', }, @@ -1364,17 +1363,19 @@ describe('writing to the store', () => { author: makeReference('Author__129'), }, }); + const storeWithoutId = writer.writeQueryToStore({ result: dataWithoutId, query: queryWithoutId, }); - expect(storeWithoutId.toObject()).toEqual(expStoreWithoutId.toObject()); + expect(storeWithoutId.toObject()).toEqual(expectedStoreWithoutId.toObject()); + const storeWithId = writer.writeQueryToStore({ result: dataWithId, query: queryWithId, store: storeWithoutId, }); - expect(storeWithId.toObject()).toEqual(expStoreWithId.toObject()); + 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', () => { diff --git a/src/cache/inmemory/writeToStore.ts b/src/cache/inmemory/writeToStore.ts index 4b27cc31e32..ebcc2a6314b 100644 --- a/src/cache/inmemory/writeToStore.ts +++ b/src/cache/inmemory/writeToStore.ts @@ -1,5 +1,5 @@ import { SelectionSetNode, FieldNode, DocumentNode } from 'graphql'; -import { invariant, InvariantError } from 'ts-invariant'; +import { invariant } from 'ts-invariant'; import { createFragmentMap, @@ -29,6 +29,7 @@ import { import { shouldInclude } from '../../utilities/graphql/directives'; import { cloneDeep } from '../../utilities/common/cloneDeep'; +import { isEqual } from '../../utilities/common/isEqual'; import { defaultNormalizedCacheFactory } from './entityCache'; import { NormalizedCache, StoreObject } from './types'; @@ -49,7 +50,6 @@ export type WriteContext = { type StoreObjectMergeFunction = ( existing: StoreObject, incoming: StoreObject, - overrides?: MergeOverrides, ) => StoreObject; type MergeOverrides = Record = function ( +const storeObjectReconciler: ReconcilerFunction<[NormalizedCache]> = function ( existingObject, incomingObject, property, // This parameter comes from the additional argument we pass to the // merge method in context.mergeStoreObjects (see writeQueryToStore). store, - mergeOverrides, ) { // 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]; - const mergeChildObj = mergeOverrides && mergeOverrides[property]; - if (mergeChildObj && typeof mergeChildObj.merge === "function") { - // If this property was overridden by a custom merge function, then - // its merged value has already been determined, so we should return - // incoming without recursively merging it into existing. - return incoming; - } - if ( existing !== incoming && // The DeepMerger class has various helpful utilities that we might as @@ -422,48 +409,22 @@ const storeObjectReconciler: ReconcilerFunction<[ return incoming; } - const childMergeOverrides = mergeChildObj && mergeChildObj.child; - - if (isReference(incoming)) { - if (isReference(existing)) { - // Incoming references always replace existing references, but we can - // avoid changing the object identity when the __ref is the same. - return incoming.__ref === existing.__ref ? existing : incoming; - } - // Incoming references can be merged with existing non-reference data - // if the existing data appears to be of a compatible type. - store.set( - incoming.__ref, - this.merge( - existing, - store.get(incoming.__ref), - store, - childMergeOverrides, - ), - ); - return incoming; - } else if (isReference(existing)) { - throw new InvariantError( - `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.`, - ); - } + 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.`, + ); - if (Array.isArray(incoming)) { - if (!Array.isArray(existing)) return incoming; - if (existing.length > incoming.length) { - // Allow the incoming array to truncate the existing array, if the - // incoming array is shorter. - return this.merge( - existing.slice(0, incoming.length), - incoming, - store, - childMergeOverrides, - ); - } + // 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 (isEqual(existing, incoming)) { + return existing; } - - return this.merge(existing, incoming, store, childMergeOverrides); } + // 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; } diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts index 8656e22ae20..06f58b996a4 100644 --- a/src/core/__tests__/QueryManager/index.ts +++ b/src/core/__tests__/QueryManager/index.ts @@ -33,6 +33,7 @@ import wrap from '../../../utilities/testing/wrap'; import observableToPromise, { observableToPromiseAndSubscription, } from '../../../utilities/testing/observableToPromise'; +import subscribeAndCount from '../../../utilities/testing/subscribeAndCount'; import { stripSymbols } from '../../../utilities/testing/stripSymbols'; import { itAsync } from '../../../utilities/testing/itAsync'; @@ -1654,7 +1655,7 @@ describe('QueryManager', () => { ]).then(resolve, reject); }); - itAsync(`updates result of previous query if the result of a new query overlaps`, (resolve, reject) => { + itAsync('updates result of previous query if the result of a new query overlaps', (resolve, reject) => { const query1 = gql` { people_one(id: 1) { @@ -1701,21 +1702,21 @@ describe('QueryManager', () => { ); const observable = queryManager.watchQuery({ query: query1 }); - return observableToPromise( - { observable }, - result => { - expect(stripSymbols(result.data)).toEqual(data1); + + subscribeAndCount(reject, observable, (handleCount, result) => { + if (handleCount === 1) { + expect(result.data).toEqual(data1); queryManager.query({ query: query2 }); - }, - // 3 because the query init action for the second query causes a callback - result => - expect(stripSymbols(result.data)).toEqual({ + } else if (handleCount === 2) { + expect(result.data).toEqual({ people_one: { - name: 'Luke Skywalker has a new name', + name: 'Luke Skywalker', age: 50, }, - }), - ).then(resolve, reject); + }); + resolve(); + } + }); }); itAsync('warns if you forget the template literal tag', async (resolve, reject) => { @@ -2374,7 +2375,7 @@ describe('QueryManager', () => { ]).then(resolve, reject); }); - itAsync('should not error when merging a generated id store node with a real id node', (resolve, reject) => { + itAsync('should not error when replacing unidentified data with a normalized ID', (resolve, reject) => { const queryWithoutId = gql` query { author { @@ -2387,6 +2388,7 @@ describe('QueryManager', () => { } } `; + const queryWithId = gql` query { author { @@ -2398,6 +2400,7 @@ describe('QueryManager', () => { } } `; + const dataWithoutId = { author: { name: { @@ -2408,6 +2411,7 @@ describe('QueryManager', () => { __typename: 'Author', }, }; + const dataWithId = { author: { name: { @@ -2417,16 +2421,7 @@ describe('QueryManager', () => { __typename: 'Author', }, }; - const mergedDataWithoutId = { - author: { - name: { - firstName: 'Jane', - lastName: 'Smith', - }, - age: '124', - __typename: 'Author', - }, - }; + const queryManager = createQueryManager({ link: mockSingleLink({ request: { query: queryWithoutId }, @@ -2440,20 +2435,20 @@ describe('QueryManager', () => { const observableWithId = queryManager.watchQuery({ query: queryWithId, }); + const observableWithoutId = queryManager.watchQuery({ query: queryWithoutId, }); - // I'm not sure the waiting 60 here really is required, but the test used to do it return Promise.all([ observableToPromise( - { observable: observableWithoutId, wait: 120 }, + { observable: observableWithoutId }, + result => expect(stripSymbols(result.data)).toEqual(dataWithoutId), result => expect(stripSymbols(result.data)).toEqual(dataWithoutId), - result => - expect(stripSymbols(result.data)).toEqual(mergedDataWithoutId), ), - observableToPromise({ observable: observableWithId, wait: 120 }, result => - expect(stripSymbols(result.data)).toEqual(dataWithId), + observableToPromise( + { observable: observableWithId }, + result => expect(stripSymbols(result.data)).toEqual(dataWithId), ), ]).then(resolve, reject); });