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	68a04f2
🔍 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... 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.

[yaacovCR]

I think this should probably silently ignore the errors....

Something like:

if (promisedIsTypeOfResults.length > 0) {
  Promise.all(promisedIsTypeOfResults).then(undefined, () => {
    /* ignore errors */
  });
}

[yaacovCR]

Seeing as one functioning isTypeOf resolver is all one should have and all one needs.

Compare also to a sync case with multiple sync isTypeOf resolvers, one that returns true, and multiple that throw instead of returning false. If the order is serendipitous, the non-erroring one will return true and mask all the other potential errors.

In #3706, we convert a sync error to an async error, but that's the not happy path of an error. Here, this change will convert every synchronous isTypeOf to a non-synchronous....

[yaacovCR]

Compare also to multiple async errors, when we return immediately with the first async error, without awaiting the others. I would argue that #3706 is a bit funny, in that if we have a sync error, there is no need for:

return promiseForObject(...).finally(() => throw originalError);

we should just have:

promiseForObject(....).then(undefined, () =>{ /*ignore*/ });
throw originalError;

[hayes]

makes sense, updated