Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

_.isEqual for DOM elements always returns true #945

Closed
wants to merge 1 commit into from

3 participants

@rudasn

DOM element comparisons always return true as the result variable is never changed to false (in eq method).

_.isEqual(document.createElement('div'), document.createElement('div')); // => true

Added a simple test on the eq method to check for the equality of two DOM elements.

@braddunbar
Collaborator

Hi @rudasn! Thanks for the patch. It seems like a rather weak test though as it means objects with a nodeName property could return false negatives. Why not just use el1 === el2?

@rudasn

How about using _.isElement instead? It returns !!(obj && obj.nodeType === 1).

I didn't want to use a === b as I thought there was a good reason such a test wasn't included in the first place.

@braddunbar
Collaborator

Hmm...I don't quite understand. What's your use case for comparing DOM elements?

@rudasn

It's not that I want to compare DOM elements directly, but I want to be able to do something like this and get a correct response:

var x = {
        'a': document.createElement('div'),
        'b': 'B property'
    },
    y = {
        'a': document.createElement('div'),
        'b': 'B property'
    }

_.isEqual(x, y); // true

Apparently that should be false.

@braddunbar
Collaborator

Yep, this is the expected behavior. _.isEqual is for a deep comparison of two objects and two newly created DOM elements have matching properties. If you change something about one element, then the comparison would be false.

var a = document.createElement('p');
a.id = _.uniqueId('el');
var b = document.createElement('p');
b.id = _.uniqueId('el');
_.isEqual(a, b); // false

If you want identity comparison you should use === instead of _.isEqual or add a differing property to the elements.

@braddunbar braddunbar closed this
@jdalton
Collaborator

Inspecting nodes is a trap. In IE8 _.isEqual(document.createElement('div'), document.createElement('div')) will throw an error. Inspection of nodes is tricky and costly (lots of properties and child nodes to iterate over), which is why Lo-Dash detects nodes (cross-browser) and just === compares them (no crawling).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 17, 2013
  1. @rudasn
This page is out of date. Refresh to see the latest.
Showing with 21 additions and 0 deletions.
  1. +17 −0 test/objects.js
  2. +4 −0 underscore.js
View
17 test/objects.js
@@ -358,6 +358,23 @@ $(document).ready(function() {
// Objects from another frame.
ok(_.isEqual({}, iObject));
+
+ // DOM elements
+ var aDom = document.createElement('div'),
+ bDom = document.createElement('span'),
+ $aDom = $(aDom),
+ $bDom = $(bDom),
+ $allDoms = $aDom.add($bDom);
+
+ ok(!_.isEqual(aDom, bDom), 'DOM Elements are not always equal.');
+
+ ok(!_.isEqual($aDom, $bDom), 'jQuery DOM Elements are not always equal.');
+
+ ok(_.isEqual(aDom, aDom), 'Same DOM Elements are equal.');
+
+ ok(_.isEqual($aDom, $aDom), 'Same jQuery DOM Elements are equal.');
+
+ ok(_.isEqual($allDoms, $allDoms), 'Same jQuery Elements are equal.');
});
test("isEmpty", function() {
View
4 underscore.js
@@ -828,6 +828,10 @@
// Unwrap any wrapped objects.
if (a instanceof _) a = a._wrapped;
if (b instanceof _) b = b._wrapped;
+ // Compare DOM elements.
+ if (a && a.nodeName || b && b.nodeName) {
+ return a === b;
+ }
// Compare `[[Class]]` names.
var className = toString.call(a);
if (className != toString.call(b)) return false;
Something went wrong with that request. Please try again.