-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add asyncify method to make async versions of synchronous methods #806
Comments
I'm happy to contribute a better version of this if it's desired. |
We're talking about this in #671 . Still deciding on what the exact name should be.... I'm probably going to pull in the implementation from https://www.npmjs.com/package/acomb#asyncify |
Works for me. I do see some issues with that implementation though. From https://github.com/aearly/acomb/blob/master/index.js#L22:
First, I think it's useful to assert that the callback function is defined in some way. Second, invoking that callback within the
|
You're right, try/catch around the callback is bad. I still think slicing the arguments and always assuming the last arg is a callback is the way to go. It will get weird when you wrap a variadic synchronous function -- it will suddenly have a function that it didn't expect. For example, in the case of |
Yeah, that's a good point. I was not satisfied with the way I extracted the original arguments and callback in my implementation. |
Added in e794801. |
Composing sync and async methods gets a little hairy. Consider a case where you're performing a waterfall of some methods, but one in the middle is synchronous. You have to do something like this:
I think this could be improved with the addition of a simple
asyncify
method. This method could take a synchronous function as an argument and return an asynchronous version. The returned function would invoke the original function or caught errors to the callback. Here is a naive implementation:With this, we can clean up the original code significantly:
Could something like this be added as an async method?
The text was updated successfully, but these errors were encountered: