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 #1386

Closed
aearly opened this issue Mar 16, 2017 · 10 comments
Closed

async Function Support #1386

aearly opened this issue Mar 16, 2017 · 10 comments

Comments

@aearly
Copy link
Collaborator

aearly commented Mar 16, 2017

One of the elephants in the room is the new async/await support that has come to Node and Chrome, and will soon hit every other major browser. I've been thinking about what Async can do in the async/await world.

Currently, we can adapt async functions by wrapping them with asyncify. Since an async function is essentially just a function that returns a Promise, that old adapter can easily convert it to a callback-style function. However, it leads to the somewhat absurd looking:

async.mapLimit(arr, 10, async.asyncify(async (val) => {
  let foo = await doSomething(val);
  //...
 return bar;
}), done);

However, one of the features in the spec for async functions is that:

Object.getPrototypeOf(asyncFn)[Symbol.toStringTag] === "AsyncFunction"

This gives a way to easily detect (native) async functions. We could use this technique to automatically asyncify them. The example above becomes:

async.mapLimit(arr, 10, async (val) => {
  let foo = await doSomething(val);
  //...
 return bar;
}, done);

...which seems to flow much more naturally. I also think we should continue to use callbacks. If a user wanted to await the result, they would have to promisify the function, or pify Async as a whole:

let result = await pify(async.mapLimit)(arr, 10, async (val) => {
  let foo = await doSomething(val);
  //...
 return bar;
});

The above method for detecting async functions only works with native functions. I don't think there is a way to detect Babel transpiled functions. We certainly can't detect normal functions that simply return Promises, because we'd have to retroactively not pass a callback. There would he a huge caveat that this would only work without a transpiler in very modern environments, otherwise you still have to manually wrap with asyncify.

Also, admittedly, many Async methods don't make sense with async/await. Most of the control flow methods (save for things like auto and queue) are more easily replicated with native control flow constructs. map and parallel can be replaced with Promise.map and Promise.all. However, the limiting collection functions would be very useful, as well as auto and a few others. (Also, autoInject with async functions is a async control flow dream!)

@megawac
Copy link
Collaborator

megawac commented Mar 17, 2017

I'm going to ponder over this some more but I'd like to ask a couple technical questions just so I can try to better envision what this looks like..

Is there a reason to do Object.getPrototypeOf(asyncFn)[Symbol.toStringTag] === "AsyncFunction" or can we do asyncFn[Symbol.toStringTag] === "AsyncFunction" (seems to work in FF)?

So is the proposal any time someone provides a callback of the format cb(err, arg) we should detect if it is an AsyncFunction; if it is an async function we should apply promisify otherwise use it as is

Also sorry, I'm not following on the await example, if we detect that the function is an AsyncFunction what are the challenges of supporting await?

@aearly
Copy link
Collaborator Author

aearly commented Mar 17, 2017

Is there a reason to do Object.getPrototypeOf(asyncFn)[Symbol.toStringTag] === "AsyncFunction" or can we do asyncFn[Symbol.toStringTag] === "AsyncFunction" (seems to work in FF)?

Thats just the canonical ECMA spec way to do it. I guess in theory, someone could overwrite asyncFn[Symbol.toStringTag].

So is the proposal any time someone provides a callback of the format cb(err, arg) we should detect if it is an AsyncFunction; if it is an async function we should apply promisify otherwise use it as is

I think you have it a bit backwards. Wherever we accept an callback-accepting iteratee function (function(args..., callback) {}), we should check to see if it is an async function, and then asyncify it.

The await example is what someone would have do if they wanted to await an Async method. I don't think we should have Async methods start returning promises so that would work -- leave it for the user to do.

@aearly
Copy link
Collaborator Author

aearly commented Apr 2, 2017

Implemented in #1390 !

@aearly aearly closed this as completed Apr 2, 2017
@manvalls
Copy link

manvalls commented Apr 5, 2017

This was a breaking change and broke our deployed code. Please think twice when doing such things without increasing the major version.

PS: thanks for all the great work you're doing with this library 😄

@megawac
Copy link
Collaborator

megawac commented Apr 5, 2017 via email

@sergiofgonzalez
Copy link

Agreed, it broke my build too...

A waterfall that was calling an async function which worked a few days back started failing with a "cb is not function" because the async callback was no longer provided to the function.

@aearly
Copy link
Collaborator Author

aearly commented Apr 6, 2017

Sorry we broke your code. We didn't anticipate your use case for async functions. I'd recommend rolling back to 2.2.0, or refactoring your code to return the values you need, rather than using callbacks. Unfortunately, the cat is out of the bag with this feature, so we can't roll back.

@sergiofgonzalez
Copy link

sergiofgonzalez commented Apr 7, 2017

@aearly Please, don't mention it!! It's really nice of you to answer 🥇

@manvalls hinted me a great solution which did not require to rollback. As you're using the symbol to detect the async in the function declaration, he thought of a clever way to cheat the detection.

My waterfall was using functions exported from other modules, one of them was an async one and thus causing the failure.

So just by changing from:

... 
/* services module */
function doThis(param, cb) {
...
}

async function doThatAsync(param, cb) {
...
}

module.exports = {
  doThis: doThis,
  doThat: doThatAsync  
}; 

...
async.waterfall([
  services.doThis,
  services.doThat,  // fails with "cb is not a function"
], err => {
...
}

To:

... 
/* services module */
function doThis(param, cb) {
...
}

async function doThatAsync(param, cb) {
...
}

module.exports = {
  doThis: doThis,
  doThat: (...args) => doThatAsync(..args)   // cheating the detection
}; 

...
async.waterfall([
  services.doThis,
  services.doThat, /* it works!!! */
], err => {
...
}

Thank you very much again

@ORESoftware
Copy link
Contributor

can we use async/await with async.autoInject()?

async.autoInject({
    
    conn1: async function () {
      return conn1;
    },
    
    conn2: async function () {
      return conn2;
    },
});

doesn't seem to work, I get:

Error: autoInject task functions require explicit parameters.
at /Users/alexamil/WebstormProjects/nabisco/cdt-now/node_modules/async/dist/async.js:2081:23
at /Users/alexamil/WebstormProjects/nabisco/cdt-now/node_modules/async

@hargasinski
Copy link
Collaborator

@ORESoftware yes, async functions should work with autoInject. I tested the code you posted in Chrome, and it ran. I got a ReferenceError in the final callback as conn1 and conn2 are undefined. After changing it to

async.autoInject({
    conn1: async function () {
      return 'foo'
    },
    conn2: async function () {
      return 'bar'
    },
})

it works fine. However, we don't support transpiled async functions. Are you transpiling your code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants