diff --git a/packages/apollo-cache-inmemory/src/__tests__/__snapshots__/cache.ts.snap b/packages/apollo-cache-inmemory/src/__tests__/__snapshots__/cache.ts.snap index 7e33ebcb999..cd85b05f9a4 100644 --- a/packages/apollo-cache-inmemory/src/__tests__/__snapshots__/cache.ts.snap +++ b/packages/apollo-cache-inmemory/src/__tests__/__snapshots__/cache.ts.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 1`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 1`] = ` Object { "bar": Object { "i": 7, @@ -14,7 +14,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 2`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 2`] = ` Object { "bar": Object { "i": 7, @@ -32,7 +32,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 3`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 3`] = ` Object { "bar": Object { "i": 10, @@ -50,7 +50,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 4`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 4`] = ` Object { "bar": Object { "i": 10, @@ -68,7 +68,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 5`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 5`] = ` Object { "bar": Object { "i": 7, @@ -86,7 +86,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 6`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 6`] = ` Object { "bar": Object { "i": 10, @@ -104,7 +104,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 1`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 1`] = ` Object { "bar": Object { "i": 7, @@ -118,7 +118,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 2`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 2`] = ` Object { "bar": Object { "i": 7, @@ -136,7 +136,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 3`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 3`] = ` Object { "bar": Object { "i": 10, @@ -154,7 +154,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 4`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 4`] = ` Object { "bar": Object { "i": 10, @@ -172,7 +172,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 5`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 5`] = ` Object { "bar": Object { "i": 7, @@ -190,111 +190,7 @@ Object { } `; -exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 6`] = ` -Object { - "bar": Object { - "i": 10, - "j": 11, - "k": 12, - }, - "foo": Object { - "e": 4, - "f": 5, - "g": 6, - "h": Object { - "__ref": "bar", - }, - }, -} -`; - -exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 1`] = ` -Object { - "bar": Object { - "i": 7, - }, - "foo": Object { - "e": 4, - "h": Object { - "__ref": "bar", - }, - }, -} -`; - -exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 2`] = ` -Object { - "bar": Object { - "i": 7, - "j": 8, - "k": 9, - }, - "foo": Object { - "e": 4, - "f": 5, - "g": 6, - "h": Object { - "__ref": "bar", - }, - }, -} -`; - -exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 3`] = ` -Object { - "bar": Object { - "i": 10, - "j": 8, - "k": 9, - }, - "foo": Object { - "e": 4, - "f": 5, - "g": 6, - "h": Object { - "__ref": "bar", - }, - }, -} -`; - -exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 4`] = ` -Object { - "bar": Object { - "i": 10, - "j": 11, - "k": 12, - }, - "foo": Object { - "e": 4, - "f": 5, - "g": 6, - "h": Object { - "__ref": "bar", - }, - }, -} -`; - -exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 5`] = ` -Object { - "bar": Object { - "i": 7, - "j": 8, - "k": 9, - }, - "foo": Object { - "e": 4, - "f": 5, - "g": 6, - "h": Object { - "__ref": "bar", - }, - }, -} -`; - -exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 6`] = ` +exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 6`] = ` Object { "bar": Object { "i": 10, diff --git a/packages/apollo-cache-inmemory/src/__tests__/cache.ts b/packages/apollo-cache-inmemory/src/__tests__/cache.ts index d12b17e4a0e..7b2748c75dd 100644 --- a/packages/apollo-cache-inmemory/src/__tests__/cache.ts +++ b/packages/apollo-cache-inmemory/src/__tests__/cache.ts @@ -24,12 +24,6 @@ describe('Cache', () => { resultCaching: false, }).restore(cloneDeep(data)), ), - initialDataForCaches.map(data => - new InMemoryCache({ - addTypename: false, - freezeResults: true, - }).restore(cloneDeep(data)), - ), ]; cachesList.forEach((caches, i) => { @@ -55,11 +49,6 @@ describe('Cache', () => { ...config, resultCaching: false, }), - new InMemoryCache({ - addTypename: false, - ...config, - freezeResults: true, - }), ]; caches.forEach((cache, i) => { diff --git a/packages/apollo-cache-inmemory/src/__tests__/roundtrip.ts b/packages/apollo-cache-inmemory/src/__tests__/roundtrip.ts index 912de5adef0..a06ceda5f04 100644 --- a/packages/apollo-cache-inmemory/src/__tests__/roundtrip.ts +++ b/packages/apollo-cache-inmemory/src/__tests__/roundtrip.ts @@ -22,7 +22,6 @@ function assertDeeplyFrozen(value: any, stack: any[] = []) { function storeRoundtrip(query: DocumentNode, result: any, variables = {}) { const reader = new StoreReader(); - const immutableReader = new StoreReader({ freezeResults: true }); const writer = new StoreWriter(); const store = writer.writeQueryToStore({ @@ -45,9 +44,9 @@ function storeRoundtrip(query: DocumentNode, result: any, variables = {}) { expect(store).toBeInstanceOf(EntityCache); expect(reader.readQueryFromStore(readOptions)).toBe(reconstructedResult); - const immutableResult = immutableReader.readQueryFromStore(readOptions); + const immutableResult = reader.readQueryFromStore(readOptions); expect(immutableResult).toEqual(reconstructedResult); - expect(immutableReader.readQueryFromStore(readOptions)).toBe(immutableResult); + expect(reader.readQueryFromStore(readOptions)).toBe(immutableResult); if (process.env.NODE_ENV !== 'production') { try { // Note: this illegal assignment will only throw in strict mode, but that's @@ -234,8 +233,8 @@ describe('roundtrip', () => { }, ); - // Just because we read from the store using { freezeResults: true }, the - // original data should not be frozen. + // Reading immutable results from the store does not mean the original + // data should get frozen. expect(Object.isExtensible(updateClub)).toBe(true); expect(Object.isFrozen(updateClub)).toBe(false); }); diff --git a/packages/apollo-cache-inmemory/src/__tests__/writeToStore.ts b/packages/apollo-cache-inmemory/src/__tests__/writeToStore.ts index c9a8855dd3b..674e97b0ff8 100644 --- a/packages/apollo-cache-inmemory/src/__tests__/writeToStore.ts +++ b/packages/apollo-cache-inmemory/src/__tests__/writeToStore.ts @@ -20,6 +20,7 @@ import { StoreWriter } from '../writeToStore'; import { defaultNormalizedCacheFactory } from '../entityCache'; import { makeReference } from '../helpers'; +import { InMemoryCache } from '../inMemoryCache'; export function withWarning(func: Function, regex?: RegExp) { let message: string = null as never; @@ -1862,4 +1863,40 @@ describe('writing to the store', () => { }, }); }); + + it('should not deep-freeze scalar objects', () => { + const query = gql` + query { + scalarFieldWithObjectValue + } + `; + + const scalarObject = { + a: 1, + b: [2, 3], + c: { + d: 4, + e: 5, + }, + }; + + const cache = new InMemoryCache(); + + cache.writeQuery({ + query, + data: { + scalarFieldWithObjectValue: scalarObject, + }, + }); + + expect(Object.isFrozen(scalarObject)).toBe(false); + expect(Object.isFrozen(scalarObject.b)).toBe(false); + expect(Object.isFrozen(scalarObject.c)).toBe(false); + + const result = cache.readQuery({ query }); + expect(result.scalarFieldWithObjectValue).not.toBe(scalarObject); + expect(Object.isFrozen(result.scalarFieldWithObjectValue)).toBe(true); + expect(Object.isFrozen(result.scalarFieldWithObjectValue.b)).toBe(true); + expect(Object.isFrozen(result.scalarFieldWithObjectValue.c)).toBe(true); + }); }); diff --git a/packages/apollo-cache-inmemory/src/inMemoryCache.ts b/packages/apollo-cache-inmemory/src/inMemoryCache.ts index 6b2303d02fd..f2a5e91727c 100644 --- a/packages/apollo-cache-inmemory/src/inMemoryCache.ts +++ b/packages/apollo-cache-inmemory/src/inMemoryCache.ts @@ -20,14 +20,12 @@ import { KeyTrie } from 'optimism'; export interface InMemoryCacheConfig extends ApolloReducerConfig { resultCaching?: boolean; - freezeResults?: boolean; } const defaultConfig: InMemoryCacheConfig = { dataIdFromObject: defaultDataIdFromObject, addTypename: true, resultCaching: true, - freezeResults: false, }; export function defaultDataIdFromObject(result: any): string | null { @@ -92,7 +90,6 @@ export class InMemoryCache extends ApolloCache { this.storeReader = new StoreReader({ cacheKeyRoot: this.cacheKeyRoot, - freezeResults: config.freezeResults, possibleTypes: this.possibleTypes, }); diff --git a/packages/apollo-cache-inmemory/src/readFromStore.ts b/packages/apollo-cache-inmemory/src/readFromStore.ts index 2d15cdab812..dc63a03154f 100644 --- a/packages/apollo-cache-inmemory/src/readFromStore.ts +++ b/packages/apollo-cache-inmemory/src/readFromStore.ts @@ -98,17 +98,14 @@ type ExecSubSelectedArrayOptions = { type PossibleTypes = import('./inMemoryCache').InMemoryCache['possibleTypes']; export interface StoreReaderConfig { cacheKeyRoot?: KeyTrie; - freezeResults?: boolean; possibleTypes?: PossibleTypes; } export class StoreReader { - private freezeResults: boolean; private possibleTypes?: PossibleTypes; constructor({ cacheKeyRoot = new KeyTrie(canUseWeakMap), - freezeResults = false, possibleTypes, }: StoreReaderConfig = {}) { const { @@ -117,7 +114,6 @@ export class StoreReader { executeSubSelectedArray, } = this; - this.freezeResults = freezeResults; this.possibleTypes = possibleTypes; this.executeStoreQuery = wrap((options: ExecStoreQueryOptions) => { @@ -391,7 +387,7 @@ export class StoreReader { // defensive shallow copies than necessary. finalResult.result = mergeDeepArray(objectsToMerge); - if (this.freezeResults && process.env.NODE_ENV !== 'production') { + if (process.env.NODE_ENV !== 'production') { Object.freeze(finalResult.result); } @@ -437,9 +433,7 @@ export class StoreReader { if (!field.selectionSet) { if (process.env.NODE_ENV !== 'production') { assertSelectionSetForIdValue(contextValue.store, field, readStoreResult.result); - if (this.freezeResults) { - maybeDeepFreeze(readStoreResult); - } + maybeDeepFreeze(readStoreResult); } return readStoreResult; } @@ -525,7 +519,7 @@ export class StoreReader { return item; }); - if (this.freezeResults && process.env.NODE_ENV !== 'production') { + if (process.env.NODE_ENV !== 'production') { Object.freeze(array); } diff --git a/packages/apollo-cache-inmemory/src/writeToStore.ts b/packages/apollo-cache-inmemory/src/writeToStore.ts index b7cecaa9a03..7d4a49dbcfc 100644 --- a/packages/apollo-cache-inmemory/src/writeToStore.ts +++ b/packages/apollo-cache-inmemory/src/writeToStore.ts @@ -15,6 +15,7 @@ import { StoreValue, DeepMerger, getTypenameFromResult, + cloneDeep, } from 'apollo-utilities'; import { invariant, InvariantError } from 'ts-invariant'; @@ -233,7 +234,10 @@ export class StoreWriter { context: WriteContext, ): StoreValue { if (!field.selectionSet || value === null) { - return value; + // In development, we need to clone scalar values so that they can be + // safely frozen with maybeDeepFreeze in readFromStore.ts. In production, + // it's cheaper to store the scalar values directly in the cache. + return process.env.NODE_ENV === 'production' ? value : cloneDeep(value); } if (Array.isArray(value)) { diff --git a/packages/apollo-client/src/__tests__/ApolloClient.ts b/packages/apollo-client/src/__tests__/ApolloClient.ts index 025744e6304..1f19be665e9 100644 --- a/packages/apollo-client/src/__tests__/ApolloClient.ts +++ b/packages/apollo-client/src/__tests__/ApolloClient.ts @@ -1329,8 +1329,6 @@ describe('ApolloClient', () => { data, ); const friends = result.data.people.friends; - friends[0].type = 'okayest'; - friends[1].type = 'okayest'; // this should re call next client.writeFragment({ @@ -1344,7 +1342,10 @@ describe('ApolloClient', () => { } `, data: { - friends, + friends: [ + { ...friends[0], type: 'okayest' }, + { ...friends[1], type: 'okayest' }, + ], __typename: 'Person', }, }); diff --git a/packages/apollo-client/src/__tests__/optimistic.ts b/packages/apollo-client/src/__tests__/optimistic.ts index 6a23f68344a..786e2211df2 100644 --- a/packages/apollo-client/src/__tests__/optimistic.ts +++ b/packages/apollo-client/src/__tests__/optimistic.ts @@ -988,13 +988,19 @@ describe('optimistic mutation results', () => { mutation, optimisticResponse, updateQueries: { - todoList: (prev, options) => { + todoList(prev, options) { const mResult = options.mutationResult as any; expect(mResult.data.createTodo.id).toEqual('99'); - - const state = cloneDeep(prev) as any; - state.todoList.todos.unshift(mResult.data.createTodo); - return state; + return { + ...prev, + todoList: { + ...prev.todoList, + todos: [ + mResult.data.createTodo, + ...prev.todoList.todos, + ], + }, + }; }, }, }); @@ -1386,12 +1392,12 @@ describe('optimistic mutation results', () => { }); let firstTime = true; - let before = new Date(); + let before = Date.now(); const promise = client.mutate({ mutation, optimisticResponse, update: (proxy, mResult: any) => { - const after = new Date(); + const after = Date.now(); const duration = after - before; if (firstTime) { expect(duration < 300).toBe(true); @@ -1401,13 +1407,18 @@ describe('optimistic mutation results', () => { } let data = proxy.readQuery({ query }); - data.todoList.todos = [ - mResult.data.createTodo, - ...data.todoList.todos, - ]; proxy.writeQuery({ query, - data, + data: { + ...data, + todoList: { + ...data.todoList, + todos: [ + mResult.data.createTodo, + ...data.todoList.todos, + ], + }, + }, }); }, }); diff --git a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts index 76be63f86c5..d801b057837 100644 --- a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts +++ b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts @@ -1863,7 +1863,10 @@ describe('ObservableQuery', () => { pollInterval: 20, }; - function check({ assumeImmutableResults, freezeResults }) { + function check({ + assumeImmutableResults = true, + assertFrozenResults = false, + }) { const client = new ApolloClient({ link: mockSingleLink( { request: queryOptions, result: { data: { value: 1 } } }, @@ -1871,7 +1874,7 @@ describe('ObservableQuery', () => { { request: queryOptions, result: { data: { value: 3 } } }, ), assumeImmutableResults, - cache: new InMemoryCache({ freezeResults }), + cache: new InMemoryCache(), }); const observable = client.watchQuery(queryOptions); @@ -1881,10 +1884,20 @@ describe('ObservableQuery', () => { observable.subscribe({ next(result) { values.push(result.data.value); - try { - result.data.value = 'oyez'; - } catch (error) { - reject(error); + if (assertFrozenResults) { + try { + result.data.value = 'oyez'; + } catch (error) { + reject(error); + } + } else { + result = { + ...result, + data: { + ...result.data, + value: 'oyez', + }, + }; } client.writeData(result); }, @@ -1896,20 +1909,6 @@ describe('ObservableQuery', () => { }); } - // When we assume immutable results, the next method will not fire as a - // result of destructively modifying result.data.value, because the data - // object is still === to the previous object. This behavior might seem - // like a bug, if you are relying on the mutability of results, but the - // cloneDeep calls required to prevent that bug are expensive. Assuming - // immutability is safe only when you write your code in an immutable - // style, but the benefits are well worth the extra effort. - expect( - await check({ - assumeImmutableResults: true, - freezeResults: false, - }), - ).toEqual([1, 2, 3]); - // When we do not assume immutable results, the observable must do // extra work to take snapshots of past results, just in case those // results are destructively modified. The benefit of that work is @@ -1920,7 +1919,7 @@ describe('ObservableQuery', () => { expect( await check({ assumeImmutableResults: false, - freezeResults: false, + assertFrozenResults: false, }), ).toEqual([1, 'oyez', 2, 'oyez', 3, 'oyez']); @@ -1933,7 +1932,7 @@ describe('ObservableQuery', () => { // modifications of the result objects will become fatal. Once you // start enforcing immutability in this way, you might as well pass // assumeImmutableResults: true, to prevent calling cloneDeep. - freezeResults: true, + assertFrozenResults: true, }); throw new Error('not reached'); } catch (error) {