apollographql / apollo-feature-requests

🧑‍🚀 Apollo Client Feature Requests | (no 🐛 please).
Other
130 stars 7 forks source link

Parent fields are not available from readField in merge functions #374

Open ab-pm opened 3 years ago

ab-pm commented 3 years ago

The docs at https://www.apollographql.com/docs/react/caching/cache-field-behavior/#handling-pagination state

Note that if you call readField(fieldName), it returns the value of the specified field from the current object. If you pass an object as a second argument to readField, (e.g., readField("id", task)), readField instead reads the specified field from the specified object.

I was surprised that the first form does not work in the readField function passed to a custom merge function - since apollographql/apollo-client#5722 the options should work the same in both read functions and merge functions.

const cache = new InMemoryCache({
    typePolicies: {
        MyEntity: {
            fields: {
                myProperty: {
                    merge(existing, incoming, {readField, args}) {
                        console.log(`Merging MyEntity(${ readField('id') }).myProperty(${ JSON.stringify(args) })`);
//                                                       ^^^^^^^^^^^^^^^
                        …
                    }
                },
            }
        },
    }
});

The use case might be to use the parent object's information, in addition to the field arguments, to figure out whether two objects should be merged or not. (I admit it's probably a bad idea to do that). It's also useful for basic logging (while debugging) as in the snippet above. Unfortunately, the call just returns undefined instead of the entity id.

I'm not quite sure whether this should be a bug report against the library, a feature request (as it would be considered a new feature), or a bug report against the documentation.

benjamn commented 3 years ago

@ab-pm Here's a comment from the source code that attempts to rationalize this decision: https://github.com/apollographql/apollo-client/blob/971ecfcc20be748084c4b769c2a7518845c3919f/src/cache/inmemory/policies.ts#L813-L826 In short, readField('id') is ambiguous in merge functions, whereas there's only one thing it could mean in read functions. That's not an absolute and final answer, but it's a challenge to consider and solve.

The current recommendation is to use the readField("id", ref) style, but I realize it might be tricky to obtain that ref, especially if you can't use readField.

I would be open to exposing an options.entity property to read and merge (and cache.modify) functions, which would always be populated with a Reference or StoreObject representing the parent/enclosing object, making options.entity suitable to be passed as the second argument to readField. Calling readField("id", options.entity) would always read from existing data, and that data might be unreliable depending on the order of cache updates, but informational inspection/logging (as in your example) seems like a fairly harmless use case that would be worth supporting.

What do you think about that?

ab-pm commented 3 years ago

it's not clear what the current object should be for merge functions: the (possibly undefined) existing object, or the incoming object?

I don't understand how those would be choices. It should be the parent object of the field, which is not ambiguous. One can already easily access field values of the existing and incoming objects through readField('id', existing)/readField('id', incoming) (with guards against undefined and array values).

If you think your merge function needs to read sibling fields in order to produce a new value for the current field, you might want to rethink your strategy, because that's a recipe for making merge behavior sensitive to the order in which fields are written into the cache.

Oh, that's a very good point I had not though of. However, I was intending to only read constant properties with primitive values from the parent object, which would never be updated.

I would be open to exposing an options.entity property to read and merge functions, which would always be populated with a Reference or StoreObject representing the parent/enclosing object. What do you think about that?

I'd be very happy to have that available! And it would seem cleaner to always explicitly pass that as the second argument to readField. However, I think that for consistency, the readField(name) (without a reference) should always fall back to that entity, not only inside read functions but also in merge functions.

MichaelGoff commented 1 year ago

I've run into a similar issue in a merge function where I'm getting Undefined 'from' passed to readField with arguments ["id"] when trying to readField('id') without a ref.

Having access to an entity field in the options would resolve my issue as I'm trying to read the parent node out of the cache. In my case, I don't care if this information is stale as the id field will never change.