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

Promisify async wrapper #688

Closed
wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

Alternate approach to promisification done by @bajtos in #173

I added more direct support for promise-like fulfill/reject in req_wrap, in the hopes it would be faster if support went deeper into C++.

The benchmarks are noisy on the machines I have access to, but it AFAICT, performance is no better or worse than node, for callback case. Also, no better or worse than #173 for the Promise case.
.
There are some open questions about whether direct use of a v8::Promise:Resolver object could be faster, but I'm blocked on that because I can't create one with any slots, so Wrap() can't work on it.

I'm also interested to look at bluebird, which at least one commentator claimed was faster than #173 when it promisifies fs. If so, it should be possible to improve the promise path in pure js.

@vkurchatkin
Copy link
Contributor

whether direct use of a v8::Promise:Resolver object could be faster

I seriously doubt that, because it simple calls some high level js functions.

I don't like the approach where both callbacks and promises could be used. I think there should be separate set of functions that return promises.

@Fishrock123
Copy link
Contributor

I don't like the approach where both callbacks and promises could be used. I think there should be separate set of functions that return promises.

I would prefer it over this situation: fs.thingSync() fs.thing() fs.thingPromise()

@Fishrock123
Copy link
Contributor

Given that promises exemplify the future of async in ES6/7 javascript, we should be looking to the future as having those default returns on async functions IMO. (where it makes sense)

var req = new FSReqWrap();
req.oncomplete = callback;
if (util.isFunction(callback)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe switch to typeof callback === 'function'?

@Fishrock123 Fishrock123 added future discuss Issues opened for discussions and feedbacks. and removed ideas labels Apr 28, 2015
@Fishrock123
Copy link
Contributor

This is interesting, but I'm going to close it out for further discussion on NG as per #11 (comment), and that there isn't momentum at this time. This is a great reference for future discussions though, thanks for making it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants