graphql-js
graphql-js copied to clipboard
Fix crash in node when mixing sync/async resolvers (backport of #3529)
Fixes #3528
This PR is a clone of #3529, which is currently held up by CLA approvals. Full credit goes to @asztal for identifying and fixing the underlying issue. @hobby203, the current steward of the original PR, has given their blessing for the PR to be re-created.
@asztal's notes from the original PR:
Ensures that if
executeFieldsencounters a mix of promises and thrown errors, the promises will be awaited before throwing the error.This does technically change
executeFieldsto 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
unhandledRejectionevent listener myself, as it seems mocha doesn't do it.
This PR has been backported to the 15.x branch as #3573 and to the 16.x branch as #3576.
Deploy Preview for compassionate-pike-271cb3 ready!
| Name | Link |
|---|---|
| Latest commit | 5f302345e008c68f39c5bfbdc7392135cb20f367 |
| Latest deploy log | https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62ac983ab95f58000864102e |
| Deploy Preview | https://deploy-preview-3651--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.
Hi @chrskrchr, 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
@saihaj - any idea on the expected turnaround time to get a review from the reviewers group?
@saihaj @n1ru4l - looks like we may have the approvals necessary to merge this, but I myself don't have right to merge. Are either of you able to merge it?
@n1ru4l - apologies for continuing to pester you about this, but does your 👍 on my message mean that you'll take care of merging this? And if so, do you have an ETA on when the PR will be merged and cut into a new release? (and similar questions for the #3573 and #3576 backports of this fix to older versions)
Hey @chrskrchr I would love to merge this one but I need to wait for another reviewer from @graphql/graphql-js-reviewers as per https://github.com/graphql/graphql-js/issues/3382
I can provide an alpha release for this
@github-actions publish-pr-on-npm
@github-actions publish-pr-on-npm
@saihaj The latest changes of this PR are available on NPM as graphql@17.0.0-alpha.1.canary.pr.3651.57364d3f9da445b2bba520d3b886e07dc2af10e2 Note: no gurantees provided so please use your own discretion.
Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-3651
I think we need someone from the foundation to verify that the cla workaround here is viable
I think we need someone from the foundation to verify that the cla workaround here is viable
@yaacovCR - is there anything I can do to help with that verification?
I believe the person who can help us in terms of CLA issues would be @brianwarner but @graphql/tsc might know better.
I suppose we can summarize the issue as follows: the original code contributor for (x seemingly technical reason) cannot sign the CLA but has posted that agreement to a new PR with their changes to be submitted under an author that can sign the CLA.
I've raised this with the TSC on Discord too, thanks for bringing it to our attention.
By the way, Brian is no longer working at the Linux Foundation so he's no longer the right person to ping.
Thank you and thanks for the update.
@benjie - curious if there are any updates on the convo you raised with the TSC?
Sadly I've not heard anything. In my understanding though taking this approach isn't legally sound; I think your options are either to do a clean re-implementation of it (where you reimplement the fix without referencing the previous code) or to solve the underlying CLA issues. Let me know if I can do anything to help on the CLA front.
I think your options are either to do a clean re-implementation of it (where you reimplement the fix without referencing the previous code)
OK - thanks, @benjie. I'll submit a new PR that makes no reference to the original code or PR.