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: isDeepStrictEqual always returns false to compare WeakMap/WeakSet #18228

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-bot nodejs-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:

const a = new WeakMap();
const b = new WeakMap();
const c = new WeakMap();
c.isClearlyDifferent = true;

util.isDeepStrictEqual(a, b); // => false even though they are indeed equal
util.isDeepStrictEqual(a, c); // => false without distinguishing this from the former check

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

@BridgeAR BridgeAR 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

jasnell commented Jan 18, 2018

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
Member Author

@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

@TimothyGu TimothyGu 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 BridgeAR closed this in 936eea5 Feb 1, 2018
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#18248
Fixes: 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: #18248
Fixes: #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: #18248
Fixes: #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#18248
Fixes: nodejs#18228
Refs: nodejs#18228
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major 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.

5 participants