graphql-js
graphql-js copied to clipboard
graphql
Fix crash in node when mixing sync/async resolvers
Fixes #3528
Ensures that if executeFields encounters a mix of promises and thrown errors, the promises will be awaited before throwing the error.
This does technically change executeFields to return a rejected promise in this scenario as opposed to throwing the not-null error synchronously, but I figured if any of the resolvers are asynchronous then returning a promise will be expected anyway, and this was better than crashing the process.
Note: I had to add an unhandledRejection event listener myself, as it seems mocha doesn't do it.
- :white_check_mark: login: IvanGoncharov / name: Ivan Goncharov (5bfb9d6788b4d8adce451210b4522b056348355b)
- :x: The commit (bc7049bcb29caf6a88bf4b6b16f7294e3b2314ce). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}
Deploy Preview for compassionate-pike-271cb3 ready!
| Name | Link |
|---|---|
| Latest commit | 5bfb9d6788b4d8adce451210b4522b056348355b |
| Latest deploy log | https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/625f19104a83690008203b68 |
| Deploy Preview | https://deploy-preview-3529--compassionate-pike-271cb3.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @asztal, I'm @github-actions bot happy to help you with this PR 👋
Supported commands
Please post this commands in separate comments and only one per comment:
@github-actions run-benchmark- Run benchmark comparing base and merge commits for this PR@github-actions publish-pr-on-npm- Build package from this PR and publish it on NPM
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@asztal I fixed everything except CLA. Please sign it and this PR is ready to merge
Thanks! My contribution counts as a corporate contribution so I need a director to approve it for me first - it should be soon hopefully 🤞
@IvanGoncharov et al, once this fix lands in main, could we backport it to the 15.x branch? Happy to submit that PR if there aren't any objections and if @asztal doesn't want to do it themselves.
@IvanGoncharov et al, once this fix lands in main, could we backport it to the 15.x branch? Happy to submit that PR if there aren't any objections and if @asztal doesn't want to do it themselves.
Sounds great 👍 This is the exact idea behind the current dev flow.
@asztal - any word on the CLA?
The CLA manager at my company is having trouble using the Corporate CLA console -- I'm not at all familiar with it, so I've asked them to check they've signed up using the correct email address, otherwise just raise a support ticket about it like the message says. Sorry for the delay.
Here's the actual error in case anyone here knows anything about it.
Submitted a PR to backport this fix to 15.x: #3573
@asztal - feel free to submit a similar PR if you like and I can close mine. Just wanted to get the ball rolling on getting this fix in 15.x which is where my team needs it.
@asztal Any news on CLA on your side? If you still experiencing technical issues with the CLA console I can connect to people at the Linux foundation that can hopefully solve it.
Hi @chrskrchr
Sorry about the delays, @asztal has since left and this one's fallen through the cracks a little. We're still chasing CLA approval, it's still stuck with the CLA approval but currently trying to find out if a ticket has been submitted
Hi all,
CLA has now been signed, so hopefully we can make progress on getting this merged.
Hi @saihaj,
I'm not sure if I can. This pr has been created from a fork under the developer's personal account, I don't think I'm able to do that. I could fork his fork and create a new PR, would that affect CLA approval? If it would I can try get in contact with @asztal and ask him to help out but would prefer not to bother him if possible.
Best, Ed
Hopefully I'll have time to rebase the branch tonight. Thanks for getting the CLA sorted @hobby203 🙏
Hi all,
Looks as though we're having issues with the CLA still. The EasyCLA tool won't approve it as @asztal doesn't have any commits on this project, but we know this to be the case as the fix is coming from their own fork and not from within the project. Is this something that's happened before? Not sure how to proceed.
@hobby203 do you want to create PR with these changes and see if your CLA passes?
@saihaj from inside this repo? or from my own fork, I checked but I can't create a new pr using these exact changes but i can try recreating the fix.
@hobby203 - I'm happy to submit a new PR if you're tired of messing with this. I've got backports of this PR to the 15.x branch (#3573) and 16.x (#3576) branches that are passing from a test and CLA perspective - we were just waiting on merging those until this PR was approved and merged.
Just let me know...
@chrskrchr - if everyone is happy with that I think it will be the best course of action. I'm not sure why we're having these problems but we are struggling to make progress on our end with this issue, so if it can be moved forward by taking another approach i'm more than happy to let that happen (:
if everyone is happy with that I think it will be the best course of action.
No problem! I'll go ahead and submit that new PR (giving credit to @asztal for the actual fix) and let the maintainers decide if they're OK with moving forward that way.
A clone of this PR has been submitted as #3651.
@saihaj @IvanGoncharov - can you please look at @hobby203's comment above that gives their blessing for this PR to be superseded by another PR to in order to move past the CLA issues?
Sadly I can't be of much help with CLA bot 🥲 if the original author is fine having the PR from someone else I am fine moving forward with it.
Sadly I can't be of much help with CLA bot 🥲 if the original author is fine having the PR from someone else I am fine moving forward with it.
I'm fine with that, as long as the bug is fixed I'm happy. Apologies for the delays on my end.
@asztal The CLA error
"The commit (https://github.com/graphql/graphql-js/commit/bc7049bcb29caf6a88bf4b6b16f7294e3b2314ce). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket."
normally indicates that there's an issue with your authorship details for the given commit. Please ensure that that commit is authored by the exact same email address that you have approved via the CLA. Here's some instructions on how to change the email address on that commit:
https://stackoverflow.com/a/3042512
