Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Waterfall with {} input #195

Closed
abrkn opened this Issue · 8 comments

4 participants

@abrkn

No description provided.

@brianmaissy

@abrkn could you clarify the issue/request?

@richmusser

I may be able to elaborate on this request since I think I just ran into it.

async.waterfall() works when you pass in an array of functions, but does not work if you pass in an object of named functions. async will just immediately invoke the final callback, without calling any of the tasks.

example:
This works as expected:

async.waterfall([
function() {...},
function(){...}
],
function(err, results){
})

This does nothing but call the final function:

async.waterfall({
f1: function() {...},
f2: function(){...}
},
function(err, results){
})

In the 2nd, nonworking example, if you change waterfall to series, it works as expected.

@brianmaissy

I don't think this is possible, since the order of the functions needs to be well-defined in waterfall, and an object's attributes aren't ordered. See also #187

@richmusser

That makes sense! The waterfall() function should probably check the input parameters to catch this error then.

@brianmaissy

What do you mean by catch the error? When it calls the callback it should provide an error instead of failing silently? That makes sense to me. @caolan, thoughts?

@richmusser

Yes, through an assertion, exception, or through the error parameter into the callback, it would be nice to alert the programmer that he is using the function in an unsupported way, especially since that 'unsupported way' is consistent with other functions in the library.

@brianmaissy

I definitely think it should be through the error parameter, not by throwing an exception, but I agree with you. If @caolan likes this, I'll submit a pull request. Something along the lines of

if (tasks.constructor !== Array) {
  var err = new Error('First argument to waterfall must be an array of functions');
  callback(err);
}else ...
@caolan
Owner

@brianmaissy yep, looks good to me. A pull request with a test would be much appreciated.

@caolan caolan closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.