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

Possible internal interface update #778

Closed
charlierudolph opened this issue Jun 5, 2015 · 8 comments · Fixed by #847
Closed

Possible internal interface update #778

charlierudolph opened this issue Jun 5, 2015 · 8 comments · Fixed by #847

Comments

@charlierudolph
Copy link
Contributor

Instead of having three functions

each(arr, iterator, [callback])
eachSeries(arr, iterator, [callback])
eachLimit(arr, limit, iterator, [callback])

How about just having each take an optional object argument at the end

each(arr, iterator[, options][, callback])

To run series have options be {series: true}
To run with limit have options be {limit: 2}

The series option could even be removed as its equivalent to {limit: 1}

I think this would help simplify the library as the number of functions will be reduced but the power will still be there and additional options can easily be added.

This could be done as a non-breaking change as eachSeries and eachLimit are deprecated and each is extended to support the optional options argument.

@aearly aearly added the feature label Jun 7, 2015
@aearly
Copy link
Collaborator

aearly commented Jun 7, 2015

I'm not really interested in changing the API right now, but you do have a very interesting observation that each and eachSeries are really just special cases of eachLimit.

each = (arr, iterator, cb) => eachLimit(arr, Infinity, iterator, cb)
eachSeries = (arr, iterator, cb) => eachLimit(arr, 1, iterator, cb) 

We could simplify our internals if we implemented each and eachSeries in terms of eachLimit, but I'd watch for performance regressions.

@aearly
Copy link
Collaborator

aearly commented Jun 8, 2015

A quick test reveals that the performance impact is negligible, but it changes the behavior of the functions subtly in ways that cause tests to break. I really like the simplification, but this would be a v2 change.

@aearly aearly reopened this Jun 8, 2015
@aearly aearly changed the title Possible interface update Possible internal interface update Jun 8, 2015
@aearly aearly added this to the 2.0 milestone Jun 29, 2015
@kobezzza
Copy link

I think that methods during and doDuring much better implement as an optional feature of whilst/doWhilst/until/doUntil.

Sync API

var count = 0;
async.whilst(
    function () { return count < 5; },
    function (callback) {
        count++;
        setTimeout(callback, 1000);
    },
    function (err) {}
);

Async API

var count = 0;
async.whilst(
    // Just check that arguments.length > 0
    function (callback) {
      return callback(null, count < 5);
    },
    function (callback) {
        count++;
        setTimeout(callback, 1000);
    },
    function (err) {}
);

@aearly
Copy link
Collaborator

aearly commented Jun 30, 2015

Relying on the arity of the function is too brittle -- functions with optional args won't have an accurate length, nor will partially applied functions. Also, in the case of doUntil, doDuring, and doWhilst, the results of the async function are passed to the test function, so length checking doesn't work at all for those.

@megawac
Copy link
Collaborator

megawac commented Jul 2, 2015

👍

@charlierudolph
Copy link
Contributor Author

Has anyone started working on this? I'm happy to take this on.

@aearly
Copy link
Collaborator

aearly commented Jul 3, 2015

Go for it. I also just created a 2.x branch for things we're tracking for
that release.

@megawac
Copy link
Collaborator

megawac commented Mar 5, 2016

I think I'd prefer leaving it as is and implementing each and eachSeries (as in the example) via eachLimit as in #847

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.

4 participants