New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the `clientMutationId` requirement by creating a new root id for each executed mutation #2349

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@robrichard
Copy link
Contributor

robrichard commented Feb 26, 2018

Fixes #1734
Fixes #2333
Fixes #825

Related to relayjs/relay-examples#30
When a mutation is executed, it's payload is added to the store using a key of the serialized arguments. In the Todo Modern example this looks like this:

{
  'client:root:addTodo(input:{"text":"a"})': { ... } // added by mutation response
  'client:VXNlcjptZQ==:__TodoList_todos_connection': {
    edges: [
      __refs: [
        'client:VXNlcjptZQ==:__TodoList_todos_connection:edges:0',
        'client:VXNlcjptZQ==:__TodoList_todos_connection:edges:1',
        'client:root:addTodo(input:{"text":"a"}):todoEdge', // added by mutation imperative updater function
      ]
    ]
  }
}

If you execute a second addTodo mutation with the same arguments, the client:root:addTodo(input:{"text":"a"}) field in the store will get overwritten with the second mutation and the store will look like this:

{
  'client:root:addTodo(input:{"text":"a"})': { ... }
  'client:VXNlcjptZQ==:__TodoList_todos_connection': {
    edges: [
      __refs: [
        'client:VXNlcjptZQ==:__TodoList_todos_connection:edges:0',
        'client:VXNlcjptZQ==:__TodoList_todos_connection:edges:1',
        'client:root:addTodo(input:{"text":"a"}):todoEdge',
        'client:root:addTodo(input:{"text":"a"}):todoEdge',
      ]
    ]
  }
}

When React renders the data, you will have the latter todo in the connection array twice and will likely get an error along the lines of Encountered two children with the same key, <id>. Child keys must be unique; when two children share a key, only the first child will be used.

This was fixed in relay-examples by adding a clientMutationId to the mutation arguments. I don't think this is ideal since

  1. it requires the developer to manually add this to each mutation
  2. it places an additional constraint on the GraphQL schema to support this field.

The approach I took to fix this, was to use a new root dataID (instead of the static client:root) for results on each mutation. This will namespace results from each mutation so they can never overwrite one another.

With these changes, the relay store now looks like this after completing the same series of mutations:

{
  'client:mutationRoot0:addTodo(input:{"text":"a"})': { ... },
  'client:mutationRoot1:addTodo(input:{"text":"a"})': { ... },
  'client:VXNlcjptZQ==:__TodoList_todos_connection': {
    edges: [
      __refs: [
        'client:VXNlcjptZQ==:__TodoList_todos_connection:edges:0',
        'client:VXNlcjptZQ==:__TodoList_todos_connection:edges:1',
        'client:mutationRoot0:addTodo(input:{"text":"a"}):todoEdge',
        'client:mutationRoot1:addTodo(input:{"text":"a"}):todoEdge'
      ]
    ]
  }
}

I tested this and it works to solve this issue, but I'm unsure if there could be any unintended consequences by this change.

@robrichard robrichard force-pushed the robrichard:mutationUid branch from 7218478 to 81e0fb7 Feb 26, 2018

robrichard added some commits Feb 26, 2018

@alloy

This comment has been minimized.

Copy link
Collaborator

alloy commented Mar 1, 2018

This PR addresses the issue described in #2333. And while this does not remove the ID, it is related to #2077.

@alloy

This comment has been minimized.

Copy link
Collaborator

alloy commented Mar 1, 2018

@steida Mind testing this PR for your situation? (And is your situation a production app?)

@steida

This comment has been minimized.

Copy link

steida commented Mar 2, 2018

@alloy My situation is https://github.com/este/este. Not a production :(

@jstejada What do you think, should I test? Will Relay team merge it? Thank you.

@steida

This comment has been minimized.

Copy link

steida commented Mar 3, 2018

@kassens What do you think? Should I test it? Is there any chance it will be merged in, say, next six months?

@alloy

This comment has been minimized.

Copy link
Collaborator

alloy commented Mar 3, 2018

@steida We should help the Relay team by testing them preemptively so that when they have time they will know it was already tested by at least somebody.

@taion

This comment has been minimized.

Copy link
Contributor

taion commented Apr 10, 2018

Is there a constructive reason for the old behavior, incidentally? It does seem odd to essentially assume idempotency for all mutations.

@facebook-github-bot
Copy link

facebook-github-bot left a comment

@jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dblock

This comment has been minimized.

Copy link

dblock commented Jul 10, 2018

@AndrewIngram mentions in https://graphql.slack.com/archives/C0BEXJLKG/p1531176878000009 that another work-around until this is merged is to clone the data that comes back. Anyone has such code to share here that, hopefully, avoids the long boilerplate in https://facebook.github.io/relay/docs/en/mutations.html#updater-configs?

@taion

This comment has been minimized.

Copy link
Contributor

taion commented Aug 31, 2018

BTW, the same problem arises for subscriptions. In this case the fix needs to be applied in this manner, as there's no place to inject a payload sequence ID (as far as I'm aware).

That said, @AndrewIngram's workaround is pretty satisfactory in practice.

@sibelius

This comment has been minimized.

Copy link
Collaborator

sibelius commented Oct 19, 2018

@jstejada @kassens can we try to import this again ?

we could also merge this one #2401 when this has landed

@jstejada

This comment has been minimized.

Copy link
Contributor

jstejada commented Oct 29, 2018

thanks for the ping @sibelius! apologies again for the delay in getting to this. We still need to discuss internally a little bit what the best approach would be here. From a short discussion, it seems like we could address separately the problem of ensuring that we don't overwrite mutation payloads vs removing the requirement for a clientMutationID. cc @josephsavona @kassens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment