Repository: jashkenas/underscore (js)

Issue:
1.9 regression failing in groupBy
Hi folks, I couldn't find an issue containing the `groupBy` function. So I'm creating one. Please redirect if I missed anything. Thanks for all the hard work. Appreciate it!

When updating from version 1.8.3 to 1.9.0 the behaviour of `groupBy` changed in a way that grouping by arrays was possible in 1.8.3, but it's not possible in 1.9.0. At least not as I'm expecting it to work 🤔 

The below test would succeed in 1.8.3 where it throws an exception in 1.9.0. Am I doing something wrong?

```javascript
var anArray = [1,2];
var list = [{
	arrayProp: anArray,
	otherProp: 1
}, {
	arrayProp: anArray,
	otherProp: 2
}];

var res = _.groupBy(list, 'arrayProp');
// var res = _.groupBy(list, function(element) {
//  	// stringified version of key is equal and gets grouped
//  	return JSON.stringify(element.arrayProp);
// });
var groupByKey = Object.keys(res)[0];
console.log(Object.keys(res).length) // 1
console.log(groupByKey); // 1,2

var expected = 2
var actual = res[groupByKey].length;
if (actual !== expected) {
	throw new Error("TEST FAILURE. EXPECTED " + 2 + ". GOT " + actual)
}
```

`-> Uncaught Error: TEST FAILURE. EXPECTED 2. GOT 1`

Pull request description:
Add `has` internal function
In some cases, `_.countby()` and `_.groupBy()`, we want to support
arrays as valid keys, since they can be coerced to strings. In the other
cases, the keys are known to always be strings anyway, so we can save
having to do the `_.isArray` check.

Fixes #2756.

Patch:
diff --git a/underscore.js b/underscore.js
index e4959b4..d19a206 100644
--- a/underscore.js
+++ b/underscore.js
@@ -148,6 +148,10 @@
     };
   };
 
+  var has = function(obj, path) {
+    return obj != null && hasOwnProperty.call(obj, path);
+  }
+
   var deepGet = function(obj, path) {
     var length = path.length;
     for (var i = 0; i < length; i++) {
@@ -445,7 +449,7 @@
   // Groups the object's values by a criterion. Pass either a string attribute
   // to group by, or a function that returns the criterion.
   _.groupBy = group(function(result, value, key) {
-    if (_.has(result, key)) result[key].push(value); else result[key] = [value];
+    if (has(result, key)) result[key].push(value); else result[key] = [value];
   });
 
   // Indexes the object's values by a criterion, similar to `groupBy`, but for
@@ -458,7 +462,7 @@
   // either a string attribute to count by, or a function that returns the
   // criterion.
   _.countBy = group(function(result, value, key) {
-    if (_.has(result, key)) result[key]++; else result[key] = 1;
+    if (has(result, key)) result[key]++; else result[key] = 1;
   });
 
   var reStrSymbol = /[^\ud800-\udfff]|[\ud800-\udbff][\udc00-\udfff]|[\ud800-\udfff]/g;
@@ -811,7 +815,7 @@
     var memoize = function(key) {
       var cache = memoize.cache;
       var address = '' + (hasher ? hasher.apply(this, arguments) : key);
-      if (!_.has(cache, address)) cache[address] = func.apply(this, arguments);
+      if (!has(cache, address)) cache[address] = func.apply(this, arguments);
       return cache[address];
     };
     memoize.cache = {};
@@ -978,7 +982,7 @@
 
     // Constructor is a special case.
     var prop = 'constructor';
-    if (_.has(obj, prop) && !_.contains(keys, prop)) keys.push(prop);
+    if (has(obj, prop) && !_.contains(keys, prop)) keys.push(prop);
 
     while (nonEnumIdx--) {
       prop = nonEnumerableProps[nonEnumIdx];
@@ -994,7 +998,7 @@
     if (!_.isObject(obj)) return [];
     if (nativeKeys) return nativeKeys(obj);
     var keys = [];
-    for (var key in obj) if (_.has(obj, key)) keys.push(key);
+    for (var key in obj) if (has(obj, key)) keys.push(key);
     // Ahem, IE < 9.
     if (hasEnumBug) collectNonEnumProps(obj, keys);
     return keys;
@@ -1279,7 +1283,7 @@
       while (length--) {
         // Deep compare each member
         key = keys[length];
-        if (!(_.has(b, key) && eq(a[key], b[key], aStack, bStack))) return false;
+        if (!(has(b, key) && eq(a[key], b[key], aStack, bStack))) return false;
       }
     }
     // Remove the first object from the stack of traversed objects.
@@ -1329,7 +1333,7 @@
   // there isn't any inspectable "Arguments" type.
   if (!_.isArguments(arguments)) {
     _.isArguments = function(obj) {
-      return _.has(obj, 'callee');
+      return has(obj, 'callee');
     };
   }
 
@@ -1371,7 +1375,7 @@
   // on itself (in other words, not on a prototype).
   _.has = function(obj, path) {
     if (!_.isArray(path)) {
-      return obj != null && hasOwnProperty.call(obj, path);
+      return has(obj, path);
     }
     var length = path.length;
     for (var i = 0; i < length; i++) {


Test patch:
diff --git a/test/collections.js b/test/collections.js
index f58a6b3..fcd7e41 100644
--- a/test/collections.js
+++ b/test/collections.js
@@ -758,6 +758,9 @@
       6: [jordan]
     };
     assert.deepEqual(_.groupBy(collection, ['stats', 'power']), expected, 'can group by deep properties');
+
+    var foos = [{foo: [1, 2]}, {foo: [1, 2]}]
+    assert.deepEqual(_.groupBy(foos, 'foo'), {"1,2": foos}, 'will coerce arrays to string keys');
   });
 
   QUnit.test('indexBy', function(assert) {
@@ -805,6 +808,9 @@
     grouped = _.countBy(array);
     assert.strictEqual(grouped['1'], 2);
     assert.strictEqual(grouped['3'], 1);
+
+    var foos = [{foo: [1, 2]}, {foo: [1, 2]}]
+    assert.deepEqual(_.countBy(foos, 'foo'), {'1,2': 2}, 'will coerce arrays to string keys');
   });
 
   QUnit.test('shuffle', function(assert) {