Concerns with deep equality algorithm

[meeber]

We had a discussion in #837 regarding concerns with the deep equality algorithm. We decided to keep the current behavior for now, and perhaps take another look at it post-4.0. I think that's still a good strategy; I'm creating this issue merely for post-4.0 considerations.

Having the deep equality traverse through both own and inherited properties leads to weird situations like this:

function ClassA () {}
ClassA.prototype.val = 5;

function ClassB () {}
ClassB.prototype.val = 7;

var objA = new ClassA();
var objB = new ClassB();

expect(objA).to.not.deep.equal(objB); // They're not deep equal, as expected

objB.val = 5;  // Shadow classB.prototype.val

expect(objA).to.deep.equal(objB); // Now they're deep equal, but should they be?

// Afterall, their prototypes aren't equal. In fact, they're not even deep equal:
expect(ClassA.prototype).to.not.deep.equal(ClassB.prototype);

Can two objects be considered deep equal if their prototypes aren't deep equal?

[meeber]

@chaijs/chai I've been thinking about this again, somewhat in relation to chaijs/check-error#18 (comment) and #1065. Error objects are going to need some custom logic in the deep equality algorithm to ignore the stack trace. However, I think we should also consider changing how deep-eql handles object prototypes when comparing any object, not just Error instances. This idea is discussed in detail in #837. My current opinion is that the best option is: "Perform deep comparison of own enumerable properties plus strict equality check on prototype."

Examples of why:

it('test', function () {
  function ClassA (val) {
    this.val = val;
  }
  function ClassB (val) {
    this.val = val;
  }

  // Currently passes as expected:
  expect(new ClassA('blah')).to.deep.equal(new ClassA('blah'));
  // I expect this to pass but it currently fails because prototypes are ignored:
  expect(new ClassA('blah')).to.not.deep.equal(new ClassB('blah'));

  // I expect this to pass but it currently fails due to the stack trace issue:
  expect(new TypeError('blah')).to.deep.equal(new TypeError('blah'));
  // I expect this to pass and it currently it does, but if we fix the stack trace issue
  // then it'll start to fail unless we compare prototypes:
  expect(new TypeError('blah')).to.not.deep.equal(new ReferenceError('blah'));
});

[keithamus]

I think deep-eql should special case when it "sees" something like {}. We should do something like:

• Check all enumerable properties
• If there are no enumerable properties check for non-enumerable
• If there are no non-enumerable properties check for Symbols or other weird things
• Finally if the object is literally just {} then pass. Otherwise fail.

I've added this to our roadmap. So let's track it there.