Merge function at the type level not working (+ suggested fix)

[mpgon]

Issue Description

Defining a merge function at the type level doesn't seem to be working for me.
With the cache defined as

const cache = new InMemoryCache({
      typePolicies: {
        Patient: {
          merge: true,
        } // ...

I get the warning

Cache data may be lost when replacing the diagnosis field of a Patient object.

And the data is overwritten in the cache.

Note: the diagnosis is a Patient object property with a typename but without an id

but with

const cache = new InMemoryCache({
      typePolicies: {
        Patient: {
          fields: {
            diagnosis: { merge: true },
          },
        } // ...

it works.

My theory is that the parent typename is not considered when doing the merge function lookup. I tried adapting the policies.ts's getMergeFunction to take this into consideration:

public getMergeFunction(/**/) {
    // ..
    let merge = policy && policy.merge;
    if (!merge && childTypename) {
      policy = this.getTypePolicy(childTypename);
      merge = policy && policy.merge;
      // added from here
      if (!merge) {
        policy = this.getTypePolicy(parentTypename);
        merge = policy && policy.merge;
      }
      // to here
    }
    return merge;
  }

and it works as expected = no warnings and fields are all merged.

Any thoughts?

Link to Reproduction

let me know if it's not clear

Reproduction Steps

No response

@apollo/client version

3.9.11

[phryneas]

Patient: { merge: true }

is for merging one patient with another patient.

The error message you are getting here though is about merging patient.diagnosis - it doesn't know how to merge one Diagnosis object with another Diagnosis object.

Have you tried (assuming that a diagnosis is of the Diagnosis type) to define

  Patient: { merge: true },
  Diagnosis: { merge: true },

?

[mpgon]

Thank you for the fast response @phryneas!

is for merging one patient with another patient.

Ah! It looks like I misunderstood the feature. What you suggest indeed works.
But then it doesn't solve the problem I'm having: I'm upgrading a big project from Apollo 2 to 3, and we have a lot of these non-normalized object properties. Isn't there any way to define the equivalent of "merge all non-normalized object properties of this parent type"? Or making merging the default?

[phryneas]

"merge all non-normalized object properties of this parent type"

No, merging happens on a type level - we don't look at parent types, only at fields of a type.

merge all non-normalized object properties of this parent type"

I believe this would be possible with a fuzzy type policy (by creating a virtual supertype this is not very well documented):

const cache = new InMemoryCache({
  possibleTypes: {
    All: [".*"],
  },
  typePolicies: {
    All: {
      merge: true
    }
  }
})

but tbh., I'd want to recommend against that, and urge you to be more explicit - add id fields to your queries where it's possible and add type policies for the few types where it isn't possible.
Going with a "merge all" approach will probably end up in a lot of very weird edge case scenarios.

[mpgon]

That fuzzy type policy works perfectly, thank you!
If I was doing this from scratch I'd probably do as you recommend, but in our specific case we have a lot jsonblobs (without ids) where we have, imagine, a list where you click on an item to see details. In the inverse direction (if you open the details view first, then the list) because the list fetches a much smaller version of the blobs, when going back to the details, the cached version would have lost a lot of data. Here the merge functionality works great. The one by one approach is very error prone, one blob (and future blobs) can be missed very easily.
Please let me know if conceptually there's a better way to handle this in apollo.

[phryneas]

The problem is now that you will miss it if a JSON blog was switched to a completely different JSON blob.

I would recommend that you at least restrict it a bit, e.g. if your JSON blob types all end with ....JSONBlob:

const cache = new InMemoryCache({
  possibleTypes: {
    All: [".*JSONBlob"],
  },
  typePolicies: {
    All: {
      merge: true
    }
  }
})

Alternatively, in your Schema definition you could have all of their types extend an common interface and then use the graphql-codegen to generate a possibleTypes from that, and then add a typePolicy on that interface.

[mpgon]

I see your point. Right now we don't have anything that distinguishes them but maybe we'll add that. I also now understand the spirit of the feature, but after reviewing I don't think we have a use case anywhere where we have non-normalized objects that are switched, only updated, the other cases are always id-normalized. (I would actually think that's the default for non-normalized object types in general, and that there's some exceptions that require replacing)

[phryneas]

Simple example where something will go very wrong easily: inserting an item to a list or removing one.

Query 1 returns:

[ { name: "Tim", height: 185}, { name: "Bob", height: 175 }, { name: "Alice", height: 179 } ]

now you delete one object, retreive the same list, but now with a "lastName" instead of height:

[ { name: "Tim", lastName: "Foo" }, { name: "Alice", lastName: "Bar" } ]

Your cache will probably end up with something like this, with a wrong height for "Alice":

[ { name: "Tim", height: 185,  lastName: "Foo"}, { name: "Alice", height: 175, lastName: "Bar" } ]

I'm just saying... be careful with merge. Adding some kind of UUID to those JSON blobs might save you a lot of headache.

[mpgon]

Oh maybe it's wasn't clear, I'm just talking about plain objects, arrays should definitely be replaced or custom merged. Does the above merge all fuzzy type policy also makes apollo try to merge arrays? If so I can probably replace the true with a function that checks if isArray right?

[phryneas]

It's not so much about "array or not", it's more about a missing knowledge about identity here. It's not about merging two arrays, but about the question if individual array elements are the same object or completely different objects.

But tbh., I'm slightly unsure here if that would really happen with arrays like this - you best give it a try, though, to be sure!

[mpgon]

Got it, my point is more: in our specific context we don't have non-normalized plain objects changing identity.

For arrays (where we wanted the type policy to still replace them):

Query 1 returns:

[ { name: "Tim", height: 185}, { name: "Bob", height: 175 }, { name: "Alice", height: 179 } ]

now you delete one object, retreive the same list, but now with a "lastName" instead of height:

[ { name: "Tim", lastName: "Foo" }, { name: "Alice", lastName: "Bar" } ]

Just tested and for future reference can confirm the resulting array in the cache is

[ { name: "Tim", lastName: "Foo" }, { name: "Alice", lastName: "Bar" } ]

From my side the issue can be closed, thanks for your support @phryneas !

[phryneas]

Okay, closing then :) Happy to help!

[github-actions]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.