-
Notifications
You must be signed in to change notification settings - Fork 29
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
Support for Promises #6
Comments
Is that really needed though? As it stands, the callbacks, and the whole code in this package, is executed synchronously. There are no Node async calls or 3rd party promise based code triggered anywhere. Functions that accept callback also return the result or an error message as a string. To be honest, I'm wondering more why are there even callbacks in the first place, rather than exceptions being thrown on errors, and just the result in normal circumstances. |
Hey @boenrobot I hadn't actually looked into the implementation. If there's a way to make this lib synchronous (instead of promisifying it), that would also be an improvement. |
It already is, that's my point. As the read me says near the bottom, the callback arguments are optional, and if not provided, either the result will be returned, or a string that is the error message. Even if passed though, they are still executed on the same tick. If you dig through the source, you'll notice that the return value of the callback is actually the return value of the function accepting it, and there's a default callback that just returns the error or the result. I hate callbacks and prefer promises too, but for this lib, since it's sync anyway, it's better to just not use callbacks. |
Hi @TassosD and @pauliusuza. This is a great library!
I'd be willing to open a PR to add support for Promises.
Would that be a welcome contribution?
The text was updated successfully, but these errors were encountered: