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

asyncify promisified functions #840

Merged
merged 12 commits into from
Jul 9, 2015
Merged

asyncify promisified functions #840

merged 12 commits into from
Jul 9, 2015

Conversation

zartdinov
Copy link
Contributor

Most of Promise realisations doesn't contain nodeify method. Here is extention of async wrapper for promisified functions.
For example promise-styled openpgp library functions looks like

openpgp.key.generate(options).then(function(key) {
    // success
}).catch(function(error) {
    // failure
});

And can be easily wrapped to use with favourite async:)

async.asyncify(openpgp.key.generate);

if (typeof result.then === "function") {
result.then(function(values) {
var args = [null].concat(values);
callback.apply(this, args);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be only one value from promise so can you do

callback.call(this, value)

@megawac
Copy link
Collaborator

megawac commented Jul 8, 2015

👍 can you add some unit tests (feel free to add a dev dependency for some promise implementation, e.g. NPO/rsvp/bluebird/es6-promise)

callback.apply(this, args);
}).catch(callback);
} else {
callback(null, result);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to have the callback call in the try/catch block.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep: The failing test is the one that guards against this: https://travis-ci.org/caolan/async/jobs/70115957

unit tests for es6-promise and rsvp
@zartdinov zartdinov changed the title asyncify Promises asyncify promisified functions Jul 9, 2015
@zartdinov
Copy link
Contributor Author

Done!

'es6-promise',
'rsvp'
].reduce(function(promises, name) {
var Promise = require(name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to make these tests node-only, because this won't work in the browser.

@zartdinov
Copy link
Contributor Author

I think node-only for a while, but all tested promise libraries differently support browsers.

@aearly
Copy link
Collaborator

aearly commented Jul 9, 2015

It's not the promise implementation, but the require(name) statement that won't work. We can fix the tests for browsers later.

aearly added a commit that referenced this pull request Jul 9, 2015
asyncify promisified functions
@aearly aearly merged commit e82ce4b into caolan:master Jul 9, 2015
@megawac megawac mentioned this pull request Jul 10, 2015
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants