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 would be better to return false to compare WeakMap/WeakSet #18227

Open
yosuke-furukawa opened this issue Jan 18, 2018 · 10 comments

Comments

Projects
None yet
8 participants
@yosuke-furukawa
Copy link
Member

commented Jan 18, 2018

  • Version:
    v9.4.0

  • Platform:
    OSX, Linux, Windows

  • Subsystem:
    N/A

util.isDeepStrictEqual always returns true when WeakMap/WeakSet comparison.

const util = require('util');
const wm1 = new WeakMap();
const wm2 = new WeakMap();
const key = {};
wm1.set(key, 1);
wm2.set(key, 2);
console.log(util.isDeepStrictEqual(wm1, wm2)); // true

this is because WeakMap|WeakSet.prototype.valueOf always returns empty object.
I know WeakMap / WeakSet do not have an API to show the full content, and they depends on GC so those collection comparison is very difficult.

However, this behavior (always return true) seems to be curious for me. I think it would be better to always return false.

@BridgeAR

This comment has been minimized.

Copy link
Member

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.

The suggested change 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
@yosuke-furukawa

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

hm, but I think WeakMap / WeakSet are not suitable to compare the equivalence, so we would be better to return false than true.

Any WeakMap and WeakSet comparison always return true is very confusing. false is also confusing but it has the reason (WeakMap and WeakSet is not suitable to compare).

Anyway, we would be better to add a note on documentation.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jan 18, 2018

@yosuke-furukawa I am definitely +1 for updating the docs! That should also apply to assert.deepStrictEqual.

About being right or wrong: I think there is no definite right approach. Only one way we want to decide on. So far it was the one in place right now (maybe by chance) and I personally feel it is the better one.

I feel returning true is not so confusing anymore after checking the documentation how weak entries are saved and after thinking about how it is possible to compare things in JS.

@yosuke-furukawa

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

+1 for updating the docs

Thank you, I will send another PR.

@nodejs/collaborators
I would like to hear other members' opinion.
My opinion is "WeakMap/Set are not comparable, so util.isDeepStrictEqual would be better to return false than return true".

@benjamingr

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

Without reading the discussion (so I stay with intuition) - I'd expect deepEqual to return true if and only if the two WeakMap or WeakSets are to the same reference.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

I ask everyone to continue discussing this in #18248. Thanks :-)

@benjamingr that would be a special handling of those and not be aligned with any other object.

@addaleax

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

@ofrobots

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

I'm +1 to what @benjamingr proposed above.

@mcollina

This comment has been minimized.

Copy link
Member

commented Feb 8, 2018

I am 👍 to:

  1. return true if the two objects are the same instance and return false otherwise
  2. throw consistently because two Weak* are not really comparable

We should also document the current behvior in our current/lts release lines.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2018

I agree with @mcollina.

BridgeAR added a commit to BridgeAR/node that referenced this issue Mar 9, 2018

util,assert: improve Weak(Map|Set) support
This improves support for Weak(Map|Set) in `assert.deepStrictEqual`,
`assert.notDeepStrictEqual` and `util.isDeepStrictEqual`.

Now, the entries of a WeakMap / WeakSet will also be compared instead
of being ignored as before.

Fixes #nodejs#18227

BridgeAR added a commit to BridgeAR/node that referenced this issue Mar 9, 2018

util,assert: improve Weak(Map|Set) support
This improves support for Weak(Map|Set) in `assert.deepStrictEqual`,
`assert.notDeepStrictEqual` and `util.isDeepStrictEqual`.

Now, the entries of a WeakMap / WeakSet will also be compared instead
of being ignored as before.

Fixes: nodejs#18227

@BridgeAR BridgeAR referenced this issue Mar 9, 2018

Closed

util: improve Weak(Map|Set) support #19259

4 of 4 tasks complete

@Trott Trott removed the tsc-review label Mar 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.