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

applyEach arguments position #1228

Closed
jamiebuilds opened this issue Jul 12, 2016 · 18 comments · Fixed by #1640
Closed

applyEach arguments position #1228

jamiebuilds opened this issue Jul 12, 2016 · 18 comments · Fixed by #1640

Comments

@jamiebuilds
Copy link

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
Copy link
Collaborator

megawac commented Jul 12, 2016

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
Copy link
Collaborator

aearly commented Jul 12, 2016

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
Copy link
Author

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
Copy link
Collaborator

megawac commented Jul 12, 2016

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

@aearly
Copy link
Collaborator

aearly commented Jul 12, 2016

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

@jamiebuilds
Copy link
Author

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
Copy link
Collaborator

aearly commented Jul 12, 2016

The first one.

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

@jamiebuilds
Copy link
Author

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
Copy link
Collaborator

aearly commented Jul 12, 2016

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
Copy link
Author

jamiebuilds commented Jul 12, 2016

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
Copy link
Collaborator

aearly commented Jul 12, 2016

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
Copy link
Collaborator

megawac commented Jul 12, 2016

@aearly lets make this a v3 change to consider

@aearly aearly added this to the 3.0 milestone Jul 12, 2016
@aearly
Copy link
Collaborator

aearly commented Jul 12, 2016

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

@jamiebuilds
Copy link
Author

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
Copy link
Collaborator

aearly commented Jul 12, 2016

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
Copy link
Author

jamiebuilds commented Jul 12, 2016

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

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

@aearly
Copy link
Collaborator

aearly commented Jul 9, 2018

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
Copy link
Collaborator

aearly commented Oct 1, 2018

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

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

Successfully merging a pull request may close this issue.

3 participants