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

Fix callback argument order in async.auto #1036

Closed
aearly opened this issue Feb 25, 2016 · 6 comments · Fixed by #1042
Closed

Fix callback argument order in async.auto #1036

aearly opened this issue Feb 25, 2016 · 6 comments · Fixed by #1042
Assignees
Milestone

Comments

@aearly
Copy link
Collaborator

aearly commented Feb 25, 2016

We should standardize the signature of async functions used in async.auto. Rather than always having the callback first, and the optional results object second, we should always make the callback the last argument.

We could also include a flip function that swaps the 2 arguments, to ease migration. (Or just instruct people to _.rearg(asyncFn, 1, 0)).

@aearly aearly self-assigned this Feb 25, 2016
@aearly aearly added this to the 2.0 milestone Feb 25, 2016
@ex1st
Copy link

ex1st commented Feb 28, 2016

Bad idea. For new ordering should be new function and "auto" stay like a flip wrapper for this (mark as deprecated).

@aearly
Copy link
Collaborator Author

aearly commented Feb 28, 2016

One of the themes of the 2.0 release is standardizing the types of functions we use. This will be a well documented breaking change. I think breaking with node's async conventions originally was a worse idea.

@sgarbesi
Copy link

Touching on what @ex1st said.

@aearly Is it possible to make this a configuration preference?

async.auto.flipArgs = true/false ?

I literally probably have thousands of auto references and a countless number of callbacks depending on the ordering.

@aearly
Copy link
Collaborator Author

aearly commented Mar 31, 2016

A config flag won't work well -- say all your functions expect the args flipped, but another node module expects the args un-flipped. If Async is de-duped to the same module, that config would conflict.

Wrapping all your dependent auto tasks with _.flip is probably the best way to migrate, until you can fix the callback ordering.

We didn't just arbitrarily make this change for the sake of standardization -- we made it so other higher-order functions would work properly. (e.g. directly passing the result of asyncify or timeout as an auto task function).

@Meekohi
Copy link

Meekohi commented May 27, 2016

This was a bad idea imho :/ Now you can't just define a function(cb, r) and pass it to .auto regardless of whether or not it has a dependency. You have to check the number or type of arguments inside every function if you want it to be re-usable!

@sffc
Copy link

sffc commented Dec 30, 2017

I think the consistency in 2.x is nice, but I agree that there are advantages to having the callback first. It's probably not worth changing at this point, but always having the callback first across the entire library, like the error argument, may have been a better design.

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.

5 participants