-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat: await-able Async methods #1572
Conversation
@@ -0,0 +1,19 @@ | |||
const PROMISE_SYMBOL = Symbol('promiseCallback') | |||
|
|||
function promiseCallback () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the older strategy. It's a bit more clunky, but doesn't add another frame to the call stack for each method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to use this in a couple places where the awaitify
wrapper couldn't work (usually due to optional arguments).
lib/internal/awaitify.js
Outdated
}) | ||
} | ||
|
||
awaitable[Symbol.toStringTag] = 'AsyncFunction' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a hack, not sure if we should do this.
lib/internal/awaitify.js
Outdated
} | ||
|
||
awaitable[Symbol.toStringTag] = 'AsyncFunction' | ||
awaitable[ASYNC_FN] = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intend to use this so we can avoid an additional layer of wrapping in wrapAsync
.
All the collection methods and relevant control flow methods are now awaitable. However, I'm running into some problems with the Utils methods. Most of the utils methods are wrappers, e.g. they wrap an async function and return an async function. We use We have avoided looking at The other option is to wrap Or we just punt on this completely. You can't await wrapped functions. This would be a bummer, because |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@megawac @hargasinski I would love to get a review on this before merging. In regards to my above comment about the wrapper Utils methods, I'm thinking we can defer a solution there until later (temporary punt). |
@aearly really sorry for the delay. These past few weeks have busy for me, and the PR ended up getting drowned out by other things. Thanks for the reminder! I will review it this weekend. I want to spend some time playing around with it, just to make sure I understand it fully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I reviewed this a couple weeks ago to make sure the code made sense. The reason I didn't approve at the time was (and is) I'm not entirely sure such a change is necessary. Currently if the user wants to use the new async/await syntax they can call utils.promisify(myFavouriteAsyncMethod)
or even promisify async.
I can definitely can see the draw of having this done by default I'm just trying to convince myself that this should be done on the methods proper or via a wrapper (e.g. import async from async/awaitable
;
if (fn === null) return; | ||
var callFn = fn; | ||
fn = null; | ||
callFn.apply(this, args); | ||
}; | ||
} | ||
Object.assign(wrapper, fn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So static properties attached to the fn
get attached to the wrapper
, namely the PROMISE_SYMBOL
attached to promiseCallback
s.
@hargasinski Don't sweat it, after all, this project is run on donated time. @megawac I hear your concerns about whether we should do this by default -- this is a rather significant change. True, users can promisify methods, but I'm thinking about developer convenience. Already, people are omitting the final callback on an Async method and being surprised a promise is not returned. Several other popular libraries already work like this.
This next major version seems like a good point to throw more weight behind |
How does this affect performance Alex? If its a significant hit my vote will be to seperate the asyncified and original methods into 2 files. I'd be completely fine with the async compatible version being our default export. I know several people who use our library due to it's performance for intensive use cases so I'm adamant about maintaining good benchmarks :) |
There was no significant difference in the benchmarks on node 8 or 10. I was concerned the extra wrapper would slow things down, but that is not the case. |
This is some really awesome work! The change look good to me. Personally, I'm in favour of it. I think there has been enough interest/requests (#439, #956, promise-async, koa-async, async-q) to justify adding it. Also, looking over past issues, it seems that historically we've been against it because promises weren't implemented natively. Now that they are, and with significant parts of the community moving towards promise-based approaches, I think this change makes sense for us. |
Closes #1515