assert: make *deepEqual() closer to *deepStrictEqual()

[Trott]

With these changes, the only difference between deepEqual() and deepStrictEqual() is that deepEqual() doesn't require the same prototype.

• The *[dD]eepEqual() functions have surprising behaviors, which is why they are deprecated. They are very easy to misuse and not easy to use correctly. It is entirely possible that most people who are using them are in fact using them incorrectly or at least using them in a way where the strict equivalent would work just fine. Even if this results in some failing tests in the ecosystem, many of those tests may in fact reveal bugs in the tests/implementation. In other words, for many people, this might be a fix, not a breaking change.
• These functions are a maintenance burden. They require mental overhead for modifications to assert and this change eliminates a good chunk of code from lib.
• We can land this as a breaking change in 13.0.0 with the intent of reverting quickly if it causes significant difficulty.
• Folks who might be invested in this change (👋 @BridgeAR!) can spend some time over the next few months looking for cases where this might cause problems and opening pull requests. There might not be very many.
• assert.deepEqual()/assert.notDeepEqual() have been deprecated for a long time, but we can never remove them. Making them near-aliases for their strict equivalents might be do-able though.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[BridgeAR]

I am in favor of something like this but it seems to break a lot of modules even in CITGM. Interestingly the tests seem to fail the prototype check and not the abstract equality check.

Modules that fail in CITGM due to a different prototype: esprima, mime and multer.
Other related failures: koa (I am not sure what it fails upon but very likely also the prototype and not the symbols).

At least multer and mime would profit from stricter prototype checks and the change should be pretty straight forward.
That does not seem to be the case for esprima and koa.

One good thing is though that not a single test fails (or at least I did not see any) because of the former use of abstract equality checks (that is the main problem with the current implementation. The other two differences are minor compared to that).

Should we maybe just change the equality and symbol checks in deepEqual for v13 but keep the prototype check as it is for now?

[Trott]

Should we maybe just change the equality and symbol checks in deepEqual for v13 but keep the prototype check as it is for now?

I had the same thought. Or maybe just a similar thought? Basically, make deepEqual() the same as deepStrictEqual() but get rid of the prototype/class check. The downside is that the names deepEqual() vs. deepStrictEqual() doesn't communicate that difference, and doesn't match the use of strict in equal() vs. strictEqual(). That's not necessarily a deal-breaker, but it's a high bar to get over.

[BridgeAR]

@Trott it is definitely not ideal when it comes to the name... since this did not show any problems with the abstract equality, we might actually just change both in one go. There will be a few more failures related to that and as you outline in the top: we have to be prepared to revert the change but I think it is worth a try.

Removing the prototype check completely does not seem ideal though. I think updating the docs and taking back the deprecation would work in this case where assert deepEqual behaves identical to assert.deepStrictEqual besides the prototype check. I will also reach out to the maintainers of esprima and koa to ask them about their opinion when it comes to the prototype check.

[Trott]

Still a WIP because the docs need to be updated (and I'm also testing each commit one-by-one to make sure there's no test failures at any point in the chain of 3 commits), but this is now closer to something that might be usable. Instead of an alias for deepStrictEqual(), there is one difference: prototypes are not checked. This should permit the esprima use case while eliminating the head-scratching results that deepEqual() can often have.

[Trott]

Turns out this should probably be just one commit. Things left to do (not necessarily consecutively):

• update the documentation
• remove WIP label
• ask for reviews
• run CI
• run CITGM

[Trott]

This is ready for reviews. @nodejs/assert

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/24202/ ✔️

[Trott]

CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1894/

CITGM against this PR (queued): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1895/

[Trott]

Rebased and force-pushed to eliminate conflict.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1928/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/24876/

[Trott]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1930/

[Trott]

CITGM with master branch for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1934/ (queued, will 404 until a worker is available)

[Trott]

CITGM looks good. This would be all clear to land on master if there are two TSC approvals and no Collborator objections. This is a rather notable change, and not one I'd want to have slip through unnoticed, so I'm going to ping widely.

@nodejs/collaborators

The upshot of this: assert.deepEqual() would now behave the same as assert.deepStrictEqual() except that it won't check that expected and actual have the same prototype.

[ljharb]

This seems to defeat the purpose of even having the loose version.

It exists because it's valuable as-is, in core - why remove the looseness?

[Trott]

It exists because it's valuable as-is, in core

I'd say not. We basically don't use it in core anymore. I don't think the loose-equality aspect of it exists to fill a demonstrable need. AFAICT, it exists because a somewhat-embryonic CommonJS spec included it, and so, very early on in Node.js's development, it was implemented according to that spec (which never gained much adoption). The Node.js implementation has evolved since then (and all-but-certainly diverges from that original spec at this point), but the loose-equal aspect of it is of questionable utility. I'm unable to locate a real-world example demonstrating the utility of the loose-equalness in .deepEqual().

- why remove the looseness?

AFAICT, that looseness is never actually why it's used. The only real-world usage of .deepEqual() that I've found that isn't better served by .deepStrictEqual() is when a prototype check needs to be bypassed. (This is the case for the tests in esprima.)

.deepEqual() has a long history of surprising end users. We don't use it in core and we've warned users away from it for a long time. It's also a maintenance burden, although it probably only burdens a small number of core devs.

This PR has already resulted in a small improvement in the ecosystem. We fixed a (very small, but nonetheless real) bug in the tests for multer. (It should have been doing strict-equality checking because multer has a bug if the types are other than expected in that particular test.)

All that said, I agree that caution is appropriate here, and if we land this change (which I think we should), we should also be prepared to revert quickly if there are large unforeseen consequences. That said, I do find the lack of CITGM problems reassuring.

[ljharb]

Let me rephrase; it's valuable to the ecosystem to have it in core, as-is.

The deep-equal package is used constantly in the ecosystem precisely for its looseness, and every test runner/framework in existence has a form of these assertions - they are valuable and have use cases.

If you're going to effectively destroy the use case of deepEqual, then I'd suggest instead deleting it entirely, and adding an option to deepStrictEqual for bypassing the [[Prototype]] check.

[Trott]

The deep-equal package is used constantly in the ecosystem precisely for its looseness

Can you (or someone else) point me to someone using it for its looseness? As you know (since you're a maintainer), it lists other selling points--runs in the browser, is faster than wrapping assert.deepEqual() in a try/catch... It provides a strict option and I suspect that most people who are using it without the strict option are doing so because it's the default and not because they need loose type checking.

they are valuable and have use cases.

Can you point me to one or two of them?

Oooh, I'll start looking through some of substack's repos to see potential use cases there....

[ljharb]

I agree that most people are using it without the strict option solely because that's the default.

However, tape, mocha, jest, ava, jasmine, etc, all have a "deep equal" that uses loose equality. Many test cases for the DOM don't want to differentiate between 3 and '3', because they all end up as strings in the DOM anyways, so the distinction doesn't matter. The same is true in node - anything you send over the wire is a string, so the only thing that matters is that they'll all stringify the same, which for many kinds of values, loose equality confirms.

[Trott]

tape, mocha, jest, ava, jasmine etc, all have a "deep equal" that uses loose equality.

AFAICT, tape is the only one of the listed tools that has a loose deep equal built in (although see caveats below). The others do not have a loose deep equality comparison. Only strict. To make sure I'm not mistaken, though /ping

• @vieiralucas (for chai--mocha does not have its own assertions)
• @SimenB (for jest)
• @novemberborn (for ava)
• @slackersoft (for jasmine)

tape's deepEqual() uses "strict comparisons (===) on leaf nodes". If you want loose equality, you have to use deepLooseEqual(). The fact that people opt into using it would seem to be an argument for keeping deepEqual() as it is, but searching on GitHub for real-world usage, I'm less sure. A (so far) cursory examination seems to suggest that the real-world uses are ones that would be fine with the strict variant instead.

Also in favor of "keep it as it is" would seem to be some chai discussions that describe use cases where someone might want deep equal to have loose comparison at the leaves. It's apparently on their roadmap but the fact that folks have lived without it for this long is also an interesting data point. chaijs/chai#644

It also seems that jasmine and jest (and possibly others) detect DOM comparisons, sort of auto-handling one of the use cases suggested above. (This can probably be used to argue things both ways. "See? People need loose deep comparisons, even if it's abstracted away from them!" vs. "See? Test tools already deal with the DOM use case. Node.js core doesn't need to and shouldn't."

[Trott]

If you're going to effectively destroy the use case of deepEqual, then I'd suggest instead deleting it entirely, and adding an option to deepStrictEqual for bypassing the [[Prototype]] check.

That would certainly solve the API design problems this change would create. I'd go very incremental on something like this:

• Add option to skip prototype check to deepStrictEqual().
• Get it in all supported release lines and released.
• Open a pull request to update esprima to use that instead of deepEqual(). Get it landed.
• Find other examples in the ecosystem where deepEqual() is only being used to bypass prototype checks. Open PRs for those too.
• Boil the sea: Open pull requests across the ecosystem to migrate away from deepEqual(). As legitimate use cases are found and/or PRs to unmaintained projects stall, stop. Reconsider your life choices.
• We'll never get here. See above bullet point. But that's OK.
• Oh, hey, now we can deprecate deepEqual() and prepare to remove it. Except we'll never get here because you can't boil the sea.

Despite the impossibility of achieving the last three bullet points, I rather like that incremental approach. (I also like the approach in this PR. I contain multitudes.)

[ljharb]

It certainly seems like it’d more carefully prove the premise of this PR, at least :-)

[SimenB]

Jest only has strict equality, yeah. I don't remember anyone asking for a loose one either (although I've only been involved in the project for 2 years, so might have come before that for all I know). @pedrottimark is the master of Jest's assertions, so maybe he can chime in on the DOM node example? We've relatively recently changed how that works and I have to admit I don't remember the details.

FWIW, I just read the post I was tagged in, the thread is a bit long to read through unless you think there's some context in here I need?

[Trott]

FWIW, I just read the post I was tagged in, the thread is a bit long to read through unless you think there's some context in here I need?

Thanks for chiming in! No, I don't think you need to read the whole thread. There are basically two questions. First, does jest have deep equality checking that uses loose equality on the leaf values? (Answer seems to be "no", which is what I thought but I wanted to make sure.) Second utterly optional question is if you have any opinions on whether it would be useful, harmful, or neither to change Node.js's assert.deepEqual() to use strict equality checking at the leaf nodes. (The only difference between assert.deepEqual() and assert.deepStrictEqual() would be that the former does not check that the two objects being compared have the same prototype.)

[novemberborn]

With AVA, the following passes:

import test from 'ava'

test('equality', t => {
  t.deepEqual({ foo: { bar: 'baz' } }, { foo: { bar: 'baz' } })
})

See https://github.com/concordancejs/concordance#comparison-details for a high-level overview of the comparison rules.

The only difference between assert.deepEqual() and assert.deepStrictEqual() would be that the former does not check that the two objects being compared have the same prototype.

It's been a long time since I wrote the code that AVA uses, but we do require string tags and constructors to be the same. Is that what you consider strict equality?

Second utterly optional question is if you have any opinions on whether it would be useful, harmful, or neither to change Node.js's assert.deepEqual() to use strict equality checking at the leaf nodes.

There's so many ways to interpret "equality" though, e.g. whether insertion order in sets and maps is significant. It might be too hard to reach consensus. The breaking changes are not worth it, it's possibly better to leave this to userland.

[Trott]

Is that what you consider strict equality?

In this particular context, I mean whether or not this should pass:

assert.deepEqual({foo: 5}, {foo: '5'});

My understanding is that there is no built-in equivalent in AVA that does deep equality but ignores types at the leaf comparisons.

There's so many ways to interpret "equality" though, e.g. whether insertion order in sets and maps is significant. It might be too hard to reach consensus.

Definitely a problem.

it's possibly better to leave this to userland.

Yeah, I'm pretty sure that if we were building Node.js all over again from the ground up, there would be no assert module. Or if there was, it would contain only assert() and not all the other methods.

[pedrottimark]

In the expect package used by Jest, toEqual deep equality calls Object.is whenever it can for leaves, and has special cases when recursive descent through own enumerable properties isn’t correct for objects like Date, Error, RegExp, and DOM nodes.

From #28011 (comment)

Many test cases for the DOM don't want to differentiate between 3 and '3', because they all end up as strings in the DOM anyways, so the distinction doesn't matter.

Yes, so a criterion is isEqualNode from DOM standard. The change a few months ago was to delete some incorrectly guarded and obsolete fallback code if isEqualNode method doesn’t exist, which was not covered in our test suite and which threw an error in a realistic user test suite.

The same is true in node - anything you send over the wire is a string, so the only thing that matters is that they'll all stringify the same, which for many kinds of values, loose equality confirms.

With respect, I don’t follow the analogy. To test a response from an endpoint like JSON.parse(stringified) deep strict equality seems more relevant than deep abstract equality.

From #28011 (comment)

The only real-world usage of .deepEqual() that I've found that isn't better served by .deepStrictEqual() is when a prototype check needs to be bypassed. (This is the case for the tests in esprima.)

Yes, I can imagine an expected criterion in literal object notation without calling constructors.

From #28011 (comment)

I'm pretty sure that if we were building Node.js all over again … it would contain only assert()

Rich, I have empathy for y’all to decide about cost and benefit of breaking changes like this.

[ljharb]

Glad to see some of my incorrect assumptions clarified; and i agree that assert probably shouldn’t have been in node - however, i still think there’s use cases for having it, and i don’t see it as having that much maintenance cost as it is - so I’d prefer to see it remain.

[Trott]

Glad to see some of my incorrect assumptions clarified; and i agree that assert probably shouldn’t have been in node - however, i still think there’s use cases for having it, and i don’t see it as having that much maintenance cost as it is - so I’d prefer to see it remain.

There's no chance assert will be removed from Node.js, or at least no chance as long as Node.js is in widespread use. The proposed breaking change here is only about making assert.deepEqual() behave more like assert.deepStrictEqual() and in a (hopefully) less surprising way.

That said, I'm finding #28011 (comment) appealing. It shifts the hardship away from the ecosystem a little bit. It's also destined to stall at some point, but when it does, the proposal here can be re-visited and we'd be able to say that we did a bunch of work to minimize impact on the ecosystem.

[ljharb]

Agreed!