`async` function support #1390

aearly · 2017-03-25T23:55:28Z

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.

…orted envs

aearly · 2017-03-25T23:59:53Z

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

megawac · 2017-03-27T18:15:19Z

.travis.yml

+#    - node_js: "6"
+#      addons:
+#        firefox: "49.0"
+#      env: BROWSER=true MAKE_TEST=true


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

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

megawac · 2017-03-27T18:17:34Z

lib/autoInject.js


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


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

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

megawac · 2017-03-27T18:18:40Z

lib/doWhilst.js

@@ -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


Is there a AsyncFunction jsdoc tag?

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.

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

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

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

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

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

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

👍 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?

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

megawac · 2017-03-27T18:20:35Z

lib/internal/applyEach.js


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


fns can be a array|object|iterable

megawac · 2017-03-27T18:21:03Z

lib/internal/applyEach.js


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


Do wrapAsync here instead

Good call. 👍

ex1st · 2017-03-26T11:02:53Z

lib/internal/wrapAsync.js

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


Eval is evil. Why not Function?

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

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

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.

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

hargasinski

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

hargasinski · 2017-03-28T03:01:18Z

lib/asyncify.js

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


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

Yeah, we should clarify here.

hargasinski · 2017-03-28T03:27:40Z

lib/doWhilst.js

@@ -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


👍 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?

hargasinski · 2017-03-28T03:31:17Z

lib/internal/wrapAsync.js

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


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

megawac · 2017-03-30T16:48:11Z

lib/index.js

+ * 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.


Link MDN here (https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/async_function)

megawac · 2017-03-30T16:49:55Z

lib/index.js

+ * 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.


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

megawac · 2017-03-30T16:51:22Z

lib/index.js

+ * 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.


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

aearly · 2017-04-01T22:08:03Z

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

megawac · 2017-04-02T17:52:39Z

.eslintrc

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


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

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

megawac · 2017-04-02T17:53:44Z

package.json

    "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",


are we using this?

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

megawac · 2017-04-02T21:33:58Z

👏 awesome work, pretty excited for this

hargasinski · 2017-04-03T02:05:56Z

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

aearly added 8 commits March 22, 2017 13:20

initial experiment with async fn support

9fc2995

disable browser test for now

77e6a21

factor out async support check, make wrapAsync identity() in non-supp…

ac0bdd0

…orted envs

wrapped all collections methods

ffd6685

handle async funcs in control flow methods

db9face

handle async functions in relevant utility methods

0bae58f

add test case for autoInject holy grail

22a5354

merge from master

cdbc105

aearly requested review from megawac and hargasinski March 25, 2017 23:56

aearly added the enhancement label Mar 25, 2017

aearly added the feedback-wanted label Mar 26, 2017

megawac requested changes Mar 27, 2017

View reviewed changes

ex1st reviewed Mar 27, 2017

View reviewed changes

aearly added 5 commits March 27, 2017 12:25

re-enable firefox testing

4b1278b

fix travis yaml

e62c53c

re-enable xvfb for travis

700aa23

fix small issues from PR

57b8877

custom typedef doc test

6ac9cb0

hargasinski reviewed Mar 28, 2017

View reviewed changes

megawac reviewed Mar 30, 2017

View reviewed changes

aearly added 2 commits March 31, 2017 14:17

clarify AsyncFunction docs

faf395c

use the AsyncFunction type in all docs 😓

94a8b2d

fix lint error

fab1e45

megawac reviewed Apr 2, 2017

View reviewed changes

missed some docs

8faed87

megawac approved these changes Apr 2, 2017

View reviewed changes

aearly merged commit 49119a8 into master Apr 2, 2017

aearly mentioned this pull request Apr 2, 2017

async Function Support #1386

Closed

aearly deleted the async-fn-support branch April 2, 2017 22:37

dependabot-preview bot mentioned this pull request Mar 21, 2018

Update async requirement to 2.6.0 randallcara/vso-agent#14

Closed

async function support #1390

async function support #1390

Conversation

aearly commented Mar 25, 2017

aearly commented Mar 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hargasinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aearly commented Apr 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megawac commented Apr 2, 2017

hargasinski commented Apr 3, 2017

`async` function support #1390

`async` function support #1390