Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enable non-strict comparisons on assert module #28780

Closed
bruno-brant opened this issue Jul 20, 2019 · 3 comments · Fixed by #28892
Closed

Re-enable non-strict comparisons on assert module #28780

bruno-brant opened this issue Jul 20, 2019 · 3 comments · Fixed by #28892

Comments

@bruno-brant
Copy link

A js quirk that can bite you is the fact that zeroes can be negative signed. Signs are ignored in this case when comparing numbers by equality, so -0 == 0, and even -0 === 0. However, -0 isn't 0, so Object.is returns false when comparing those values.

The thing is that the strictEqual function inside the assert module has a very special treatment for zeroes, where it prefers to call Object.is:

function innerDeepEqual(val1, val2, strict, memos) {
  // All identical values are equivalent, as determined by ===.
  if (val1 === val2) {
    if (val1 !== 0)
      return true;
    return strict ? Object.is(val1, val2) : true;
  }

Since the legacy (non-strict) mode is deprecated, one can no longer easily assert that a statement will have a result of 0 without knowing whether or not that statement will result in a negative zero.

So I suggest to either remove the special treatment for negative zeroes (which might be a breaking change) or enable legacy mode.

The current work around is moving from

assert.equal(fn(), 0)

to

assert(fn() === 0)

which results in worse error messages.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 20, 2019

@bruno-brant realistically, the legacy assert module can never be removed (or even runtime deprecated) as it would be too disruptive to the ecosystem. There is also nothing fundamentally wrong with the legacy module as long as you understand the difference between strict and loose comparisons. I'd suggest using the legacy module if it best fits your need.

@Trott
Copy link
Member

Trott commented Jul 20, 2019

I'd suggest using the legacy module if it best fits your need.

I would go further and say that using strict mode is unusual. Most people use legacy since it's the default.

@bruno-brant Is the code base where this came up a publicly available one? I'd be interested to see how this kind of thing comes up in the real world. I'm interested in obstacles people face that keep them using loose equalities. I'm especially interested if this only came up with the simple equality functions or if you had it come up with one of the deep-equal functions too.

@bruno-brant
Copy link
Author

@bruno-brant Is the code base where this came up a publicly available one?

I can actually upload it to a public repo, but the reason I'm using it is actually to run tests. Instead of installing chai or any other assertion library, I'm calling assert.strictEqual and assert.deepStrictEqual directly. I don't think I would actually do it in production code, although, I'm wondering if I actually need chai.

@cjihrig cjihrig mentioned this issue Jul 29, 2019
3 tasks
cjihrig added a commit to cjihrig/node that referenced this issue Jul 31, 2019
Using the legacy assert module is not discouraged. Revoke
DEP0089 to avoid user confusion.

PR-URL: nodejs#28892
Fixes: nodejs#28780
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this issue Aug 2, 2019
Using the legacy assert module is not discouraged. Revoke
DEP0089 to avoid user confusion.

PR-URL: #28892
Fixes: #28780
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants