Update deep-eql to version 1.0.0 🚀

[greenkeeperio-bot]

Hello lovely humans,

deep-eql just published its new version 1.0.0.

State	Update 🚀
Dependency	deep-eql
New version	1.0.0
Type	dependency

This version is not covered by your current version range.

Without accepting this pull request your project will work just like it did before. There might be a bunch of new features, fixes and perf improvements that the maintainers worked on for you though.

I recommend you look into these changes and try to get onto the latest version of deep-eql.
Given that you have a decent test suite, a passing build is a strong indicator that you can take advantage of these changes by merging the proposed change into your project. Otherwise this branch is a great starting point for you to work on the update.

Do you have any ideas how I could improve these pull requests? Did I report anything you think isn’t right?
Are you unsure about how things are supposed to work?

There is a collection of frequently asked questions and while I’m just a bot, there is a group of people who are happy to teach me new things. Let them know.

Good luck with your project ✨

You rock!

🌴

[meeber]

Summary of issues:

• Objects that are identical except for different prototypes are no longer considered deeply equal even when their prototypes are deeply equal. Example:

var obja = Object.create({ tea: 'chai' })
  , objb = Object.create({ tea: 'chai' });

assert.deepEqual(obja, objb);  // Fails

• Circular objects are no longer considered deeply equal. Example:

var circularObject = {}
  , secondCircularObject = {};
circularObject.field = circularObject;
secondCircularObject.field = secondCircularObject;

assert.deepEqual(circularObject, secondCircularObject);  // Fails

The second assertion feels like it should pass. I'm not sure how I feel about the first assertion. Should the "deepness" check extend up both objects' prototype chains?

[lucasfcosta]

@meeber I totally agree that the example with circular objects should pass.
However, IMO, deep-equality should be really be applied all the way up the prototype chain, otherwise objects are not deep-equal as the module name itself says.

[meeber]

I misinterpreted the situation regarding the first example:

var obja = Object.create({ tea: 'chai' })
  , objb = Object.create({ tea: 'chai' });

assert.deepEqual(obja, objb);  // Fails

The above test used to pass because the previous version of deep-eql checks all enumerable properties, both own and inherited, whereas the new version only checks own enumerable properties but also performs a strict equality comparison on the objects' prototypes. The new behavior is more in line with node's assert.deepStrictEqual. Other implementations, such as node's assert.deepEqual and lodash's isEqual, also only check own enumerable properties, but don't pay any attention to the objects' prototypes.

I think a strong argument could be made for any implementation:

• Perform deep comparison of own enumerable properties plus strict equality check on prototype
• Perform deep comparison of own enumerable properties and ignore prototype
• Perform deep comparison of both own and inherited enumerable properties and ignore prototype

Hell, you could even make an argument for any of the above implementations to also include non-enumerable properties.

I don't think there's a single right answer here. Personally, I prefer the first (new deep-eql) or the third (old deep-eql) over the second (lodash and node's assert.deepEqual). But I don't currently have a preference between the first and third.

Whatever we decide should be documented in deep-eql's readme and either noted or linked in chai's deep equal jsdoc.

Thoughts @keithamus @lucasfcosta @vieiralucas @shvaikalesh?

[meeber]

Regarding the second example:

var circularObject = {}
  , secondCircularObject = {};
circularObject.field = circularObject;
secondCircularObject.field = secondCircularObject;

assert.deepEqual(circularObject, secondCircularObject);  // Fails

There's actually a test in deep-eql demonstrating that such a comparison should indeed return false:

it('returns false with recursive objects of differing values', function () {
  var objectA = { foo: 1 };
  var objectB = { foo: 1 };
  objectA.bar = objectB;
  objectB.bar = objectA;
  assert(eql(objectA, objectB) === false,
    'eql({ foo: 1, bar: -> }, { foo: 1, bar: <- }) === false');
});

I don't follow the logic on why this comparison should return false. @keithamus help!

[lucasfcosta]

@meeber thanks for the detailed description!
Personally I agree with you regarding the prototype issue, IMO the first option seems to be the best, since objects that inherit from different prototypes are almost always viewed as different and it makes no sense (99.99% of the times) to have two deep equal prototypes.

Regarding the circular reference, since you mentioned deep-eql tests that explicitly I'd like to hear what @keithamus thinks so I can have a better base to think and express my opinion.

[meeber]

The failing tests from this PR seem to be the only real blocker currently for a 4.0 canary release. It's just a matter of coming to an understanding on the desired behavior for a couple scenarios. Pinging @keithamus @lucasfcosta @vieiralucas @shvaikalesh for feedback!

[keithamus]

Ah sorry, yes, forgot about this one.

My opinion is that we should do the old behaviour wherever we can. The intent of the rewrite was not to break the fundamentals of how it worked like this, so these are, IMO bugs

[meeber]

Regarding the first issue: The new "bugged" behavior of performing a strict equality check between the objects' prototypes is actually more in-line with how Node's assert.deepStrictEqual works. Also, re-implementing the old behavior of performing a deep equality check on all enumerable properties, both own and inherited, will likely have a noticeable performance impact. I'd actually be in favor of introducing a breaking change with this new behavior. But I'm not opposed to reverting to the old behavior, either, if that's preferred by the team. I think it's a judgement call and pretty close either way.

Regarding the second issue: I haven't looked too closely at what's causing the failed test with circular dependencies in otherwise empty objects but I suspect it's related to these lines temporarily setting the operands to false.

[vieiralucas]

To me the first option describe by @meeber LGT.M

Perform deep comparison of own enumerable properties plus strict equality check on prototype

As @lucasfcosta said. objects that inherit from different prototypes are almost always viewed as different

[lucasfcosta]

Hmm, I have thought about it again and I'm not sure our users would like the new behavior.
The first option described by @meeber is okay, but I was doing some philosophy and I came to the conclusion that the deep comparison should indeed "deep" compare every key until they become strictly comparable values (such as Numbers, Strings etc...) throughout the whole property tree.

Also, I don't think performance will be much of a problem since we're a testing library and won't be running on production environments, our number 1 priority should be correctness.

I vote with @keithamus, let's correct deep-eql and get the old behavior back without sacrificing too much performance.

What do you think? Would you guys like to vote?

[keithamus]

We seem to be at en impasse. On one hand, it could be painful for our users - such a fundemental change. On the other hand - both @meeber and @vieiralucas agree, and I suppose I do, that if prototypes differ then those two things aren't equal. I'm not sure what the right answer is. Perhaps we can solicit @shvaikalesh's opinion?

[meeber]

Both options relate to the same concept: taking both objects' prototype into consideration when performing a deep equality comparison. They just take different paths to get there. Old: Cycle through both own and inherited properties and perform a deep equal on each one. New: Cycle through own properties only but also perform a strict equal on the objects' prototypes. I lean toward the old implementation because then it wouldn't be a breaking change. And then I lean toward the new implementation because it's more in line with Node's assert.deepStrictEqual. But conceptually, I think both are sound.

You know what, I'm gonna change my vote to "keep the old implementation unless we can come up with a strong reason why the new implementation is not only better, but so much better that it justifies a breaking change".

[shvaikalesh]

Two objects are equal if:

• Have enumerable keys (symbols? order?) with the same values (===? SameValue?).
• prev + Have the same prototypes.
• prev + Have equal descriptors of enumerable values.
• prev + Have equal [[Extensible]] slot value.
• prev + Have the same internal slots (and their values).
• prev + Have the same Realm.

Every item of the list may be a requirement for implementing deep equal algo, depending on strictness and our free time 😄 . And every each of them are correct. But I think we should choose the most mainstream one (lodash) and not break compat.

[meeber]

Switching to lodash's algorithm will be a pretty big breaking change though, not only on the topic of prototypes/inherited properties, but also concepts like +0/-0.

[keithamus]

Yeah I think picking another implementation, while saving us time, would A) deviate further from what we're already breaking and B) prevent us from doing things like changing/adding features at the cadence we would like to.

I don't think it would be too significant to get these failing tests passing in deep-eql, and I think in the interests of getting 4.0 done, we should just get it working, and think about possibly breaking this later (if ever).