underscore
underscore copied to clipboard
jashkenas
_.union doesn't work with arrays of objects
_.union will always produce duplicates when passed arrays of objects.
e.g. _.union( [ { a:1 } ], [ { a:1 } ]) will return [ { a:1 }, { a:1 } ]
Perversely, underscore's own isEqual function will tell you that the objects in question are equal. Maybe we could have a flag/option which dictates the equality comparison to use, or the option to pass in a comparator?
I'm surprised it doesn't already accept the comparison function. :+1:
It's worth pointing out that this applies to all the other array computation functions like difference, intersection, unique, etc.
Would be nice if the full suite of array functions could be updated to allow use of a different equality comparator for objects.
Wouldn't it be easier if we would have an option that compares equality based on boolean parameter value that could be passed to _.union() function? If it is true then it would automatically compare all objects in that array.
E.g. _.union([1, 2, 3, 10, [{a:1}, {a:1}]], true), would output [1,2,3,10, {a:1}]
@amiral84 No. That is unrelated. If you want that behaviour, compose union with flatten.
@amiral84 It appears so. The feature request is explained fully and succinctly in the first comment.
the underlying problem seems to be in _.uniq as _.union is merely a wrapper function for unique and flatten.
_.union = restArgs(function(arrays) {
return _.uniq(flatten(arrays, true, true));
});
This thread inspired me to add _.intersectionWith, _.differenceWith, _.unionWith, and _.uniqWith to handle comparison customization in my own code.
var array = [ { 'a': 1, 'b': 2 }, { 'a': 1, 'b': 3 }, { 'a': 1, 'b': 2 } ];
_.uniqWith(array, _.isEqual);
// => [{ 'a': 1, 'b': 2 }, { 'a': 1, 'b': 3 }]
or something along the lines of _.isCollection to determine if you are dealing with a collection. If dealing with a collection, then the comparisons should use _.isEqual instead of === which does no good in the case of a collection.
@dperrymorrow
should use
_.isEqualinstead of===which does no good in the case of a collection.
Dynamic switching sounds like a bad idea. JS uses === or SameValueZero comparisons for many things. If there's a need to go outside those comparisons something like _.uniqWith would do.
Thanks for this @jdalton, the _opWith functions you mention are absolutely perfect for what I'm trying to achieve. Any idea when they'll be available via release?
@jdalton good point on the comparisons, but wouldn't you usually use a key for unique on a collection rather than forcing Underscore to detect the entire difference between the objects?
Wouldn't the following solve @wilhen01 's request (albeit more verbose than desired)
_.chain([{ a: 1 }]).union( [{a: 1}]).unique('a').value();
//=> [{a: 1}]
Wouldn't the following solve @wilhen01 's request (albeit more verbose than desired)
_.uniq already supports that.
right, thats my point, the above code works currently as posted. could't you just call uniq / unique with a key on the result of the union?
ok, gotcha, sorry... Im not trying to be belligerent, just wanted to totally understand the problem. Id love to submit a pull request on the _.uniqWith function.
_.intersectionWith, _.differenceWith, _.unionWith, and _.uniqWith
Wouldn't it be a nicer API to just allow the comparison function to be optionally passed as the final argument, instead of minting four new functions?
@jashkenas
Wouldn't it be a nicer API to just allow the comparison function to be optionally passed as the final argument, instead of minting four new functions?
Yes, it could be done but there's complications because methods like _.uniq already support passing an iteratee and are heavily overloaded with support for binary/sorted search flags and context params too . This would mean introducing arity sniffing which feels too clever for this situation. This would also complicate future modularization efforts because it bundles lots of optional functionality into a single point when the implementations could be simplified and split into separate methods.
Right, a totally unfortunate design issue. But making new functions just to allow a comparator doesn't feel like the right solution either.
ok so additional comparison function parameters are the way to go here then? If so I can update my pull request.
the only tricky part I forsee is making the parameter parsing a little more hairy as @jdalton mentioned above.
_.uniq = _.unique = function(array, isSorted, iteratee, context) {
if (!_.isBoolean(isSorted)) {
context = iteratee;
iteratee = isSorted;
isSorted = false;
}
//...
Perhaps adding an additional check to see if isSorted _.isFunction then treat it as the comparator.
@jashkenas
But making new functions just to allow a comparator doesn't feel like the right solution either.
It may be the best option for a bad situation. I've gone on a kick of splitting out overloaded functionality recently and have been pretty happy with the result. Though it increases the API surface it allows for simpler implementations and grouping of similarly themed methods like maxBy, uniqBy, pickBy, or uniqWith, unionWith, zipWith, or sortedIndexBy, sortedIndexOf, sortedUniq. In the case of uniq though I still use a shared base function at the moment.
I'm :+1: for uniqBy or uniqWith. I would be completely against overloading uniq further (as #2368 currently is proposed)
Fwiw lodash will be using uniqBy as the split out form of _.uniq(array, iteratee) and _.uniqWith as the form to allow comparator customization.
should I pull request on Lodash with the separate method then? I thought the two projects merged, am i mistaken?
@dperrymorrow
should I pull request on Lodash with the separate method then
No need, they're already in lodash's edge master branch.
I thought the two projects merged, am i mistaken?
Not yet. Lodash v4 proofs out some of the ideas of the merge though.
@jdalton Can you please elaborate a liitle more on implementation of _.uniqWith with other iteratee.
@Pavnii
Sure. You can check out lodash/npm/_baseUniq.
If a comparator is passed it uses arrayIncludesWith helper to do the check instead of arrayIncludes (Underscore's contains).