-
Notifications
You must be signed in to change notification settings - Fork 118
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
Revise Promise support #52
Comments
I'm not opposed to falling back to promises if a callback is absent. Should be easy and doesn't break compatibility afaik. But - I'd point out that the "core" code that switches between callbacks and promises is quite small and localised ( We'll definitely always support callbacks I think - if for no other reason than backwards compatibility. |
@bobjflong Alright. I see what you're saying. I'll look more into that sometime next week and see if can be cleaned up some to use nodeify... and if so, send a PR. |
When using the
We're using node 10.26, with bluebird. If I promisifyAll on the models, I get acceptable errors. But when using |
@Nathan219 thanks for raising this. Would you mind checking out this branch? #64 I plan on removing |
Co-authored-by: Nadia Zhuk <nadia.zhuk@intercom.io>
I would recommend copying Stripe's approach to supporting Promises rather than the current
usePromises()
approach. Typically, you would just have it return a promise unless there's a callback passed in. You also can use Bluebird's nodeify function or just a separate node module to handle this for you- https://www.npmjs.com/package/nodeify- and just stick to Promises in the core- wouldn't recommend anything different tbh.It's also acceptable to just expose Promises and let the developer using the module nodeify it / wrap it if they prefer callbacks, imo (same with callbacks, just that Promises are the way things are headed towards).
Thoughts? @bobjflong
The text was updated successfully, but these errors were encountered: