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

assert: add checkPrototype option for assert.deepStrictEqual() #29334

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 27, 2019

Allow usage of assert.deepStrictEqual() without prototype checks.

Refs: #28011

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

Allow usage of assert.deepStrictEqual() without prototype checks.

Refs: nodejs#28011
@Trott Trott added assert Issues and PRs related to the assert subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 27, 2019
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Aug 27, 2019
@Trott
Copy link
Member Author

Trott commented Aug 27, 2019

Seemed like a pretty good idea when we talked about it in #28011 but now that it's implemented, I wonder if we're trying to solve the wrong problem. People already have a way to do this: assert.deepEqual(). This doesn't really provide a smooth onramp to moving to assert.deepStrictEqual() since it won't work in older versions of Node.js. If people want strict comparisons, they can use Object.create() or Object.setPrototypeOf() or another mechanism to duplicate prototypes to their test results.

@Trott
Copy link
Member Author

Trott commented Aug 29, 2019

This doesn't really provide a smooth onramp to moving to assert.deepStrictEqual() since it won't work in older versions of Node.js.

Maybe a smooth-enough onramp might be to set .strict mode to cause deepEqual() to be the same as deepStrictEqual() but with this option set to false. That would be a breaking change so would land in 13.x at the earliest. This would allow packages like esprima to use .strict mode without needing to modify the tests (beyond specifying .strict). Packages that only care about 13.x and newer could use .strict. Packages that need to support 12.x can do that and add this option explicitly. If we backport this to 10.x, people can use the option there too. For earlier versions, people can guard with assert = assert.strict || assert to keep current behavior in 8.x and earlier. The option in 8.x and earlier will result in [object Object] messages when AssertionErrors occur, unless additional precautions are taken. But that seems manageable or even ignorable in many cases as 8.x will be EOL in four months.

Interested in @BridgeAR's thoughts on the above.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

Trott commented Aug 29, 2019

Maybe a smooth-enough onramp might be to set .strict mode to cause deepEqual() to be the same as deepStrictEqual() but with this option set to false.

Although I guess that could be a foot-gun for current .strict users and also a surprise for new adopters.

@Trott
Copy link
Member Author

Trott commented Aug 29, 2019

@nodejs/assert

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I would rather not add this option. Instead, I would favor rewriting assert to a class where it's possible to set options. One of those could be to deactivate the prototype check.
We just have to make sure to keep the assert module backwards compatible.

@Trott Trott closed this Sep 4, 2019
@Trott Trott deleted the skip-prototype branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. 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.

4 participants