Skip to content

asyncify promisified functions#840

Merged
aearly merged 12 commits intocaolan:masterfrom
zartdinov:master
Jul 9, 2015
Merged

asyncify promisified functions#840
aearly merged 12 commits intocaolan:masterfrom
zartdinov:master

Conversation

@zartdinov
Copy link
Copy Markdown
Contributor

@zartdinov zartdinov commented Jul 8, 2015

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);

megawac
megawac reviewed Jul 8, 2015
View reviewed changes
Comment thread lib/async.js Outdated
Copy link
Copy Markdown
Collaborator

@megawac megawac Jul 8, 2015

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
Copy Markdown
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)

aearly
aearly reviewed Jul 8, 2015
View reviewed changes
Comment thread lib/async.js Outdated
Copy link
Copy Markdown
Collaborator

@aearly aearly Jul 8, 2015

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
Copy Markdown
Collaborator

@aearly aearly Jul 9, 2015

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

@zartdinov
callback moved out from try/catch block
3c2fd71 
unit tests for es6-promise and rsvp
@zartdinov zartdinov changed the title asyncify Promises asyncify promisified functions Jul 9, 2015
@zartdinov
Copy link
Copy Markdown
Contributor Author

zartdinov commented Jul 9, 2015

Done!

aearly
aearly reviewed Jul 9, 2015
View reviewed changes
Comment thread test/test-async.js Outdated
Copy link
Copy Markdown
Collaborator

@aearly aearly Jul 9, 2015

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
Copy Markdown
Contributor Author

zartdinov commented Jul 9, 2015

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

@aearly
Copy link
Copy Markdown
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
@aearly
Merge pull request #840 from zartdinov/master
e82ce4b 
asyncify promisified functions
@aearly aearly merged commit e82ce4b into caolan:master Jul 9, 2015
@megawac megawac mentioned this pull request Jul 10, 2015
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zartdinov @megawac @aearly