Newest _.isEqual creates incompatibility with Backbone.Relational (and more!)

[kmalakoff]

Hello,

The newest _.isEqual introduces an incompatibility with Backbone.Relational, but it could be more wide-spread. I'm not sure if this bug should go here or in Backbone, you can decide!

The new check in eq() introduces array comparisons:
if (a.length === +a.length || b.length === +b.length)

Backbone.Collection reset() sets a length variable:
this.length = 0;

And Backbone.Model set() checks for a change in attribute using:
if (!_.isEqual(now[attr], val))

Finally, Backbone.Relational replaces the serialized json arrays with a collection using Backbone.Model set(), but with the new _isEqual and the length variable on the Collection, when you pass an empty array with length = 0, the set doesn't occur because _.isEqual incorrectly detects that they are equal, even though one is an array and the other is a Backbone.Collection.

So I think the new _isEqual will incorrectly say anything is equal with zero length because of the new test:
if (a.length === +a.length || b.length === +b.length)

I'm really not sure what the right fix is. Is it adding a check that something is a "pure"-array like object (no non-array properties)? Is it updating Backbone.Model set() to be more strict on checking the types than _.isEqual? Is it removing the array-like implementation in Backbone.Collection?

Either way, there should be a test like equal(_.isEqual(new Backbone.Collection(), []), false, 'Backbone.Collection is array-like, but also has properties'), but without the dependency on Backbone.

If this isn't clear, please let me know.

Cheers!

[ghost]

Ah, yes. The incompatibility is due to the fact that the previous _.isEqual function used a for...in loop to compare arrays and objects, whereas the current version uses an optimized while loop for arrays and array-like objects.

A possible workaround would be to change the if (a.length === +a.length || b.length === +b.length) branch to the following:

var isArrayA = _.isArray(a), isArrayB = _.isArray(b);
if (isArrayA || isArrayB) {
  if (!isArrayA || !isArrayB) return false;
  // Compare object lengths to determine if a deep comparison is necessary.
  // ...
} else {
  // Deep compare objects.
  // ...
}

@jashkenas, @michaelficarra: What are your thoughts on this?

[michaelficarra]

@kitcambridge: I don't fully understand the problem. Does @kmalakoff not want to treat array-like objects as arrays? If that is the case (and your suggested solution makes it look that way), then I'd say Backbone is using Underscore improperly (which is actually pretty funny, considering their authors). Underscore defines array-like objects as anything with numeric length properties (but not numeric-strings, as I would prefer). If they want to use the Underscore functions, they'll need to understand that and change. And, yes, I understand we just changed this function, so it is understandable that their code broke, but it does indicate they were using the function improperly to begin with.

[kmalakoff]

@michaelficarra: what I am raising is the false positives of the new array-like check and how it has real-implications on Backbone. I came to a similar conclusion about Backbone (in my local repositories, I updated the Backbone check), but the Underscore definition of array-like still doesn't sit well with me because it ignores the possible non-array like properties of an object in a comparison. A solution that wouldn't yield "false positives" (maybe these aren't considered as false positives?) would be to both check the array-like objects in the while loop and then the non-array like properties if it passes that first check.

@kitcambridge: I did a similar solution locally (in Backbone, though), but slightly different:

var isArrayA = _.isArray(a), isArrayB = _.isArray(b); if (isArrayA || isArrayB) { if (isArrayA !== isArrayB) return false; // Compare object lengths to determine if a deep comparison is necessary. // ... } else { // Deep compare objects. // ... }

The change in 'if (isArrayA !== isArrayB)' makes it so the array-nesses match (either both arrays or both non-arrays). Otherwise, the non-arrays will never get checked. Of course, you could check a step further and make sure they are the same type (but that it even stricter and I believe @michaelficarra would prefer to keep array-like checks more flexible):

if (a.length === +a.length || b.length === +b.length) { if !((a instanceof b.constructor) || (b instanceof a.constructor)) return false; // Compare object lengths to determine if a deep comparison is necessary. // ... } else { // Deep compare objects. // ... }

Personally, I believe the most "correct" (meaning eliminating false positives) solution for array-like is to check the arrays as is correctly implemented and if passes, also check the properties. That said, if the intention is to be strict as @michaelficarra implies as was the intention, then library writers may be surprised by the additional strictness with the recent change...I was "surprised", meaning I spent a few hours trying to track down the bug across Underscore, Backbone and Backbone-relational ;-)

[kmalakoff]

@michaelficarra: this has been going around in my mind and think that maybe the constructor comparison is the best implementation:
if !((a instanceof b.constructor) || (b instanceof a.constructor)) return false;

My thought is if two array-like containers are different types, should they really be "equal" even if they have identical items because you might not be able swap one for another? (as in the example I raised).

In order words, maybe _.isEqual should be defined as, "I can swap a for b and vice-versa with no side-effects"? That way, the new while loop check is only a comparison optimization, not an equality definition.

Whatchathink?

[michaelficarra]

@kmalakoff: Unfortunately, the constructor property is writeable. I think you are trying to test that the objects are equivalent over the instanceof relation. Object.getPrototypeOf(a) === Object.getPrototypeOf(b) implies that a instanceof X === b instanceof X for any X (which is what you want). Unfortunately, Object.getPrototypeOf is ES5 only, and there's no reliable way to get the [[Prototype]] value in ES3.

I think your next suggestion is to just stop treating array-like objects differently than any other objects. And with that approach, I wouldn't have a problem. But I believe the current approach will be faster and match the expectations of most underscore users. Two arguments array-like objects should be equivalent over the _.isEqual relation, even if their callee property is not.

Speaking of non-enumerable properties, though: @kitcambridge: why do we only test the enumerable properties for object equality? Does a method even exist to enumerate the non-enumerable properties using only ES3 facilities? I can't think of one.

[kmalakoff]

@michaelficarra: thank you for the excellent reply - I'm no Javascript expert so it is great to learn this!

I am unsure of what "the expectations of most underscore users" are with regards to equality (fast, I totally agree with!). But I am sure there is a significant base with a more strict interpretation of equality...I mean Backbone itself wasn't written with this new definition of _.isEqual in mind and I can only imagine how much other code written by mere mortals will break.

That's why I kind of like the "I can swap a for b and vice-versa with no side-effects" definition of equality..I think as part of the underscore code comments, the definition should be provided. Currently they read 'Perform a deep comparison to check if two objects are equal.'...deep sort of sounds as you imply, not just testing part of an object.

I look forward to seeing what you and @kitcambridge propose.

[jashkenas]

So, looking at this patch more closely -- it appears that we shipped some seriously funky equality comparisons with 1.2.0.

_.isEqual({length: 0, other: 'thing'}, []);

... should definitely be false. Deep object equivalence should be implemented in terms of for in semantics ... and objects with the same own properties but different prototypes are not the same. The above commit is my attempt to rectify the situation and fix @kmalakoff's problem. I'm of the inclination to quickly ship a 1.2.1 update with this change, but I'd appreciate it if @michaelficarra and @kitcambridge have an opportunity to look over the changes, particularly to the tests, and let us know what you think.

[michaelficarra]

Yeah, I was definitely worried about shipping a new version so quickly after those isEqual changes landed. I'll take a look at that commit shortly.

[ghost]

@michaelficarra:

Speaking of non-enumerable properties, though: @kitcambridge: why do we only test the enumerable properties for object equality? Does a method even exist to enumerate the non-enumerable properties using only ES3 facilities? I can't think of one.

Not in ES 3, unfortunately. Object::propertyIsEnumerable can test if a property is enumerable, but the name of the property in question must be known.

[jashkenas]

Thanks for looking over these changes, Kit and Michael ... closing the ticket.