[#1348] initial groupBy implementation by hargasinski · Pull Request #1364 · caolan/async

hargasinski · 2017-02-06T07:00:29Z

My attempt at implementing groupBy for #1348 (and I guess #214). This is my first feature implementation for async, so feedback is greatly appreciated.

I may have been a little excessive with the testing, as I ported a lot of the generic tests (like ensuring groupByLimit follows the concurrency limit) from mocha_test/map.js, while including a few specific groupBy tests. Also, the example I used for the docs isn't that great as I couldn't really come up with anything concise but illustrative. I'd like to change it before merging, if anyone has any ideas.

aearly

Overall, LGTM. The implementation is pretty simple. 👍

aearly · 2017-02-06T23:52:07Z

+ * @param {Function} [callback] - A callback which is called when all `iteratee`
+ * functions have finished, or an error occurs. Result is an `Object` whoses
+ * properties are arrays of values which returned the corresponding key.
+ * @example


I think this is a decent example.

aearly · 2017-02-06T23:53:43Z

+export default doParallelLimit(function(eachFn, coll, iteratee, callback) {
+    callback = callback || noop;
+    coll = coll || [];
+    var result = {};


What do you think about having result = Object.create(null)? Then instead of hasOwnProperty.call(...) down below, you could just do key in result.

I prefer to avoid using Object.create(null) as the behaviour can not be emulated consistently in older environments. I prefer using standard objects + hasOwn (hasOwn tends to be faster anyway last time I checked)

I wouldn't mind using Object.create(null) internally, but I'd also prefer to avoid using it for objects returned to the user. The user might use something like result.hasOwnProperty(anImportantKey) somewhere later in the code, which would result in a TypeError.

Object.create(null) is ES5, which is our minimum required version as of 2.0. @hargasinski brings up a good point though, best not to use it because of that.

aearly · 2017-02-06T23:55:22Z

+            });
+        });
+
+        it('with reflect', function(done) {


I don't think the reflect tests are useful here.

Good point, I'll remove them, thanks!

megawac · 2017-02-07T01:45:35Z

+ * correspond to the values passed to the `iteratee` callback. Note: Since this
+ * function applies the `iteratee` to each item in parallel, there is no
+ * guarantee that the `iteratee` functions will complete in order and there is
+ * no guarantee that grouped items will appear in the same order as in `coll`.


Reconsidering about groupBy again I question whether its better implemented synchronously after all the asynchronous work has been resolved, as making the order fixed in an asynchronous processing flow would make things much more complicated

Just to make sure I understand, do you mean kinda like ./internal/filter.js where it asynchronously transforms the values, but synchronously filters them afterwards? I could look into that tomorrow.

megawac · 2017-02-07T01:46:34Z

+        }, val * 25);
+    }
+
+    context('groupBy', function() {


Do we use context anywhere else?

Yes, I was looking at the mapValues tests when I added that. We use it in the compose and queue tests too.

It's more personal preference than anything, but I feel like it makes the test results easier to read and it forces them to be more organized:

hargasinski · 2017-02-20T01:46:55Z

Updated the PR to use a synchronous groupBy implementation.

/cc @megawac @aearly

hargasinski · 2017-02-20T01:48:55Z

+        var hasOwnProperty = Object.prototype.hasOwnProperty;
+
+        for (var i = 0; i < mapResults.length; i++) {
+            if (mapResults[i]) {


Code style question: is there a preference for:

if (!mapResults[i]) continue; // do something ...

versus

if (mapResults[i]) { // do something ... }

I like the continue style, but that's just like... ...my opinion, man.

aearly · 2017-02-25T22:22:53Z

BTW, the CI failures for this were bogus, so I'd say this is 👍 to merge.

hargasinski · 2017-02-26T18:40:58Z

I wanted to make sure @megawac is good with the implementation before merging.

hargasinski · 2017-03-23T03:53:33Z

@aearly @megawac should we publish v2.2.0?

aearly · 2017-03-24T20:27:23Z

Yep, there's a couple other enhancements ready too.

aearly · 2017-03-24T21:20:52Z

@hargasinski I'll let you do the honors, otherwise I'll do it later this evening.

hargasinski · 2017-03-25T00:29:05Z

@aearly thank you, but I don't have permission to publish async (I'm not listed as a collaborator on npm). You'll have to do it 😅. I did update the changelog for v2.2.0 though.

aearly · 2017-03-25T20:57:18Z

Ok, published! Thanks for writing the changelog.

Kikobeats · 2017-03-25T22:14:15Z

Just to know, there is no documentation for this? or was not generated 🤔

aearly · 2017-03-25T22:49:55Z

There was an error publishing docs. Are we using the gh-pages branch, or the docs/ folder now? Either way, make publish-docs needs to be fixed.

hargasinski · 2017-03-25T22:50:44Z

I just updated the gh-pages, I'll look into make publish-docs now. Thanks for catching this @Kikobeats.

Edit: we're still using the gh-pages branch.

hargasinski · 2017-03-28T05:06:34Z

@aearly rerunning npm install; npm install gh-pages-deploy -g appears to have resolved the issue for me.

I couldn't figure out what was causing the exact issue (it might have been a bug in an older version of duplexify they were using), so I looked into alternatives just in case. The simplest one I found is git-directory-deploy. It's similar to gh-pages-deploy, though it has two downsides: it doesn't confirm the commands beforehand, and it appends the given message to previous commit's message. The command would be: git-directory-deploy --directory docs --branch gh-pages --message "gh-pages update". We could also use git directly as suggested by this gist. The downside to that is it requires tracking the docs folder.

For now, if updating gh-pages-deploy works, I'd suggest we stick to that, as it appears to be the best option.

aearly · 2017-03-28T06:01:44Z

Thanks for looking in to this. It appears there was an incompatibility with Node 6.3 streams that was making it fail: mafintosh/duplexify@acf662f (I think I last published with Node 6.2)

Updating it fixes things. I say we keep with gh-pages-deploy with this issue resolved.

initial groupBy implementation

bf88ab8

hargasinski added feature feedback-wanted labels Feb 6, 2017

hargasinski self-assigned this Feb 6, 2017

hargasinski requested review from aearly and megawac February 6, 2017 07:00

aearly approved these changes Feb 6, 2017

View reviewed changes

megawac reviewed Feb 7, 2017

View reviewed changes

groupBy PR [caolan#1364] fixes

434d047

hargasinski commented Feb 20, 2017

View reviewed changes

megawac approved these changes Feb 27, 2017

View reviewed changes

hargasinski merged commit bdc3d81 into caolan:master Feb 27, 2017

hargasinski deleted the groupBy branch February 27, 2017 05:05

hargasinski mentioned this pull request Feb 27, 2017

Feature Request: Partition #1348

Closed

dependabot-preview Bot mentioned this pull request Mar 21, 2018

Update async requirement to 2.6.0 randallcara/vso-agent#14

Closed

Conversation

hargasinski commented Feb 6, 2017

Uh oh!

aearly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hargasinski commented Feb 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aearly commented Feb 25, 2017

Uh oh!

hargasinski commented Feb 26, 2017

Uh oh!

hargasinski commented Mar 23, 2017

Uh oh!

aearly commented Mar 24, 2017

Uh oh!

aearly commented Mar 24, 2017

Uh oh!

hargasinski commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aearly commented Mar 25, 2017

Uh oh!

Kikobeats commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aearly commented Mar 25, 2017

Uh oh!

hargasinski commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hargasinski commented Mar 28, 2017

Uh oh!

aearly commented Mar 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hargasinski commented Mar 25, 2017 •

edited

Loading

Kikobeats commented Mar 25, 2017 •

edited

Loading

hargasinski commented Mar 25, 2017 •

edited

Loading