[asyncify] Make errors in callbacks throw globally#1408
Conversation
|
I’m going to fix the broken tests, but would like feedback on the change first. |
|
I've thought about deferring the callback in Another strategy would be to try/catch the callback, and then re-throw the error on the next tick. This is what Bluebird does: https://github.com/petkaantonov/bluebird/blob/master/src/nodeify.js#L38-L42 Either way, if it involves changing our tests, it's a breaking change, albeit a small one. Also, in the next version of Node, unhandled promise rejections are going to start killing the process, just like an unhandled error. |
|
Re. catching an async error, you can do this: https://github.com/caolan/async/blob/master/mocha_test/waterfall.js#L110-L119 |
|
Thanks for the hints. I updated the PR.
Seems the best way to do it. I adapted the code accordingly.
It is changing behavior, but you could argue it is a fix. The behavior is now the same as when asyncify’ing functions that don’t return promises. There’s even a test for errors to be thrown in the non-promise case. |
If asyncify wraps a function returning promises, the callback will be executed as part of the promise’s `.then()` method. This means that any error thrown from that method will be silenced, and lead to a “unhandled promise rejection” warning in modern engines. Usually, we want these errors to be visible, and even crash the process.
megawac
left a comment
There was a problem hiding this comment.
👍 I consider this a bug fix
|
You're right, after further thought, this is more of a bug fix. It will change how people's code behaves, though. |
|
Released in v2.4.0! |
|
great, thanks for letting me know! |
If asyncify wraps a function returning promises, the callback will be executed as part of the promise’s
.then()method.This means that any error thrown from that method will be silenced, and lead to a “unhandled promise rejection” warning in modern engines.
Usually, we want these errors to be visible, and even crash the process.
I’d like to add a test, but there doesn’t seem to be a good way to do it, especially for both node.js and browsers.