-
-
Notifications
You must be signed in to change notification settings - Fork 820
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
fix(documents): do not order mutations #6196
fix(documents): do not order mutations #6196
Conversation
🦋 Changeset detectedLatest commit: 7031026 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
e3bd211
to
bdfc4c7
Compare
@martyganz can you please add a changeset? |
bdfc4c7
to
c70f343
Compare
@n1ru4l done! |
@martyganz I think it is still safe to sort the selection set on a mutation fields selection set, so we should still do that and only retain the order of the fields on the root mutation type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please apply the feedback from #6196 (comment) ?
@n1ru4l I'm not sure it's safe to sort sub fields in mutations. Not sure we can make the assumption that mutations actually happens only at root level. Some APIs are using namespaces to organize mutations in logical groups. |
@EmrysMyrddin GraphQL implementations should only run mutation root field resolvers in sequence. All other fields are run in parallel. See https://spec.graphql.org/October2021/#sec-Mutation |
I have the same concern with @n1ru4l . Sorting should be applied in the nested selection sets. By the way, I know it's an edge case but we still need to consider root fragment spreads for Mutation; fragment SomeMutationFragment on Mutation { # Should not be sorted as well
a
b
}
mutation {
fooMutation {
...SomeMutationFragment
}
} We don't have to cover it now though. Just saying haha :D |
🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Description
printExecutableGraphQLDocument sorts mutations by name whereas from the GraphQL spec the order of mutation matters, they do not run in parallel like queries, but in series. See: graphql.org/learn/queries/#multiple-fields-in-mutations.
This PR makes sure that mutations are not sorted and thus if you have a mutation where order matters they will run as
you expect them to.
Related #6195
Type of change
Please delete options that are not relevant.
I was in doubt if it is a breaking change, but I think that anyone using the package currently without it breaking their workflow should be fine with this bugfix.
Screenshots/Sandbox (if appropriate/relevant):
N/A
How Has This Been Tested?
Linked locally to our project and tested if the fix worked.
Test Environment:
OS: MacOS
"@graphql-tools/documents@^1.0.0"::
NodeJS: LTS Iron (v20) - Now using node v20.12.2 (npm v10.5.0)
Checklist:
CONTRIBUTING doc and the
style guidelines of this project
Further comments
Linked issue in graphql-code-generator: dotansimha/graphql-code-generator#9925
PR I made to graphql-code-generator to provide an alternative: dotansimha/graphql-code-generator#9926 - here I was asked to create a PR here ;)
Open to any comments, remarks as I'm very well aware I did not discuss the solution before proceeding to create a PR. Let me know :)