From 29c0fa18b923153d45945d22b3011f179ce4d502 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 1 Mar 2019 17:27:38 -0500 Subject: [PATCH] Avoid costly cloneDeep calls if { assumeImmutableResults: true }. Part of the plan I outlined in this comment: https://github.com/apollographql/apollo-client/issues/4464#issuecomment-467548798 Passing { assumeImmutableResults: true } to the ApolloClient constructor should probably always be accompanied by passing { freezeResults: true } to the InMemoryCache constructor (see #4514), though of course the use of InMemoryCache is optional, and other cache implementations may not support that option. --- packages/apollo-client/src/ApolloClient.ts | 9 ++ .../apollo-client/src/core/ObservableQuery.ts | 6 +- .../apollo-client/src/core/QueryManager.ts | 4 + .../src/core/__tests__/ObservableQuery.ts | 102 +++++++++++++++++- 4 files changed, 118 insertions(+), 3 deletions(-) diff --git a/packages/apollo-client/src/ApolloClient.ts b/packages/apollo-client/src/ApolloClient.ts index c7541138a86..25d34cabcb8 100644 --- a/packages/apollo-client/src/ApolloClient.ts +++ b/packages/apollo-client/src/ApolloClient.ts @@ -55,6 +55,7 @@ export type ApolloClientOptions = { connectToDevTools?: boolean; queryDeduplication?: boolean; defaultOptions?: DefaultOptions; + assumeImmutableResults?: boolean; resolvers?: Resolvers | Resolvers[]; typeDefs?: string | string[] | DocumentNode | DocumentNode[]; fragmentMatcher?: FragmentMatcher; @@ -103,6 +104,12 @@ export default class ApolloClient implements DataProxy { * options supplied to `watchQuery`, `query`, or * `mutate`. * + * @param assumeImmutableResults When this option is true, the client will assume results + * read from the cache are never mutated by application code, + * which enables substantial performance optimizations. Passing + * `{ freezeResults: true }` to the `InMemoryCache` constructor + * can help enforce this immutability. + * * @param name A custom name that can be used to identify this client, when * using Apollo client awareness features. E.g. "iOS". * @@ -120,6 +127,7 @@ export default class ApolloClient implements DataProxy { connectToDevTools, queryDeduplication = true, defaultOptions, + assumeImmutableResults = false, resolvers, typeDefs, fragmentMatcher, @@ -244,6 +252,7 @@ export default class ApolloClient implements DataProxy { version: clientAwarenessVersion!, }, localState: this.localState, + assumeImmutableResults, onBroadcast: () => { if (this.devToolsHookCb) { this.devToolsHookCb({ diff --git a/packages/apollo-client/src/core/ObservableQuery.ts b/packages/apollo-client/src/core/ObservableQuery.ts index 34ad1a644e4..92d421a4556 100644 --- a/packages/apollo-client/src/core/ObservableQuery.ts +++ b/packages/apollo-client/src/core/ObservableQuery.ts @@ -250,7 +250,8 @@ export class ObservableQuery< if (!partial) { this.lastResult = { ...result, stale: false }; - this.lastResultSnapshot = cloneDeep(this.lastResult); + this.lastResultSnapshot = this.queryManager.assumeImmutableResults + ? this.lastResult : cloneDeep(this.lastResult); } return { ...result, partial }; @@ -611,7 +612,8 @@ export class ObservableQuery< const observer: Observer> = { next: (result: ApolloQueryResult) => { this.lastResult = result; - this.lastResultSnapshot = cloneDeep(result); + this.lastResultSnapshot = this.queryManager.assumeImmutableResults + ? result : cloneDeep(result); this.observers.forEach(obs => obs.next && obs.next(result)); }, error: (error: ApolloError) => { diff --git a/packages/apollo-client/src/core/QueryManager.ts b/packages/apollo-client/src/core/QueryManager.ts index e3f8765db18..3e89c572f42 100644 --- a/packages/apollo-client/src/core/QueryManager.ts +++ b/packages/apollo-client/src/core/QueryManager.ts @@ -57,6 +57,7 @@ export class QueryManager { public mutationStore: MutationStore = new MutationStore(); public queryStore: QueryStore = new QueryStore(); public dataStore: DataStore; + public readonly assumeImmutableResults: boolean; private deduplicator: ApolloLink; private queryDeduplication: boolean; @@ -94,6 +95,7 @@ export class QueryManager { ssrMode = false, clientAwareness = {}, localState, + assumeImmutableResults, }: { link: ApolloLink; queryDeduplication?: boolean; @@ -102,6 +104,7 @@ export class QueryManager { ssrMode?: boolean; clientAwareness?: Record; localState?: LocalState; + assumeImmutableResults?: boolean; }) { this.link = link; this.deduplicator = ApolloLink.from([new Deduplicator(), link]); @@ -111,6 +114,7 @@ export class QueryManager { this.clientAwareness = clientAwareness; this.localState = localState || new LocalState({ cache: store.getCache() }); this.ssrMode = ssrMode; + this.assumeImmutableResults = !!assumeImmutableResults; } /** diff --git a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts index f21d93fcb8b..9e1b23495ff 100644 --- a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts +++ b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts @@ -49,7 +49,11 @@ describe('ObservableQuery', () => { const createQueryManager = ({ link }: { link?: ApolloLink }) => { return new QueryManager({ link: link || mockSingleLink(), - store: new DataStore(new InMemoryCache({ addTypename: false })), + assumeImmutableResults: true, + store: new DataStore(new InMemoryCache({ + addTypename: false, + freezeResults: true, + })), }); }; @@ -1690,6 +1694,102 @@ describe('ObservableQuery', () => { }); }); + describe('assumeImmutableResults', () => { + it('should prevent costly (but safe) cloneDeep calls', async () => { + const queryOptions = { + query: gql` + query { + value + } + `, + pollInterval: 20, + }; + + function check({ assumeImmutableResults, freezeResults }) { + const client = new ApolloClient({ + link: mockSingleLink( + { request: queryOptions, result: { data: { value: 1 } } }, + { request: queryOptions, result: { data: { value: 2 } } }, + { request: queryOptions, result: { data: { value: 3 } } }, + ), + assumeImmutableResults, + cache: new InMemoryCache({ freezeResults }), + }); + + const observable = client.watchQuery(queryOptions); + const values = []; + + return new Promise((resolve, reject) => { + observable.subscribe({ + next(result) { + values.push(result.data.value); + try { + result.data.value = 'oyez'; + } catch (error) { + reject(error); + } + client.writeData(result); + }, + error(err) { + expect(err.message).toMatch(/No more mocked responses/); + resolve(values); + }, + }); + }); + } + + // 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 + // that such mutations can be detected, which is why "oyez" appears + // in the list of values here. This is a somewhat indirect way of + // detecting that cloneDeep must have been called, but at least it + // doesn't violate any abstractions. + expect( + await check({ + assumeImmutableResults: false, + freezeResults: false, + }), + ).toEqual([1, 'oyez', 2, 'oyez', 3, 'oyez']); + + async function checkThrows(assumeImmutableResults) { + try { + await check({ + assumeImmutableResults, + // No matter what value we provide for assumeImmutableResults, if we + // tell the InMemoryCache to deep-freeze its results, destructive + // 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, + }); + throw new Error('not reached'); + } catch (error) { + expect(error).toBeInstanceOf(TypeError); + expect(error.message).toMatch( + /Cannot assign to read only property 'value'/, + ); + } + } + await checkThrows(true); + await checkThrows(false); + }); + }); + describe('stopPolling', () => { it('does not restart polling after stopping and resubscribing', done => { const observable = mockWatchQuery(