applyEach arguments position

[jamiebuilds]

Currently the applyEach and applyEachSeries functions have the following type signature:

async.applyEach(fns, ...args?, callback?)

Having a rest parameter in the middle of the arguments list is very difficult to write a type annotation for. It's currently not possible in either Flow or TypeScript, which is why you see stuff like this (I'm writing a similar type definition for Flow right now).

Would it be okay to switch the argument order around and make callback nullable so it is like this?

async.applyEach(fns, callback | null, ...args?)

Or use an array instead of a rest parameter?

async.applyEach(fns, args?: Array<any>, callback?)

[megawac]

doDuring's test function also has that type of function definition. I think @aearly will have some feedback as its fresh in his mind (#1217, #1224)

[aearly]

I'd really prefer to always have a callback be the last arg of any async function, as per node's style. I think using an array for the args would be a good compromise. It also has more symmetry with the native Function.apply.

async.applyEach(fns, args?: Array<any>, callback?)

Also, the problem here is function (arg1, ...args, arg2) right? function(...args, callback) is type-able in TypeScript/Flow? There are many functions in Async that have the latter signature -- waterfall tasks, doDuring's test function, autoInject tasks...

[jamiebuilds]

Also, the problem here is function (arg1, ...args, arg2) right? function(...args, callback) is type-able in TypeScript/Flow?

I'm afraid not, in both type systems rest params are only allowed as the final param.

doDuring's test function also has that type of function definition.

I don't understand. According to the docs, it doesn't appear to have anything similar.

[megawac]

The test function for doDuring is of the syntax function test(err, ...args, callback)

[aearly]

Nah, it's (...args, callback) like many others: https://github.com/caolan/async/blob/master/lib/doDuring.js#L29-32

[jamiebuilds]

Wait, I'm sorry, I'm so confused...

Do you mean like this?

async.doDuring(fn, function test(...args, callback) {
  // ...
}, callback);

or this?

async.doDuring(fn, function test(callback) {
  callback(...args, function() { /* ... */ });
}, callback);

or something else?

[aearly]

The first one.

doDuiring was recently updated; it's not released yet so the docs are a bit inaccurate.

[jamiebuilds]

Ah, okay. That's the source of my confusion.

From the perspective of authoring type definitions I'd advise against an api like that. But if it adds a lot of value I guess it might be worth it.

[aearly]

Yeah, I understand. Unfortunately, "function(...args, callback) {}" describes every node-style async function, so it's an API we have to support.

Do you think TypeScript might be enhanced to allow that form? Or would it lead to ambiguities?

[jamiebuilds]

I don't know what the TypeScript team wants to do. I'll ask some others on the Flow team what their thoughts are.

[aearly]

BTW, I'm 👍 for having the args be an array in the case of applyEach. Having an optional param after a rest param will always lead to ambiguities.

[megawac]

@aearly lets make this a v3 change to consider

[aearly]

Agreed. It's also possible that Flow and TS might be augmented in the interim.

[jamiebuilds]

So current status about having rest parameters in a type system:

It might be possible to have within library definitions without ambiguity. However, because rest parameters aren't allowed at the start/middle of a parameter list, it would be impossible to write a type definition within the normal syntax.

function method(a, ...b, c) { } // can't be turned into valid es6 without a syntax transform.

[aearly]

Yeah, I understand that. It's a bit tricky, since we have a scenario where the rest params are untyped, except for the last one. We basically implement it like this:

function foo(...args) {
  var callback = args.pop();
  ///...
}

So while the actual argument syntax can't be directly implemented in ES6, we have a situation where there's the possibility for extra type information on top of what ES6 allows.

[jamiebuilds]

I've proposed allowing the usage of spread elements within tuples to solve this case syntactically

function method(...args: [string, ...number, boolean]) {}

[aearly]

I'm revisiting this, and I'm not sure there's any way to satisfy TypeScript.

The proposal is to change applyEach to use an array for the initially applied args.

async.applyEach(fns, args?: Array<any>, callback?)

However, both args, and callback are optional. When they are both omited, a function is returned with the signature (invalid typescript):

appliedFn(...args: Array<any>, callback?)

This is designed to be slotted into waterfall and similar.

We could change those args to be an array, but that severely limits the usefulness of the method, having to combine arguments into an array. The each example in the docs becomes pretty clunky:

async.each(
    buckets.map(bucket => [bucket]), // have to turn each item of the array into an array
    async.applyEach([enableSearch, updateSchema]),
    callback
);

---

Another posibility is to deprecate the partial application feature of applyEach. With arrows being available basically everywhere now, the above example could become:

async.each(
    buckets,
    (bucket, next) => async.applyEach([enableSearch, updateSchema], [bucket], next),
    callback
);

A bit more clear, and less magic involved.

[aearly]

Another option:

Make the args required, don't accept a callback, and return a function that only takes a callback. This turns it into a wrapper method that hooks two or more functions together.

async.applyEach(fns: Array<Function>, ...args: Array<any>) : Function

Which returns a function of the type:

fn(callback?: Function)

The example in the docs would become:

async.each(
    buckets,
    (bucket, next) => async.applyEach([enableSearch, updateSchema], bucket)(next),
    callback
);

Or to be even more clever:

async.each(
    buckets,
    async bucket => async.applyEach([enableSearch, updateSchema], bucket)(), // the returned function could be made awaitable
    callback
);