Skip to content

Commit

Permalink
Stop paying attention to previousResult in InMemoryCache.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
benjamn committed Dec 3, 2019
1 parent b709e86 commit 8c622bb
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 28 deletions.
22 changes: 7 additions & 15 deletions src/cache/inmemory/inMemoryCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,20 +96,15 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
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 }),
);
}
}
Expand All @@ -136,7 +131,6 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
query: options.query,
variables: options.variables,
rootId: options.rootId,
previousResult: options.previousResult,
config: this.config,
}) || null;
}
Expand All @@ -159,7 +153,6 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
query: options.query,
variables: options.variables,
returnPartialData: options.returnPartialData,
previousResult: options.previousResult,
config: this.config,
});
}
Expand Down Expand Up @@ -305,7 +298,6 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
this.diff({
query: c.query,
variables: c.variables,
previousResult: c.previousResult && c.previousResult(),
optimistic: c.optimistic,
}),
);
Expand Down
13 changes: 0 additions & 13 deletions src/cache/inmemory/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<QueryType>(
options: ReadQueryOptions,
Expand All @@ -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<T>({
store,
query,
variables,
previousResult,
returnPartialData = true,
rootId = 'ROOT_QUERY',
config,
Expand Down Expand Up @@ -205,12 +198,6 @@ export class StoreReader {
});
}

if (previousResult) {
if (isEqual(previousResult, execResult.result)) {
execResult.result = previousResult;
}
}

return {
result: execResult.result,
complete: !hasMissingFields,
Expand Down

0 comments on commit 8c622bb

Please sign in to comment.