The check for whether asyncFunction result is a promise is too strict

[unframework]

Looking at this line https://github.com/slorber/react-async-hook/blob/master/src/index.ts#L249 there is a condition if (promise instanceof Promise) { ... }. That causes a failure in one of my project's Jest tests, because any mocked rejections end up being instantiated with some different constructor, so they are misclassified as synchronous successful result.

One could argue that Jest is misbehaving by using a weird alternate-universe Promise constructor, but it should be a moot point because per the spec notes https://promisesaplus.com/#the-promise-resolution-procedure any object with a then method should be treated as a promise because there are many legacy wrapper libraries/polyfills/etc out there.

That being said, even checking for the "then" method is not enough. There is one other problem that can happen when a synchronous function is passed in to e.g. useAsyncCallback - if it throws immediately then that error crashes the app instead of being caught and safely reported.

To fix both of these problems, I would recommend wrapping the asyncFunction.apply(void 0, args) expression in var promise = new Promise(resolve => resolve(asyncFunction.apply(void 0, args))). This would catch any sync errors but otherwise report any results as usual. In that case, there is no need to check for whether the result is a promise (it always is).

The only downside is that the loading state is set for at least a single tick, but that seems acceptable, since synchronous functions are a rare corner case anyway. The ability to catch synchronous errors (which might happen in functions that otherwise return a promise) would be a great safety feature though. And it would resolve the issue with exotic mocks like the one I am getting from Jest, hehe.

Thanks for your hard work Sebastian, hopefully this feedback can be useful!

[unframework]

By the way, for this specific situation with Jest, it is a known issue (jestjs/jest#6645), that is likely due to this (jestjs/jest#2549).

[slorber]

Hi, sorry for the delay I have not much time to update this project currently.

Would you mind opening a PR?

[G-Rath]

This could be what OP is experiencing (and is whats detailed in the linked Jest issues), but here's an example of what I'm hitting in jest for clarity:

fetchPullRequestStatsMock.mockImplementation(async () => {
  return {
    numberOfPullRequestsClosed: 3,
    averageTimePullRequestsSpendOpen: 1234
  };
});

fetchPullRequestStatsMock.mockResolvedValue({
  numberOfPullRequestsClosed: 3,
  averageTimePullRequestsSpendOpen: 1234
});

The above result in different outcomes since mockImplementation results in an instance of Promise, whereas the second results in a spec-compliant Promise but that is not an instance of the Promise class.

I'm happy to help get the PR fixing this landed :)

[slorber]

I've simplified the impl to support uniformily:

• synchronous callbacks
• errors thrown synchronously
• promise-likes are converted to regular promises (added a test with mockResolvedValue)