graphql-js icon indicating copy to clipboard operation
graphql-js copied to clipboard

fix: improve handling of promises in defaultTypeResolver

Open hayes opened this issue 4 years ago • 8 comments

Currently it is possible to have unhandled promise rejections that arise from a mix of sync and async isTypeOf checks.

This should fix that issue.

hayes avatar Feb 11 '22 01:02 hayes

This means that a mixture of sync/async calls becomes sync unnecessarily, as we return sync value…. Maybe catch handlers should be added to the promises so they reject silently? Is that possible?

yaacovCR avatar Feb 11 '22 05:02 yaacovCR

I had considered that, but silently swallowing errors seems like a bad idea. I think it makes sense for execution to become async if any async functions are called. This should be a pretty rare case, but if these errors were swallowed silently it could become very tricky to debug.

hayes avatar Feb 11 '22 06:02 hayes

Maybe there should be a side channel for logging these errors then so as they are not silent? In fact, this can be addressed currently in user space, as long as isTypeOf functions catch their errors and log them…

this differs from field or type resolvers which must never reject, but it’s not quite a problem for an isTypeOf resolver fail if another succeeds.

not 100% sure what the right answer is

yaacovCR avatar Feb 11 '22 09:02 yaacovCR

A side channel might be useful as well in context of https://github.com/graphql/graphql-js/issues/2974 to report the first error to client and potentially log all errors

yaacovCR avatar Feb 11 '22 10:02 yaacovCR

But as there currently is no side channel, I think I would approve these changes, and then refactor later

yaacovCR avatar Feb 11 '22 10:02 yaacovCR

The latest changes of this PR are available on NPM as graphql@16.3.0-canary.pr.3494.25e1e8c8de6b13288ac61febf75d6f415f81a044 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-3494

github-actions[bot] avatar Feb 17 '22 12:02 github-actions[bot]

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
Latest commit 68a04f2b9a69d9f0ced6c555e45cee5bae5aef2b
Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/64fddef2b40a23000876ade7
Deploy Preview https://deploy-preview-3494--compassionate-pike-271cb3.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Sep 10 '23 02:09 netlify[bot]

kinda forgot about this PR, but I think this change would still be good to get in

hayes avatar Sep 10 '23 02:09 hayes