Skip to content
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 promises in addition to continuation-passing #134

Open
ghost opened this issue Jan 27, 2018 · 6 comments
Open

Support promises in addition to continuation-passing #134

ghost opened this issue Jan 27, 2018 · 6 comments
Assignees

Comments

@ghost
Copy link

ghost commented Jan 27, 2018

My reasoning for this is to support promises on Model methods. Maybe even better idea would be to enable this in some higher configuration.
When promisify: true all the methods would be wrapped with bluebird's Promise.promisify(). I assume this could be done without side effects in internals.compileModel within index.js.

The other idea is to support both at the same time as it's done in Joi.validate

When used without a callback, this function returns a Promise-like object that can be used as a promise

@cdhowie
Copy link
Collaborator

cdhowie commented Jan 30, 2018

I agree that natively supporting promises would be nice. I'd suggest instead that the configuration parameter be a promise factory. By default it would be global.Promise but could be replaced with require('bluebird') or require('Q') to use a different promise implementation.

@cdhowie cdhowie self-assigned this Jan 30, 2018
@cdhowie cdhowie changed the title Add promisify property to the ModelConfiguration Support promises in additional to continuation-passing Jan 30, 2018
@cdhowie cdhowie changed the title Support promises in additional to continuation-passing Support promises in addition to continuation-passing Jan 30, 2018
@pdeschen
Copy link

Promise support would be a nice addition. Why not go the AWS sdk route where each function supports promises as

function(params).promise()...

@cdhowie
Copy link
Collaborator

cdhowie commented Apr 2, 2018

I think it makes more sense to have the callback argument be optional. If not provided, asynchronous methods will return a promise instead.

@pdeschen
Copy link

pdeschen commented Apr 2, 2018

I would have agreed to directly return a promise on unprovided callback for a pristine new library but for backward compatibility I think it would be safer to explicitly opt in for a promise.

Reason: I think there could be existing cases out in the field where callbacks are not provided which could then potentially lead to UnhandledPromiseRejectionWarning

@cdhowie
Copy link
Collaborator

cdhowie commented Apr 2, 2018

I don't consider additional warnings to break backwards compatibility, as the program will function the same way in every other respect. In fact, the warnings could be very useful in cases where programs should be handling errors, but are silently ignoring them.

@Darkhogg
Copy link

Any idea if/when this will be done or how can we help??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants