Skip to content

Commit

Permalink
Fixes #733 -- quick-fix implementation of isEqual on naked objects fr…
Browse files Browse the repository at this point in the history
…om different frames.
  • Loading branch information
jashkenas committed Sep 19, 2012
1 parent d7fdfa7 commit 4efca3e
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 2 deletions.
4 changes: 4 additions & 0 deletions test/objects.js
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,9 @@ $(document).ready(function() {
ok(_.isEqual(isEqualObjClone, isEqualObj), 'Commutative equality is implemented for objects with custom `isEqual` methods');
ok(!_.isEqual(isEqualObj, {}), 'Objects that do not implement equivalent `isEqual` methods are not equal');
ok(!_.isEqual({}, isEqualObj), 'Commutative equality is implemented for objects with different `isEqual` methods');

// Objects from another frame.
ok(_.isEqual({}, iObject));
});

test("isEmpty", function() {
Expand Down Expand Up @@ -378,6 +381,7 @@ $(document).ready(function() {
parent.iNull = null;\
parent.iBoolean = new Boolean(false);\
parent.iUndefined = undefined;\
parent.iObject = {};\
</script>"
);
iDoc.close();
Expand Down
9 changes: 7 additions & 2 deletions underscore.js
Original file line number Diff line number Diff line change
Expand Up @@ -847,8 +847,13 @@
}
}
} else {
// Objects with different constructors are not equivalent.
if ('constructor' in a != 'constructor' in b || a.constructor != b.constructor) return false;
// Objects with different constructors are not equivalent, but `Object`s
// from different frames are.
var aCtor = a.constructor, bCtor = b.constructor;
if (aCtor !== bCtor && !(_.isFunction(aCtor) && (aCtor instanceof aCtor) &&

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra Sep 19, 2012

Collaborator

aCtor instanceof aCtor? Shouldn't that only be true for the native Function and Object constructors? What kind of test is that?

This comment has been minimized.

Copy link
@jashkenas

jashkenas Sep 19, 2012

Author Owner

That's what it's for, unfortunately. To test that the constructor is Object, even if from a different frame. If you have a better test, I'd love to replace it.

This comment has been minimized.

Copy link
@jdalton

jdalton Sep 19, 2012

Contributor

@michaelficarra Your original check caused only Object objects to fail cross-document tests with isEqual. This avoids that.

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra Sep 19, 2012

Collaborator

@jashkenas:

To test that the constructor is Object

Or Function, right? ok(_.isEqual(function(){}, iFunction));

This comment has been minimized.

Copy link
@jashkenas

jashkenas Sep 19, 2012

Author Owner

That's actually still returning false, which I think is fine.

This comment has been minimized.

Copy link
@jdalton

jdalton Sep 19, 2012

Contributor

That's actually still returning false, which I think is fine.

Right, because that gets into the whole deep compare of custom props on regexes, arrays, and the like vs. common-use/practical comparisons.

_.isFunction(bCtor) && (bCtor instanceof bCtor))) {
return false;
}
// Deep compare objects.
for (var key in a) {
if (_.has(a, key)) {
Expand Down

0 comments on commit 4efca3e

Please sign in to comment.