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

preserve order, make variadic and handle falsy values in concat #1436

Merged
merged 6 commits into from
Jun 23, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
preserve order in concat
  • Loading branch information
hargasinski committed Jun 12, 2017
commit f37078ff5b6c740fe57bdffdb0378dda38737a83
6 changes: 3 additions & 3 deletions lib/concat.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import concat from './internal/concat';
import doParallel from './internal/doParallel';
import doLimit from './internal/doLimit';
import concatLimit from './concatLimit';

/**
* Applies `iteratee` to each item in `coll`, concatenating the results. Returns
Expand All @@ -26,4 +26,4 @@ import doParallel from './internal/doParallel';
* // files is now a list of filenames that exist in the 3 directories
* });
*/
export default doParallel(concat);
export default doLimit(concatLimit, Infinity);
20 changes: 17 additions & 3 deletions lib/concatLimit.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import doParallelLimit from './internal/doParallelLimit';
import concat from './internal/concat';
import noop from 'lodash/noop';
import mapLimit from './mapLimit';

/**
* The same as [`concat`]{@link module:Collections.concat} but runs a maximum of `limit` async operations at a time.
Expand All @@ -19,4 +19,18 @@ import concat from './internal/concat';
* containing the concatenated results of the `iteratee` function. Invoked with
* (err, results).
*/
export default doParallelLimit(concat);
// export default doParallelLimit(concat);
export default function(coll, limit, iteratee, callback) {
callback = callback || noop;
mapLimit(coll, limit, iteratee, function(err, mapResults) {
var result = [];

for (var i = 0; i < mapResults.length; i++) {
if (mapResults[i]) {
result = result.concat(mapResults[i]);
}
}

return callback(err, result);
});
}
6 changes: 3 additions & 3 deletions lib/concatSeries.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import concat from './internal/concat';
import doSeries from './internal/doSeries';
import doLimit from './internal/doLimit';
import concatLimit from './concatLimit';

/**
* The same as [`concat`]{@link module:Collections.concat} but runs only a single async operation at a time.
Expand All @@ -19,4 +19,4 @@ import doSeries from './internal/doSeries';
* containing the concatenated results of the `iteratee` function. Invoked with
* (err, results).
*/
export default doSeries(concat);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the last use of doSeries?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. From the history, it looks like all of the other doSeries were converted to doLimit(<function_name>Limit, 1); when the *Series functions were implemented in terms of *Limit here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's time to delete it, then!

export default doLimit(concatLimit, 1);
11 changes: 0 additions & 11 deletions lib/internal/concat.js

This file was deleted.

8 changes: 0 additions & 8 deletions lib/internal/doSeries.js

This file was deleted.

15 changes: 14 additions & 1 deletion mocha_test/concat.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe('concat', function() {
}, x*25);
};
async.concat([1,3,2], iteratee, function(err, results){
expect(results).to.eql([1,2,1,3,2,1]);
expect(results).to.eql([1,3,2,1,2,1]);
expect(call_order).to.eql([1,2,3]);
assert(err === null, err + " passed instead of 'null'");
done();
Expand All @@ -34,6 +34,19 @@ describe('concat', function() {
});
});

it('concat preserves order', function(done) {
var arr = [30, 15];
async.concat(arr, function(x, cb) {
setTimeout(function() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another way to test this besides expecting it to be a certain way after a timeout? This kind of thing leads to flaky tests.

Copy link
Collaborator Author

@hargasinski hargasinski Jun 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, fixed. Thank you! It now uses async.setImmediate to wait arr.length event loop cycles before checking the result.

The *Limit versions of parallel, each, map and groupBy use a similar test to the other before the fix. When I get a chance, I'll look into updating them to use async.setImmediate as well.

cb(null, x);
}, x);
}, function(err, result) {
expect(err).to.eql(null);
expect(result).to.eql(arr);
done();
})
});

it('concatSeries', function(done) {
var call_order = [];
var iteratee = function (x, cb) {
Expand Down
8 changes: 8 additions & 0 deletions mocha_test/es2017/asyncFunctions.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,14 @@ module.exports = function () {
});
});

it('should handle async functions in concatLimit', (done) => {
async.concatLimit(input, 2, asyncIdentity, (err, result) => {
expect(err).to.eql(null);
expect(result).to.eql(input);
done(err);
});
});

it('should handle async functions in concatSeries', (done) => {
async.concatSeries(input, asyncIdentity, (err, result) => {
expect(result).to.eql(input);
Expand Down