fix: improve handling of promises in defaultTypeResolver

[hayes]

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.

[yaacovCR]

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?

[hayes]

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.

[yaacovCR]

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]

A side channel might be useful as well in context of #2974 to report the first error to client and potentially log all errors

[yaacovCR]

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

[github-actions]

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

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	3c32518
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/66b38a477eb5d200073f0f89
😎 Deploy Preview	https://deploy-preview-3494--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 configuration.

[hayes]

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

[yaacovCR]

There are 2 different questions:

1. Should there ever be unhandled promise rejections?
2. Must all errors be reported?

There probably should never be unhandled promise rejections, but we do not necessarily need to report all errors.

[hayes]

Forgot about this PR, but wanted to check in and see if this can get merged

[JoviDeCroock]

This looks like the right change to me in terms of error-resilience - there is an unwanted commit in the history here though

[hayes]

Good catch! I removed the extra commit and rebased the pr

[JoviDeCroock]

@hayes is the intention of landing this in 16? Else we might need to target the 16.x.x branch.

CC @yaacovCR are you okay with the changes as they are currently lined out, wanted to follow up on your concerns.

[hayes]

@JoviDeCroock I don't feel strongly about which release this makes it into, for me its not urgent, it just seems like it would be good to land eventually