From 8c622bbaa7d6a40115e000f83855d9de45acfa92 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 27 Nov 2019 17:30:22 -0500 Subject: [PATCH] Stop paying attention to previousResult in InMemoryCache. The previousResult option was originally a way to ensure referential identity of structurally equivalent cache results, before the result caching system was introduced in #3394. It worked by returning previousResult whenever it was deeply equal to the new result. The result caching system works a bit differently, and in particular never needs to do a deep comparison of results. However, there were still a few (test) cases where previousResult seemed to have a positive effect, and removing it seemed like a breaking change, so we kept it around. In the meantime, the equality check has continued to waste CPU cycles, and the behavior of previousResult has undermined other improvements, such as freezing cache results (#4514). Even worse, previousResult effectively disabled an optimization that allowed InMemoryCache#broadcastWatches to skip unchanged queries (see comments I removed if curious). This commit restores that optimization. I realized eliminating previousResult might finally be possible while working on PR #5617, which made the result caching system more precise by depending on IDs+fields rather than just IDs. This additional precision seems to have eliminated the few remaining cases where previousResult had any meaningful benefit, as evidenced by the lack of any test changes in this commit... even among the many direct tests of previousResult in __tests__/diffAgainstStore.ts! The removal of previousResult is definitely a breaking change (appropriate for Apollo Client 3.0), because you can still contrive cases where some never-before-seen previousResult object just happens to be deeply equal to the new result. Also, it's fair to say that this removal will strongly discourage disabling the result caching system (which is still possible for diagnostic purposes), since we rely on result caching to get the benefits that previousResult provided. --- src/cache/inmemory/inMemoryCache.ts | 22 +++++++--------------- src/cache/inmemory/readFromStore.ts | 13 ------------- 2 files changed, 7 insertions(+), 28 deletions(-) diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index 6aa5c6263d8..50ff65301af 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -96,20 +96,15 @@ export class InMemoryCache extends ApolloCache { return maybeBroadcastWatch.call(this, c); }, { makeCacheKey(c: Cache.WatchOptions) { - if (c.previousResult) { - // If a previousResult was provided, assume the caller would prefer - // to compare the previous data to the new data to determine whether - // to broadcast, so we should disable caching by returning here, to - // give maybeBroadcastWatch a chance to do that comparison. - return; - } - - if (supportsResultCaching(cache.data)) { - // Return a cache key (thus enabling caching) only if we're currently - // using a data store that can track cache dependencies. + // Return a cache key (thus enabling result caching) only if we're + // currently using a data store that can track cache dependencies. + const store = c.optimistic ? cache.optimisticData : cache.data; + if (supportsResultCaching(store)) { + const { optimistic, rootId, variables } = c; return cache.cacheKeyRoot.lookup( c.query, - JSON.stringify(c.variables), + store, + JSON.stringify({ optimistic, rootId, variables }), ); } } @@ -136,7 +131,6 @@ export class InMemoryCache extends ApolloCache { query: options.query, variables: options.variables, rootId: options.rootId, - previousResult: options.previousResult, config: this.config, }) || null; } @@ -159,7 +153,6 @@ export class InMemoryCache extends ApolloCache { query: options.query, variables: options.variables, returnPartialData: options.returnPartialData, - previousResult: options.previousResult, config: this.config, }); } @@ -305,7 +298,6 @@ export class InMemoryCache extends ApolloCache { this.diff({ query: c.query, variables: c.variables, - previousResult: c.previousResult && c.previousResult(), optimistic: c.optimistic, }), ); diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index ec26ae65b65..e3f08d148a9 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -26,7 +26,6 @@ import { getMainDefinition, getQueryDefinition, } from '../../utilities/graphql/getFromAST'; -import { isEqual } from '../../utilities/common/isEqual'; import { maybeDeepFreeze } from '../../utilities/common/maybeDeepFreeze'; import { mergeDeepArray } from '../../utilities/common/mergeDeep'; import { Cache } from '../core/types/Cache'; @@ -141,10 +140,6 @@ export class StoreReader { * * @param {Object} [variables] A map from the name of a variable to its value. These variables can * be referenced by the query document. - * - * @param {any} previousResult The previous result returned by this function for the same query. - * If nothing in the store changed since that previous result then values from the previous result - * will be returned to preserve referential equality. */ public readQueryFromStore( options: ReadQueryOptions, @@ -160,14 +155,12 @@ export class StoreReader { * identify if any data was missing from the store. * @param {DocumentNode} query A parsed GraphQL query document * @param {Store} store The Apollo Client store object - * @param {any} previousResult The previous result returned by this function for the same query * @return {result: Object, complete: [boolean]} */ public diffQueryAgainstStore({ store, query, variables, - previousResult, returnPartialData = true, rootId = 'ROOT_QUERY', config, @@ -205,12 +198,6 @@ export class StoreReader { }); } - if (previousResult) { - if (isEqual(previousResult, execResult.result)) { - execResult.result = previousResult; - } - } - return { result: execResult.result, complete: !hasMissingFields,