Skip to content

Conversation

@yosuke-furukawa
Copy link
Member

solve #18227

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

@nodejs-github-botnodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jan 18, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Jan 18, 2018

I thought about this before and I feel like the current way is the more correct one.

The comparison always relies on what is possible to detect. Since Weap(Map|Set) entries do not provide a way to compare them, they should just be ignored.

This PR will also make it impossible to distinguish differences like these:

consta=newWeakMap();constb=newWeakMap();constc=newWeakMap();c.isClearlyDifferent=true;util.isDeepStrictEqual(a,b);// => false even though they are indeed equalutil.isDeepStrictEqual(a,c);// => false without distinguishing this from the former check

Because of those reasons I am -1 on landing this.

@BridgeARBridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 18, 2018
@jasnell
Copy link
Member

I wonder if it wouldn't be better to throw when one of the arguments is a WeakMap/WeakSet specifically because of the limitations here.

@BridgeAR
Copy link
Member

@jasnell hm... I am not certain if it is a good default behavior. But there is again no definite right or wrong.

@yosuke-furukawa
Copy link
MemberAuthor

@jasnell my ideal is so, they are not comparable then throw an Error. First I feel JavaScript engineers do not like Error so much. so I implemented this behavior, but some core members thinks throwing Error is better, I will change this PR.

Copy link
Member

@TimothyGuTimothyGu left a comment

Choose a reason for hiding this comment

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

As stated in #18248 (comment), I don't see why WeakMap and WeakSet should be treated differently from regular objects. Thus, -1 on this PR.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
Right now it is not documentated that WeakMap entries are not compared. This might result in some confusion. This adds a note about the behavior in `assert.deepStrictEqual`. This documentation is also references in `util.isDeepStrictEqual`, so we do not have to document it again for that function as the underlying algorithm is the same. PR-URL: nodejs#18248Fixes: nodejs#18228 Refs: nodejs#18228 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
Right now it is not documentated that WeakMap entries are not compared. This might result in some confusion. This adds a note about the behavior in `assert.deepStrictEqual`. This documentation is also references in `util.isDeepStrictEqual`, so we do not have to document it again for that function as the underlying algorithm is the same. Backport-PR-URL: #19230 PR-URL: #18248Fixes: #18228 Refs: #18228 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Right now it is not documentated that WeakMap entries are not compared. This might result in some confusion. This adds a note about the behavior in `assert.deepStrictEqual`. This documentation is also references in `util.isDeepStrictEqual`, so we do not have to document it again for that function as the underlying algorithm is the same. Backport-PR-URL: #19230 PR-URL: #18248Fixes: #18228 Refs: #18228 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Right now it is not documentated that WeakMap entries are not compared. This might result in some confusion. This adds a note about the behavior in `assert.deepStrictEqual`. This documentation is also references in `util.isDeepStrictEqual`, so we do not have to document it again for that function as the underlying algorithm is the same. PR-URL: nodejs#18248Fixes: nodejs#18228 Refs: nodejs#18228 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-majorPRs that contain breaking changes and should be released in the next major version.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@yosuke-furukawa@BridgeAR@jasnell@TimothyGu@nodejs-github-bot