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

util,assert: expose util.isDeepStrictEqual() #16084

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 8, 2017

Provide util.isDeepStrictEqual() that works like assert.deepStrictEqual() but returns a bollean rather than throwing an error.

Several userland modules have needed this functionality and implemented
independently. This functionality already exists in Node.js core, so
this exposes it for use by modules. Modules that have needed this
functionality include lodash, concordance (used by ava), and
qunit.

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
Affected core subsystem(s)

util

@Trott Trott added notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module. labels Oct 8, 2017
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 8, 2017
@Trott
Copy link
Member Author

Trott commented Oct 8, 2017

ping @jdalton @BridgeAR (Ruben, I think you were the one who mentioned wanting util.isEqual() today. If I'm wrong about that, sorry for the noise!)

@Trott
Copy link
Member Author

Trott commented Oct 8, 2017

For reference, here are things similar to this functionality (that already exists in Node.js core but is not currently exposed without throwing assertion errors) being reimplemented in userland multiple times. I'm sure there are more examples

(edited by @vsemozhetbyt: fix formatting)

@refack
Copy link
Contributor

refack commented Oct 8, 2017

And so I would rather see only util.isEqual() (with util.deepStrictEqual() semantics) since we don't really support the non-strict version.

@Trott
Copy link
Member Author

Trott commented Oct 9, 2017

@refack Can you explain a bit more about us not supporting the non-strict version? I know we (mostly) don't use it in core and we even have a lint rule to flag it. Is that what you mean by not supporting it?

@BridgeAR
Copy link
Member

BridgeAR commented Oct 9, 2017

Yes, I planned on implementing this on my own as well, so I am definitely +1 on adding this in general. I strongly oppose adding the losse variation though. It was a bad idea to ever implement this and I recommend to only have util.isEqual as @refack also pointed out. The loose equality is more like a "this might be somewhat similar" than anything else.

We do support the loose version but we should not. And even if we want to add support for that, I would rather add a optional options object to the function instead of using a separate function name.

I am not completely sure if it makes sense to pretty much copy all the assert tests though. It would probably be enough to move them and only test that the assert version actually throws. It would reduce the amount of tests needed.

I would also like the documentation to be more expressive but we can also improve that after the feature itself has landed.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 9, 2017

I actually worked a lot on assert before because of adding exactly this. This is a long needed feature for Node and many libraries will likely benefit by this.

@refack
Copy link
Contributor

refack commented Oct 9, 2017

Can you explain a bit more about us not supporting the non-strict version? I know we (mostly) don't use it in core and we even have a lint rule to flag it. Is that what you mean by not supporting it?

We do support the loose version but we should not. And even if we want to add support for that, I would rather add a optional options object to the function instead of using a separate function name.

As @BridgeAR there has been minimal work done to improve the non-strict variant. So IMHO it's frozen as-is and not actively supported. If the language comes up with new primitives, I'm not sure how quickly we'll implement their comparison in the non-strict variant.

Just an example:

> assert.deepStrictEqual(new Number(1), new Number(2));
AssertionError [ERR_ASSERTION]: [Number: 1] deepStrictEqual [Number: 2]
    at repl:1:8
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:239:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:440:10)
    at emitOne (events.js:120:20)
    at REPLServer.emit (events.js:210:7)
    at REPLServer.Interface._onLine (readline.js:282:10)
    at REPLServer.Interface._line (readline.js:631:8)
> assert.deepEqual(new Number(1), new Number(2));
undefined

or

> assert.deepStrictEqual(NaN, NaN);
undefined
> assert.deepEqual(NaN, NaN);
AssertionError [ERR_ASSERTION]: NaN deepEqual NaN
    at repl:1:8
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:242:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:466:10)
    at emitOne (events.js:120:20)
    at REPLServer.emit (events.js:210:7)
    at REPLServer.Interface._onLine (readline.js:287:10)
    at REPLServer.Interface._line (readline.js:642:8)

@jdalton
Copy link
Member

jdalton commented Oct 9, 2017

I'm cool with just exposing the deepStrictEqual form, though I think if deepEqual is to be treated as deprecated then it should be officially deprecated.

@Fishrock123
Copy link
Contributor

I'm also a little uncertain as to what use exposing a deepEqual helper is, but I would definitely like a deepStrictEqual helper.

@Trott
Copy link
Member Author

Trott commented Oct 9, 2017

No argument from me on not exposing the deepEqual variant. Next bit of bikeshedding, though:

Should it be util.isDeepStrictEqual() to make it clear that it matches assert.deepStrictEqual()?

Or should it be util.isEqual() which is confusing (shouldn't that match what assert.equal() does?) but doesn't leave the user wondering why there's an isDeepStrictEqual() but none of the obvious corresponding variants isDeepEqual(), isStrictEqual(), and isEqual()?

Or should it be both? That is, util.isDeepStrictEqual() and util.isEqual() are the same thing? I can see some upside but it also is confusing. I would not expect isDeepStrictEqual() and isEqual() to do the same thing.

I'm not super-excited about any of these options, but we should be careful about which one we pick because we will probably be stuck with it forever once it's out there.

@jdalton
Copy link
Member

jdalton commented Oct 9, 2017

🚲 🏠:

I'm a big fan of consistency and not a fan of aliases so I'd dig
util.isDeepStrictEqual() with no aliases.

@Fishrock123
Copy link
Contributor

util.isDeepStrictEqual() seems fine to me

@Trott
Copy link
Member Author

Trott commented Oct 11, 2017

util.isDeepStrictEqual() it is. Removed util.isDeepEqual(). PTAL.

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

@Trott
Copy link
Member Author

Trott commented Oct 11, 2017

Argh! Left out the test file before. One more time...

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

@Trott Trott changed the title util,assert: expose util.isDeep[Strict]Equal() util,assert: expose util.isDeepStrictEqual() Oct 11, 2017
@Trott
Copy link
Member Author

Trott commented Oct 12, 2017

CI passed. This has one approval. It's been open for a few days. In theory it can land. I'd be more comfortable with a bit more review, though. I'll start by pinging the people listed for lib/util in the onboarding-extras doc: @bnoordhuis, @cjihrig, @evanlucas

Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is happy. I've definitely needed this before :]

@BridgeAR
Copy link
Member

I would prefer not to use the name strict. Using a bad name because of historic reasons for something independent and new is not the right way out of my perspective. I would actually just use the name util.isEqual or if that is not clear enough util.isDeepEqual. As there is only a single util function with that name, there should also not be any confusion with the assert part. It should be seen independently anyway.

@BridgeAR
Copy link
Member

Since most people seem to not have an issue with the current name, I will not block this any further.

@Trott
Copy link
Member Author

Trott commented Oct 25, 2017

It's been 12 days since the last CI run, so it seems prudent to do another one before even considering landing...

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

@mhdawson mhdawson removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Oct 25, 2017
Trott added a commit to Trott/io.js that referenced this pull request Oct 26, 2017
Provide `util.isDeepStrictEqual()` that works like
`assert.deepStrictEqual()` but returns a boolean rather than throwing an
error.

Several userland modules have needed this functionality and implemented
it independently. This functionality already exists in Node.js core, so
this exposes it for use by modules. Modules that have needed this
functionality include `lodash`, `concordance` (used by `ava`), and
`qunit`.

PR-URL: nodejs#16084
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott
Copy link
Member Author

Trott commented Oct 26, 2017

Landed in 3673208.

@Trott Trott closed this Oct 26, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
Provide `util.isDeepStrictEqual()` that works like
`assert.deepStrictEqual()` but returns a boolean rather than throwing an
error.

Several userland modules have needed this functionality and implemented
it independently. This functionality already exists in Node.js core, so
this exposes it for use by modules. Modules that have needed this
functionality include `lodash`, `concordance` (used by `ava`), and
`qunit`.

PR-URL: nodejs/node#16084
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented Oct 30, 2017

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@Trott
Copy link
Member Author

Trott commented Oct 31, 2017

I'm inclined to not backport as it depends on other semver-major commits. But I could see the case for perhaps avoiding future merge conflicts. But on balance, I'm inclined to treat this as 9.0.0-and-above-only.

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Provide `util.isDeepStrictEqual()` that works like
`assert.deepStrictEqual()` but returns a boolean rather than throwing an
error.

Several userland modules have needed this functionality and implemented
it independently. This functionality already exists in Node.js core, so
this exposes it for use by modules. Modules that have needed this
functionality include `lodash`, `concordance` (used by `ava`), and
`qunit`.

PR-URL: nodejs/node#16084
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Mar 8, 2018

I personally would like this to be backported to 8.x. I think it would be good to have this feature there as well. Backporting will not be trivial though.

@gibfahn
Copy link
Member

gibfahn commented Mar 9, 2018

I personally would like this to be backported to 8.x. I think it would be good to have this feature there as well. Backporting will not be trivial though.

If someone would like to backport then I'm happy to land it on v8.x.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 9, 2018

Ok, I am going to do that.

goto-bus-stop added a commit to goto-bus-stop/node that referenced this pull request Jun 21, 2018
When the deep(Strict)Equal comparison functions were moved to an
internal module, a variable named `current` was replaced with `val1`.
That accidentally also replaced a few "currently"s in comments.

Refs: nodejs#16084
@goto-bus-stop goto-bus-stop mentioned this pull request Jun 21, 2018
1 task
targos pushed a commit that referenced this pull request Jun 24, 2018
When the deep(Strict)Equal comparison functions were moved to an
internal module, a variable named `current` was replaced with `val1`.
That accidentally also replaced a few "currently"s in comments.

Refs: #16084

PR-URL: #21436
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Jun 24, 2018
When the deep(Strict)Equal comparison functions were moved to an
internal module, a variable named `current` was replaced with `val1`.
That accidentally also replaced a few "currently"s in comments.

Refs: #16084

PR-URL: #21436
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@Trott Trott deleted the very-deep branch January 13, 2022 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.