[#1348] initial groupBy implementation #1364

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

lib/groupBy.js

+ * @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

lib/groupByLimit.js

+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

mocha_test/groupBy.js

+            });
+        });
+
+        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

lib/groupBy.js

+ * 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

mocha_test/groupBy.js

+        }, 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

lib/groupByLimit.js

+        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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1348] initial groupBy implementation #1364

[#1348] initial groupBy implementation #1364

hargasinski commented Feb 6, 2017

aearly left a comment

aearly Feb 6, 2017

aearly Feb 6, 2017

megawac Feb 7, 2017

hargasinski Feb 7, 2017

aearly Feb 7, 2017

aearly Feb 6, 2017

hargasinski Feb 7, 2017

megawac Feb 7, 2017

hargasinski Feb 7, 2017

megawac Feb 7, 2017

hargasinski Feb 7, 2017

hargasinski commented Feb 20, 2017

hargasinski Feb 20, 2017

aearly Feb 20, 2017

aearly commented Feb 25, 2017

hargasinski commented Feb 26, 2017

hargasinski commented Mar 23, 2017

aearly commented Mar 24, 2017

aearly commented Mar 24, 2017

hargasinski commented Mar 25, 2017 •

edited

aearly commented Mar 25, 2017

Kikobeats commented Mar 25, 2017 •

edited

aearly commented Mar 25, 2017

hargasinski commented Mar 25, 2017 •

edited

hargasinski commented Mar 28, 2017

aearly commented Mar 28, 2017

[#1348] initial groupBy implementation #1364

[#1348] initial groupBy implementation #1364

Conversation

hargasinski commented Feb 6, 2017

aearly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hargasinski commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aearly commented Feb 25, 2017

hargasinski commented Feb 26, 2017

hargasinski commented Mar 23, 2017

aearly commented Mar 24, 2017

aearly commented Mar 24, 2017

hargasinski commented Mar 25, 2017 • edited

aearly commented Mar 25, 2017

Kikobeats commented Mar 25, 2017 • edited

aearly commented Mar 25, 2017

hargasinski commented Mar 25, 2017 • edited

hargasinski commented Mar 28, 2017

aearly commented Mar 28, 2017

hargasinski commented Mar 25, 2017 •

edited

Kikobeats commented Mar 25, 2017 •

edited

hargasinski commented Mar 25, 2017 •

edited