-
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
[wip] POC for await-able Async functions #1526
Conversation
@@ -25,6 +25,7 @@ import wrapAsync from './internal/wrapAsync' | |||
* If you need the index, use `eachOf`. | |||
* @param {Function} [callback] - A callback which is called when all | |||
* `iteratee` functions have finished, or an error occurs. Invoked with (err). | |||
* @returns {Promise} a promise, if the callback is omitted |
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.
Deliberately never returning anything from an Async function has given us the headroom to do this!
@@ -0,0 +1,24 @@ | |||
import noop from 'lodash/noop'; | |||
|
|||
var supportsPromise = typeof Promise === 'function'; |
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.
A pretty crude way to detect promise support. This would be a 3.0 feature. I thinking of dropping support for non-ES2015 environments, in which case we could always return a Promise.
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.
👍 for dropping pre-ES2015 support in v3.
done(err); | ||
}); | ||
it('should handle async functions in map', async () => { | ||
var result = await async.map(input, asyncIdentity); |
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.
😍
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 sweet! It would be cool to also test async with generators, perhaps having a generator function that initializes some data for some of the test cases
it('should handle async functions in each', async () => { | ||
var promise = async.each(input, asyncIdentity); | ||
assert(typeof promise.then === 'function'); | ||
assert(Object.getPrototypeOf(promise) === promiseProto); |
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.
Why not simpler?
assert(promise instanceof Promise);
This is exciting! Looks good so far, let me know if you need help with this PR though. I could add the rest of the tests/docs. |
if (fn === null) return; | ||
var callFn = fn; | ||
fn = null; | ||
callFn.apply(this, arguments); | ||
}; | ||
wrapped.promise = fn.promise; |
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.
Should the wrapped function not return a promise instead to allow once(fn)(...args).then(...)
or how do we intend this to work?
@@ -50,5 +50,6 @@ export default function _eachOfLimit(limit) { | |||
} | |||
|
|||
replenish(); | |||
return callback.promise; |
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 think we'll want to track whether the callback is a user defined callback or an internal promise callback and only return promiseCallback.promise
if its an internal version to avoid cases where users pass us a callback with some state on it.
Hi guys! I've just saw this PR, and I want to tell you guys I've been developing an alternative version of Currently I’ve promisified about 65% of the library functions (with their respective tests), you can see the progress here: https://github.com/DanielRamosAcosta/async-promised/blob/master/TODO.md My plans were:
But now that I’ve seen this PR, maybe we can do something together! I’ll appreciate any thoughts you share with me. |
This branch has diverged greatly from master, I intend to re-implement this feature soon. |
Closing in favor of #1572 |
Related to #1515
This is something I played around with over a year ago.