Return mutation result as entity object

[lukasluecke]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes (modified!)
Fixed tickets	#2605
License	MIT
Doc PR	-

See #2605 for details.

[lukasluecke]

@alanpoulain Currently breaks custom normalization_context for specific mutations - see failing Behat test.

Since we are now re-using the same GraphQL-type, I don't think we can add the fields dynamically. Any ideas?

—
I think it might make sense to not allow setting a normalization context on the mutations, and just using the one set on the entity / query.

[lukasluecke]

@alanpoulain I have modified the failing test, but need some input on wether or not this should be the "correct" behaviour.

[alanpoulain]

@lukasluecke I've done some modifications to handle delete like the other mutations (I've changed my mind about it sorry), to manage the normalization context correctly and I've fixed some latent issues. I will push these modifications to your branch today I hope, if you're okay with that.

[lukasluecke]

@alanpoulain Sure, no problem at all 🙂

[alanpoulain]

Done. The changes:

• new types to handle different normalization contexts: PayloadData (the object inside the output of a mutation) and NestedPayload (when there is a relation in a PayloadData).
• the input / output behavior has been changed (@soyuka if you can validate): when there is no output, it's not possible schema-wise to query any field; when there is no input it's not possible schema-wise to add field in the input (but you can give it an empty input with or without a clientMutationId, in this case there is a null response for the output object and no persist is done).
• the depth issue has been fixed (its value was strange before because it was inside a loop)

@lukasluecke please tell me if you are okay with the changes 🙂

[lukasluecke]

@alanpoulain I fear that adding these new wrapper types might lead back to the original issue, because the __typename of the returned objects will be different.

Apollo client, which is afaik the most popular JS client, would not "match" this for caching purposes

By default, InMemoryCache will attempt to use the commonly found primary keys of id and _id for the unique identifier if they exist along with __typename on an object
https://www.apollographql.com/docs/react/advanced/caching.html

The Relay spec is not totally clear about this, but from my understanding it also expects the "real" type (and full object) to get returned, not just a "similar" object.

Which lead me to my earlier comment:

I think it might make sense to not allow setting a normalization context on the mutations

I'll leave this decision up to you, but personally I'd prefer ease of use over the marginal flexibility.

[lukasluecke]

It only says that you need to query all the fields that changed, in order to get them updated in the cache. The problem is that the `__typename` is part of the cache key in Apollo, which means that using different types will break it . That is also the reason why the issue currently exists (pre-PR), since all the other fields of the object have always been there and identical (including id).

…

Am 21.03.2019 um 15:57 schrieb Alan Poulain ***@***.***>: You're right, this is really not clear. But in this part: https://www.apollographql.com/docs/react/advanced/caching.html#automatic-updates <https://www.apollographql.com/docs/react/advanced/caching.html#automatic-updates>, this sentence seems to say the output of a mutation could be different from the queried data: One nice way to take advantage of this property as much as possible is to make your mutation results have all of the data necessary to update the queries previously fetched.. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2612 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGWC_p59wdvAySfrxx7akJVGwb0mlKZaks5vY53bgaJpZM4btSrd>.

[alanpoulain]

I deleted my previous comment because it was wrong indeed. Maybe a solution would be to change the type only if the normalization context is different. WDYT?

[lukasluecke]

I think that's a good idea! 👍 If you have time to make the changes, I'd appreciate it - otherwise I'll try to squeeze them in next week 🙂

We should also __typename to the Behat test queries to make sure this is enforced.

[alanpoulain]

Done 🙂

[alanpoulain]

Thank you @lukasluecke!