Skip to content

Commit

Permalink
Assume keyArgs:false only if *both* read and merge provided. (#5862)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
benjamn authored Jan 28, 2020
1 parent e5ecfd5 commit 8a86adc
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 5 deletions.
4 changes: 2 additions & 2 deletions src/cache/inmemory/__tests__/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -525,7 +525,7 @@ describe("type policies", function () {
expect(cache.extract()).toEqual({
ROOT_QUERY: {
__typename: "Query",
types: [
'types({"from":"A","to":"F"})': [
{
__typename: "TypeA",
},
Expand Down
9 changes: 6 additions & 3 deletions src/cache/inmemory/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
});
Expand Down

0 comments on commit 8a86adc

Please sign in to comment.