Stop paying attention to previousResult in InMemoryCache. (#5644)

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
src/cache/inmemory/__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

@@ -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 }),
           );
         }
       }
@@ -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;
   }
@@ -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,
     });
   }
@@ -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,
       }),
     );

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<QueryType>(
     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<T>({
     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,