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

adds a field sort by source location before executing serially #493

Merged
merged 3 commits into from
Feb 5, 2020

Conversation

bhoriuchi
Copy link
Contributor

Resolves #145 by sorting fields by source location before executing serially

@coveralls
Copy link

coveralls commented Jul 3, 2019

Coverage Status

Coverage increased (+0.03%) to 92.378% when pulling b6b24cf on bhoriuchi:master into d925012 on graphql-go:master.

@djmcgreal-cc
Copy link

I'd love this to get some attention from reviewers. Not having mutations executed in the correct order means that we go back to needing multiple operation requests to enforce order of a chain of mutations.

Personally, I'd also like to see this configurable to be usable outside of the top level of a mutation operation. Our schema wraps every operation in an authenticate field so it's actually the second level of fields we would like executed in serial.

Perhaps there should also be a test case for this.

@chris-ramon chris-ramon self-requested a review December 7, 2019 23:50
@chris-ramon
Copy link
Member

chris-ramon commented Feb 5, 2020

@bhoriuchi thanks a lot for improving the serially execution order! –– it definitely looks good to me 👍 💯

I've tested it and seems working neat! –– Sharing the gist with the working example: https://gist.github.com/chris-ramon/6c98e31259d1c7837fc3f240ee4f988d

@chris-ramon
Copy link
Member

I'd love this to get some attention from reviewers. Not having mutations executed in the correct order means that we go back to needing multiple operation requests to enforce order of a chain of mutations.

Personally, I'd also like to see this configurable to be usable outside of the top level of a mutation operation. Our schema wraps every operation in an authenticate field so it's actually the second level of fields we would like executed in serial.

Perhaps there should also be a test case for this.

@djmcgreal-cc thanks a lot for the feedback! 👍 –– perhaps could you file an issue and share an example of the query your mentioning, so we could make this PR configurable via a follow-up PR.

Copy link
Member

@chris-ramon chris-ramon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 –– Neat improvement @bhoriuchi 💯 🚢

@chris-ramon chris-ramon merged commit 59637ea into graphql-go:master Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrong execution order of multiple mutations in one request
5 participants