From 42dafcf0216b7ebcd35f1656d4703ed01fc116ea Mon Sep 17 00:00:00 2001
From: Ben Newman <ben@apollographql.com>
Date: Sat, 25 Jan 2020 13:58:31 -0500
Subject: [PATCH] Assume keyArgs:false only if *both* read and merge provided.

Follow-up to #5680.

Assuming keyArgs:false just because a merge function (or a read function)
was defined means that the following pattern can actually change cache
behavior in surprising ways:

  const cache = new InMemoryCache({
    typePolicies: {
      Person: {
        fields: {
          friends: {
            // This is supposed to be identical to what the cache does by
            // default, except without warnings about data loss, but it
            // subtly alters the default argument-based field identity
            // behavior when arguments are provided to the friends field.
            merge(_, incoming) {
              return incoming;
            },
          },
        },
      },
    },
  });

Although this merge function mirrors how the cache behaves without a merge
function, it has the benefit of making that behavior explicit, thereby
silencing the warnings introduced by #5833. In order to recommend this
strategy for silencing warnings when a last-write-wins merge strategy is
desired, it seems unacceptable for this pattern to have any unexpected
consequences beyond silencing the warnings.

Thinking about this default keyArgs:false behavior more, disabling field
identity with keyArgs:false doesn't really make sense if you have only a
merge function, or only a read function, since any fancy argument handling
done by one of those functions cannot be balanced in the other direction.
---
 src/cache/inmemory/__tests__/policies.ts | 4 ++--
 src/cache/inmemory/policies.ts           | 9 ++++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts
index e65723a56df..d520ebf5711 100644
--- a/src/cache/inmemory/__tests__/policies.ts
+++ b/src/cache/inmemory/__tests__/policies.ts
@@ -397,7 +397,7 @@ describe("type policies", function () {
       expect(result).toEqual(data);
     });
 
-    it("assumes keyArgs:false when read or merge function present", function () {
+    it("assumes keyArgs:false when read and merge function present", function () {
       const cache = new InMemoryCache({
         typePolicies: {
           TypeA: {
@@ -525,7 +525,7 @@ describe("type policies", function () {
       expect(cache.extract()).toEqual({
         ROOT_QUERY: {
           __typename: "Query",
-          types: [
+          'types({"from":"A","to":"F"})': [
             {
               __typename: "TypeA",
             },
diff --git a/src/cache/inmemory/policies.ts b/src/cache/inmemory/policies.ts
index 3d93bd7e780..22333fc0e37 100644
--- a/src/cache/inmemory/policies.ts
+++ b/src/cache/inmemory/policies.ts
@@ -324,9 +324,12 @@ export class Policies {
             if (typeof merge === "function") existing.merge = merge;
           }
 
-          if (existing.read || existing.merge) {
-            // If we have a read or merge function, assume keyArgs:false
-            // unless existing.keyFn has already been explicitly set.
+          if (existing.read && existing.merge) {
+            // If we have both a read and a merge function, assume
+            // keyArgs:false, because read and merge together can take
+            // responsibility for interpreting arguments in and out. This
+            // default assumption can always be overridden by specifying
+            // keyArgs explicitly in the FieldPolicy.
             existing.keyFn = existing.keyFn || simpleKeyArgsFn;
           }
         });