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

add concatLimit [fixes #1426] #1430

Merged
merged 1 commit into from Jun 11, 2017
Merged

add concatLimit [fixes #1426] #1430

merged 1 commit into from Jun 11, 2017

Conversation

hargasinski
Copy link
Collaborator

@hargasinski hargasinski commented Jun 7, 2017

Adds the elusive concatLimit.

@hargasinski hargasinski self-assigned this Jun 7, 2017
@hargasinski
Copy link
Collaborator Author

@hargasinski hargasinski commented Jun 7, 2017

While I was playing around with the concat function, I noticed it doesn't preserve order. i.e.

async.concat([30, 15], function(val, cb) {
    setTimeout(function() {
        cb(null, [''+val]);
    }, val);
}, function(err, result) {
    console.log(result);
});

will log

[ '15', '30' ]

as opposed to

[ '30', '15' ]

Should this be changed to the match the behavior of functions like filter, groupBy or map, which preserve the order in result? Would that be considered a breaking change?

/cc @aearly @megawac

@aearly
Copy link
Collaborator

@aearly aearly commented Jun 8, 2017

You're right -- it should be order-preserving. I think that would be a subtle breaking change, but perhaps not worth saving for 3.0. I think concat has very few users (judging by how long it took someone to notice concatLimit missing).

megawac
megawac approved these changes Jun 9, 2017
@hargasinski hargasinski merged commit 9c23239 into master Jun 11, 2017
3 checks passed
@hargasinski hargasinski deleted the concatLimit branch Jun 11, 2017
@hargasinski
Copy link
Collaborator Author

@hargasinski hargasinski commented Jun 11, 2017

@aearly after thinking about it a little more, it could technically fall under a bug fix, i.e. a patch release. Either way, I'll open a PR for it either tonight or at some point tomorrow. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

4 participants