Skip to content
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 clientMutationId #2077

Closed
steida opened this issue Aug 30, 2017 · 15 comments
Closed

Remove clientMutationId #2077

steida opened this issue Aug 30, 2017 · 15 comments
Assignees

Comments

@steida
Copy link

steida commented Aug 30, 2017

We should figure out how to remove it. I'm pretty sure it's just a fb-ism. Can you open an issue?

https://twitter.com/kassens/status/903031923827654658

cc @kassens

@steida
Copy link
Author

steida commented Aug 30, 2017

It's somehow used by the code because when I tried to use constant, React key was the same...
https://www.graph.cool/forum/t/clientmutationid/426/3?u=daniel_steigerwald

@omerzach
Copy link
Contributor

omerzach commented Nov 9, 2017

Is there any hack possible to avoid all the boilerplate of parsing this ID and returning it again in every mutation? E.g., can I just include a random string in my mutation responses?

@steida
Copy link
Author

steida commented Nov 9, 2017

https://github.com/este/este/blob/0dce17f0553a0ecb0df0c378695192cd0351e20b/components/withMutation.js#L8

@steida steida closed this as completed Feb 7, 2018
@steida
Copy link
Author

steida commented Feb 7, 2018

I think it was caused by graph.cool

@omerzach
Copy link
Contributor

omerzach commented Feb 7, 2018

Why was this closed? clientMutationId is still all over Relay. If it's not necessary it would be really great to remove it.

I'm happy to try to submit a PR removing it if someone can confirm it's unnecessary. +@leebyron

@steida
Copy link
Author

steida commented Feb 22, 2018

@kassens told me, that it should be removed from Relay. Now it must be used, otherwise, Relay will generate wrong ID for an item with the same text for an unknown reason.

steida added a commit to este/este that referenced this issue Feb 22, 2018
steida added a commit to este/este that referenced this issue Feb 23, 2018
* Prisma, almost done

* Deps

* Always return AuthPayload

* Remove backend/src directory

We don't need it.

* Reorganize backend

* Finish app errors, simplify code.

* Deps

* Use non-nullable types for database and nullable types for backend.

* Add deploy-database script

* Remove trim from Validation.

Single responsibility principle ftw.

* Finally trim strings design pattern figured out

* Comments fml

* Add requestFailed validation error

* invalid to wrongPassword

Invalid was too generic.

* Merge backend into root

- Great simplification
- Universal (aka multi-platform) JavaScript app demands just one common package.json file. Otherwise, the life will be hard to live.
- Flat structure ftw.
- Servers (web and GraphQL) and their dependent modules are written in plain JavaScript with Flow comments. They will be webpacked in Next.js 5.
- Some files has been commented for next step.
- Relay 1.5rc hotfix removed. We don't have nested types and they already merged pull request.
- nodemon watches server files.
- npm scripts refactored

* Ups

* Deps

* Refactor validation

Add chain helper.

* Color 3

* Redesign app error

- Finally, I feel this is the right.
- Much cleaner code and naming.
- Error = DomainError | ValidationError

* Flow 0.65

* Move validation to lib

* Next.js 5

* Fix start script

* Next 5.0.1-canary.1

Fixes style flashing

* Refactor server code

- move all server code to /server
- update nodemon and scripts
- explain why server code is not transpiled
- remove Sentry until I figure out how to use it universally.
- refactor lib, it was to much general

* Refactor DEFAULT_LOCALE to server constant

This is constant, not something what should be set by env.

* Fix production:server:graphql

* Remove APP_VERSION

KISS.

* Remove global HOSTNAME

* Fok this shit

* Remove declare var Intl: Object;

* Redesign environments

- replace babel-plugin-transform-define with babel-plugin-inline-dotenv and babel-plugin-transform-inline-environment-variables
- now we have only one .env file and we set it via yarn env dev or yarn env production
- yarn start:local for test production locally
- some refactoring towards deploy

* Improve npm scripts, fix deploy Close #1420

* Fix yarn relay

* Enforce Page component type

* 5.0.1-canary.2

- 5.0.0 has broken SSR styled JSX
- 5.0.1-canary.4 has broken redirect, it's already fixed in master, not yet released.

* Refactor App to stateless functional component

* Mutation in, Mutate and RelayProvider out

* Deps and readme

* The hell is the other people software

#1466

* Remove unused lib

* Cache invalidation fml

* Refactor errors

* Move pending logic to Mutation

* Deps

* Docker ftw

* Deploy in readme

* Rename and move .graphql files

* run yarn prettier

* Update nodemon depencency

* Flow 0.66

* Update eslint dependency

* Deps

* Close #1466

* Temp fix for graphql-cli/graphql dependency

* dev:relay with watch

* Use websConnection

Always return connection instead of plain list / array for Relay. It's must, because Relay does not have an API for list client state modification. Last but no least, connection is an awesome pattern.

* Update readme

* Close #1469

* Remove styled-jsx dependency since it's requied by Next.js itself

* Progress

- Connection pattern: It's up to the server to set connection params. It can be always parametrized later. Client shall not set first / last to prevent over fetching.
- * as generated pattern: Module namespace import is good for DX.
- Improve dev scripts.
- Put back RelayProvider.
- Refactor app.js, types belong where they are used for functions.

* Deps

dotansimha/graphql-yoga#182

* Put back editor

- UrlQuery has to be object to allow casting
- Note server returns nullable Web, that's exactly what we always want.

* Deps

* Friendlify eslint shit

* Destructuring fml

* Fok that shit

- create web
- simplify things

* Deps

* Relay 1.5 and fix some types

#1470

* Fix dev:relay

It was including graphql types from database and server with strange error that type is already defined. Explicit include is better than brittle exclude.

* Deps

* Fix Flow

* Refactor scripts

It's almost perfect, except we still have to run yarn schema && yarn relay on the other terminal window.

* Finally solve foking clientMutationId curse

There must be input type http://graphql.org/learn/schema/#input-types with explicit clientMutationId and clientMutationId must be unique. It seems that server does not have to return anything.

* Deps

* Helper for clientMutationId

facebook/relay#2077

* yarn schema-relay

* Protect web endpoint

* Clean server/model.graphql

* Deps

* DeleteWebMutation and some refactoring

* Add mutation utils. clientRoot and ensureConnection fml.

* Remove relay script from test script

CI error: Watchman:  Watchman was not found in PATH.

As far I remember, it was Relay issue supposed to be fixed already. Ok, not, fok.
@steida
Copy link
Author

steida commented Apr 7, 2018

Random clientMutationId must be passed to Relay and defined in GraphQL model as well.

@kassens Please check this. That's what happens when I add two items with the same text. This is Relay bug.

screen shot 2018-04-07 at 05 40 38

@taion
Copy link
Contributor

taion commented May 17, 2018

this is gated on #2349

@taion
Copy link
Contributor

taion commented May 17, 2018

it's essentially the same thing as that PR

@alloy
Copy link
Contributor

alloy commented Feb 17, 2020

This is fixed now by #2349 right? (Re-open if not.)

@alloy alloy closed this as completed Feb 17, 2020
@ersinakinci
Copy link

So is this line from the docs inaccurate?

Mutations

Relay uses a common pattern for mutations, where there are root fields on the mutation type with a single argument, input, and where the input and output both contain a client mutation identifier used to reconcile requests and responses.

@sibelius
Copy link
Contributor

Feel free to update

@ersinakinci
Copy link

@sibelius, I saw your response on the other thread, but I don't understand enough to change the docs. The only info I have is basically that one sentence in the doc, a couple of half-answered SO posts suggesting that the "client mutation identifier" means "put a field called clientMutationId into the schema," and the title of the PR that I linked to.

To distill my general confusion into a some concrete questions:

  • What are some concrete examples of how clientMutationId is used, within Relay and externally? The docs simply state that it's used "to reconcile requests and responses," but what does that actually mean?
  • What are some concrete examples of how the generated root id is used, within Relay and externally? The title of Remove the clientMutationId requirement by creating a new root id for each executed mutation #2349 implies that clientMutationId has been replaced by some kind of "root id" mechanism, but do root id's serve the exact same purpose as clientMutationId or do they also take on some other roles?
  • More generally, what is a "root id" and what does it mean that a new "root id" has been created for each executed mutation?
  • If root id's have succeeded clientMutationId's, it doesn't sound like they've completely supplanted them. When would you use a clientMutationId over relying on the default root id mechanism?
  • Does supplying a clientMutationId field on the input type and/or payload type somehow "override" the root id?

If you could help me answer these, I could take a stab at updating the docs.

@sibelius
Copy link
Contributor

What are some concrete examples of how clientMutationId is used, within Relay and externally? The docs simply state that it's used "to reconcile requests and responses," but what does that actually mean?
clientMutationId can be used on server to make sure you don't send the same request twice

clientMutationId is the same as an idempotency key (https://stripe.com/en-br/blog/idempotency)

What are some concrete examples of how the generated root id is used, within Relay and externally?
image
Relay Store (https://medium.com/@sibelius/relay-modern-the-relay-store-8984cd148798) uses normalized data to cache records, when doing a mutation without clientId, the "key" to store the mutation inside the store will be auto generated, so you can access mutation data inside updater

More generally, what is a "root id" and what does it mean that a new "root id" has been created for each executed mutation?
QueryType is an example of root id (

)

If root id's have succeeded clientMutationId's, it doesn't sound like they've completely supplanted them. When would you use a clientMutationId over relying on the default root id mechanism?
This won't accept anything on client side, only server for idempotency

Does supplying a clientMutationId field on the input type and/or payload type somehow "override" the root id?
@robrichard can explain better this change

@robrichard
Copy link
Contributor

@earksiinni @sibelius this is my understanding of the clientMutationId

clientMutationId was a requirement for Relay Classic. As far as I'm aware it is not required for Relay Modern and all references in the docs should be removed to avoid confusion.

I think the title for #2349 is causing some confusion. There was a bug in relay where mutations with identical input caused a conflict in the store. One possible workaround for this bug was to use a clientMutationId like was used in Relay Classic, but that issue is fixed now.

The root id is the id relay generates to store response data in the store. By making it dynamic, instead of static, we no longer have the issue of mutations conflicting. It's an implementation detail of Relay, not something a developer needs to do as part of their mutation code.

Hopefully this clears things up

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

No branches or pull requests

9 participants