assert: check object prototype in deepEqual

[vkurchatkin]

Fixes: #620

R=@bnoordhuis

[bnoordhuis]

Long line, please wrap at 80 columns. If you use vim, adding the following to your .vimrc gives you a visual indicator:

set textwidth=80 set colorcolumn=+1 

[vkurchatkin]

Sorry about that. I actually DO have a guideline, but in this case I saw some long lines just above and ignored it. Anyway, wrapped this and other offending lines.

[bnoordhuis]

LGTM apart from the long lines. I'd like one or two other contributors to sign off on this as well.

Can you update the commit log with a few lines explaining what changed and why?

[vkurchatkin]

@bnoordhuis updated. I labeled this PR as semver-major because it is kind of a breaking change.

[bnoordhuis]

Don't hate me but textwidth=72 in commit logs. :-) I have a autocmd BufEnter COMMIT_EDITMSG set textwidth=72 for that in my .vimrc.

Summoning more @iojs/collaborators.

[evanlucas]

I'm +1 on this.

[indutny]

LGTM

[cjihrig]

LGTM

[seishun]

I'm not so sure, people might expect this to be CommonJS compliant, even though the spec is horribly broken (and deepEqual is just one example).

[rvagg]

I'm not convinced either, I can imagine this breaking a whole lot of userland code, let's try and solicit broader feedback, particularly from people implementing other assertion libraries and test runners.

[rvagg]

Some poeple I can think of that may be able to muster an opinion on this change (and may even be impacted by it): @substack, @cjohansen, @isaacs, @thlorenz

semver-major is also going to complicate getting this merged because we haven't even figured out how to handle semver-minor branching and releases yet.

[neumino]

Please don't change the behavior of deepEqual; consider adding a new method instead.

This makes impossible to compare an object sent over the network in the JSON format and an instance of a model without creating another instance of the model.

[bevacqua]

+1 to what @neumino said. Prototype equality means that you can't compare a model object with a plain JS object for quick assertions

[thlorenz]

I've mainly been using deepEqual to compare arrays and object literals so not much would break for what I'm aware of, but there are going to be edge cases like the one @neumino is describing.

The question is what does deepEqual mean regarding non-primitive types?

• a) all properties -- that are not builtin (like __proto__) -- are equal recursively
• b) all properties and the builtin object properties like __proto__ are equal recursively

Neither is clear from the docs:

Tests for deep equality.

I understand the current check for prototype doesn't seem to be of much use, but that would be a remnant of history we'd need to live with. Removing this check entirely could also be discussed although I'm not sure what effect on existing userland code that woulld have.

However, whether the current behavior is due to some misunderstanding or not, changing it now as proposed in this PR is problematic.
For starters, changes to the method also need to be reflected in the deep-equal and deep-ispossibly more? modules.
More painful would be to go and fix all tests breaking after upgrading the io.js version.

The CommonJS Unit testing Wiki is not a specification, but merely a collection of suggestions.

This wiki is a starting point for collecting up ideas, any draft API suggestions

Additionally io.js deviated from these in other areas as well.

Taking all this into account I'd suggest to stick with the current behavior for deepEqual and discuss an alternative method, i.e. protoEqual to just check the __proto__ or alternatively deepProtoEqual (maybe better name) to combine deepEqual and protoEqual.

[aheckmann]

-1 from me as well since it will break existing apps and isn't necessarily "more correct" behavior than current behavior. Either a new method or an option seems safer.

[thlorenz]

Do we actually have a way to test at least how many modules/tests on npm would break with this change?
That could give us more data to judge how much pain people would have to endure if we preferred correctness over backwards-compat.

[jonathanong]

LGTM, but this is semver-major like the label states.

@thlorenz i'm interested in making some acceptance test build system described in nodejs/roadmap#11, just need to plan it out in iojs/build.

[vkurchatkin]

as a non-breaking alternative: introduce deepStrictEqual which uses === and also does this

[Fishrock123]

as a non-breaking alternative: introduce deepStrictEqual which uses === and also does this

The problem is that deep and strict are mutually exclusive conceptually.

+1 to deepStrictlyQuacksLike()

[meandmycode]

I'm -1 on this personally the times I use deep equality are on things that aren't the same but are at least shaped the same. I.e. structural equivalence or 'duck typed'.

[cjohansen]

deepEqual({}, []) should definitely throw. I don't have a strong opinion on whether this means deepEquals should enforce a check on the prototype. I'm leaning towards +1 on this change.

To fill the void, you could add a function like referee's assert.match (although with less flexibility/confusing amounts of input types) - fn(a, b) that passes when a contains all the properties in b with the same (recursive check) values, but allows a to have additional properties (and different prototype).

[vkurchatkin]

thanks for the feedback! the PR seems to be too controversial to be merged. We need to go in a different direction to solve this issue.