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

`async` function support #1390

Merged
merged 17 commits into from Apr 2, 2017

Conversation

Projects
None yet
4 participants
@aearly
Copy link
Collaborator

commented Mar 25, 2017

#1386

This adds support for passing async functions wherever it might make sense.

Obviously we are duplicating some core functionality with things like map and race, but this allows us to treat things uniformly. We do have some good value to add with functions that limit concurrency, and adding more higher-order operations that just map and each (as you would get with Promise.map() and Promise.all()). "Lodash for async functions" is a good thing to strive to be.

Most of the control flow functions are of dubious value, (waterfall, parallel, whilst), but you can do some really neat things with auto and queue. (I really love the autoInject shorthand test!)

The utility functions are a bit strange. retry, reflect, memoize and timeout now take in an async function and return a callback-accepting function. It feels like those should create a Promise-returning function.

@aearly

This comment has been minimized.

Copy link
Collaborator Author

commented on 9fc2995 Mar 22, 2017

@megawac @hargasinski I'd love to get your 👀 on this before I continue further.

This comment has been minimized.

Copy link
Collaborator

replied Mar 23, 2017

The wrapAsync implementation looks good to me, especially with the refactor in ac0bdd0.

One question though, is there a reason for only wrapping certain iteratees in wrapAsync? Wrapping the iteratee in internal/eachOfLimit would give most methods support for async functions. From #1386, I know async/await does not make sense in flow controls functions, but I don't see a reason not to support it unless the additional performance of an extra wrapper is a concern?

By the way, is there anything I could help with?

This comment has been minimized.

Copy link
Collaborator Author

replied Mar 23, 2017

This is just the first pass with a handful of functions -- I wanted to validate the strategy before applying it to every method.

Wrapping the iteratee in internal/eachOfLimit does catch a lot of things, but we have to wrap async functions before they are wrapped with something like withoutIndex, so it means it will have to be handled in a lot of places.

I think it's worth adding support to control flow functions -- will come in handy if you need to mix and match async functions and callback functions in the same parallel call, for example.

This comment has been minimized.

Copy link
Collaborator

replied Mar 23, 2017

This is really cool! Are there going to be some cases we have to worry about where the async callback has a different form than (err, result) arguments? For example, would waterfall/series be supported?

Anyway, looks like great progress 🥇

This comment has been minimized.

Copy link
Collaborator Author

replied Mar 24, 2017

Since promises only support resolving to a single value, things like waterfall would get weird/tricky. You'd have to use arrays like:

async.waterfall([
  async function () {
    let a = await foo();
    return [a, b, c];
  }
  async function([a, b, c]) {
    let d = await bar(a, b, c);
    return [a, d];
  },
  async function ([a, d]) {
    // etc...
  }
], done)
@megawac

This comment has been minimized.

Copy link
Collaborator

commented on lib/asyncify.js in 9fc2995 Mar 23, 2017

So the asyncify part of this won't be necessary any more? Very cool!

@megawac

This comment has been minimized.

Copy link
Collaborator

commented on lib/internal/wrapAsync.js in 9fc2995 Mar 23, 2017

nit: typeof Symbol === 'function'

@aearly aearly requested review from megawac and hargasinski Mar 25, 2017

@aearly aearly added the enhancement label Mar 25, 2017

@aearly

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 25, 2017

Also, I ran the benchmarks -- there is no difference in performance, whether the environment supports async functions or not.

.travis.yml Outdated
# - node_js: "6"
# addons:
# firefox: "49.0"
# env: BROWSER=true MAKE_TEST=true

This comment has been minimized.

Copy link
@megawac

megawac Mar 27, 2017

Collaborator

why are we disabling this stuff? Can we just up the firefox version?

This comment has been minimized.

Copy link
@aearly

aearly Mar 27, 2017

Author Collaborator

I disabled this thinking it was an issue with karma, but it's a syntax error. I'll investigate further and re-enable. We could test both 45 (ESR, no async support) and 52 (async supported).


if (isArray(taskFn)) {
params = taskFn.slice(0, -1);
taskFn = taskFn[taskFn.length - 1];

newTasks[key] = params.concat(params.length > 0 ? newTask : taskFn);
} else if (taskFn.length === 1) {
} else if ((!fnIsAsync && taskFn.length === 1) ||
(fnIsAsync && taskFn.length === 0)) {

This comment has been minimized.

Copy link
@megawac

megawac Mar 27, 2017

Collaborator

fnIsAsync + taskFn.length === 1 =P (dont do it lol)

This comment has been minimized.

Copy link
@aearly

aearly Mar 27, 2017

Author Collaborator

🤢

This comment has been minimized.

Copy link
@aearly

aearly Mar 27, 2017

Author Collaborator

Actually, I should probably factor this condition out into a hasNoDeps variable.

@@ -19,7 +20,7 @@ import onlyOnce from './internal/onlyOnce';
* passes. The function is passed a `callback(err)`, which must be called once
* it has completed with an optional `err` argument. Invoked with (callback).
* @param {Function} test - synchronous truth test to perform after each

This comment has been minimized.

Copy link
@megawac

megawac Mar 27, 2017

Collaborator

Is there a AsyncFunction jsdoc tag?

This comment has been minimized.

Copy link
@aearly

aearly Mar 27, 2017

Author Collaborator

I was thinking about the best way to document this. For sure, add something to the intro.md, but we have lots of repeated docs where we describe the iteratee function as a node-style async function. It would be nice to centralize the description of what Async accepts, with each Async method just describing the non-callback parameters.

This comment has been minimized.

Copy link
@megawac

megawac Mar 27, 2017

Collaborator

Perhaps an external definition if jsdoc doesn't support some way of documenting an async function?

http://usejsdoc.org/tags-external.html

This comment has been minimized.

Copy link
@aearly

aearly Mar 27, 2017

Author Collaborator

I'm also thinking of using a @typedef, like we do for the return values of queue/cargo.

http://usejsdoc.org/tags-typedef.html

This comment has been minimized.

Copy link
@megawac

megawac Mar 27, 2017

Collaborator

I really don't like how the @typedefs look in our code. I'd rather just link to MDN

This comment has been minimized.

Copy link
@aearly

aearly Mar 27, 2017

Author Collaborator

Check the latest commit. It's cleaner than the JSDoc docs make it out to be...

This comment has been minimized.

Copy link
@hargasinski

hargasinski Mar 28, 2017

Collaborator

👍 for using typedef for this. Does this mean most of the docs should to be updated to reflect that iteratee should be an AsyncFunction type?

This comment has been minimized.

Copy link
@aearly

aearly Mar 28, 2017

Author Collaborator

Yes, lots of docs to update. 😓 It does create a nice link to the central AsyncFunction docs in the rendered docs, though.


export default function applyEach(eachfn) {
return rest(function(fns, args) {
var go = initialParams(function(args, callback) {
var that = this;
return eachfn(fns, function (fn, cb) {
return eachfn(arrayMap(fns, wrapAsync), function (fn, cb) {

This comment has been minimized.

Copy link
@megawac

megawac Mar 27, 2017

Collaborator

fns can be a array|object|iterable


export default function applyEach(eachfn) {
return rest(function(fns, args) {
var go = initialParams(function(args, callback) {
var that = this;
return eachfn(fns, function (fn, cb) {
return eachfn(arrayMap(fns, wrapAsync), function (fn, cb) {
fn.apply(that, args.concat(cb));

This comment has been minimized.

Copy link
@megawac

megawac Mar 27, 2017

Collaborator

Do wrapAsync here instead

This comment has been minimized.

Copy link
@aearly

aearly Mar 27, 2017

Author Collaborator

Good call. 👍

var supported;
try {
/* eslint no-eval: 0 */
supported = isAsync(eval('(async function () {})'));

This comment has been minimized.

Copy link
@ex1st

ex1st Mar 27, 2017

Contributor

Eval is evil. Why not Function?

supported = isAsync(new Function("return (async function () {})")());

This comment has been minimized.

Copy link
@megawac

megawac Mar 27, 2017

Collaborator

eval can be abused in the same way that new Function can be abused. I'd rather just use eval here

This comment has been minimized.

Copy link
@aearly

aearly Mar 27, 2017

Author Collaborator

new Function() and eval are equivalently evil. Here, they are necessary, because there is no other way to detect async support without causing a parse error that halts JS execution.

This comment has been minimized.

Copy link
@hargasinski

hargasinski Mar 28, 2017

Collaborator

I'm good with using eval here instead of new Function.

aearly added some commits Mar 27, 2017

@hargasinski
Copy link
Collaborator

left a comment

I'm really excited for this, awesome work! 😄

@@ -48,7 +48,7 @@ import initialParams from './internal/initialParams';
* }
* ], callback);
*
* // es6 example
* // es2017 example

This comment has been minimized.

Copy link
@hargasinski

hargasinski Mar 28, 2017

Collaborator

Would it be useful to add a note here that async functions are supported out of the box (when not transpiled)?

This comment has been minimized.

Copy link
@aearly

aearly Mar 28, 2017

Author Collaborator

Yeah, we should clarify here.

@@ -19,7 +20,7 @@ import onlyOnce from './internal/onlyOnce';
* passes. The function is passed a `callback(err)`, which must be called once
* it has completed with an optional `err` argument. Invoked with (callback).
* @param {Function} test - synchronous truth test to perform after each

This comment has been minimized.

Copy link
@hargasinski

hargasinski Mar 28, 2017

Collaborator

👍 for using typedef for this. Does this mean most of the docs should to be updated to reflect that iteratee should be an AsyncFunction type?

var supported;
try {
/* eslint no-eval: 0 */
supported = isAsync(eval('(async function () {})'));

This comment has been minimized.

Copy link
@hargasinski

hargasinski Mar 28, 2017

Collaborator

I'm good with using eval here instead of new Function.

* This type of function is also referred to as a "Node-style async function".
*
* Wherever we accept a Node-style async function, we also directly accept an
* ES2017 `async` function.

This comment has been minimized.

* ES2017 `async` function.
* In this case, the `async` function will not be passed a callback, and any
* thrown error will be used as the `err` argument of a theoretical callback,
* and the return value will be used as the `result` value.

This comment has been minimized.

Copy link
@megawac

megawac Mar 30, 2017

Collaborator

What do you mean the async function will not be passed a callback? Do you mean the result of the async function?

* thrown error will be used as the `err` argument of a theoretical callback,
* and the return value will be used as the `result` value.
*
* Note that we can only detect native `async function`s in this case.

This comment has been minimized.

Copy link
@megawac

megawac Mar 30, 2017

Collaborator

Note, due to JavaScript limitations, we can only detect native async functions and not polyfilled implementations.

aearly added some commits Mar 31, 2017

@aearly

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 1, 2017

@megawac @hargasinski Many doc updates for your 👀. 😓

@megawac

This comment has been minimized.

Copy link
Collaborator

commented on lib/asyncify.js in 94a8b2d Apr 2, 2017

do you if to @link this one?

This comment has been minimized.

Copy link
Collaborator Author

replied Apr 2, 2017

No, it works just like the @param types.

This comment has been minimized.

Copy link
Collaborator

replied Apr 2, 2017

thanks

@megawac

This comment has been minimized.

Copy link
Collaborator

commented on lib/compose.js in 94a8b2d Apr 2, 2017

does this need a @link

This comment has been minimized.

Copy link
Collaborator Author

replied Apr 2, 2017

Nope.

@@ -6,7 +6,7 @@
"es6": true
},
"parserOptions": {
"ecmaVersion": 6,
"ecmaVersion": 8,

This comment has been minimized.

Copy link
@megawac

megawac Apr 2, 2017

Collaborator

what is es8? can we set this to 2017/2018 whatever?

This comment has been minimized.

Copy link
@aearly

aearly Apr 2, 2017

Author Collaborator

ES8 is eslint's shorthand for ES2017. No, you can't use "2017"... Just subtract 2009. 😛

"babel-plugin-add-module-exports": "^0.2.1",
"babel-plugin-istanbul": "^2.0.1",
"babel-plugin-transform-es2015-modules-commonjs": "^6.3.16",
"babel-preset-es2015": "^6.3.13",
"babel-preset-es2017": "^6.22.0",

This comment has been minimized.

Copy link
@megawac

megawac Apr 2, 2017

Collaborator

are we using this?

This comment has been minimized.

Copy link
@aearly

aearly Apr 2, 2017

Author Collaborator

Yes, otherwise babel can't parse the test file where we use async functions, even if they're being used natively.

@megawac

megawac approved these changes Apr 2, 2017

@megawac

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2017

👏 awesome work, pretty excited for this

@aearly aearly merged commit 49119a8 into master Apr 2, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.8%) to 98.738%
Details

@aearly aearly referenced this pull request Apr 2, 2017

Closed

`async` Function Support #1386

@aearly aearly deleted the async-fn-support branch Apr 2, 2017

@hargasinski

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2017

I'm a little late, but really great work. I'm excited about this 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.