Repository navigation
util.isDeepStrictEqual would be better to return false to compare WeakMap/WeakSet #18227
Description
-
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.
Activity
BridgeAR commentedon 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:
yosuke-furukawa commentedon 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
trueis very confusing.falseis 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 commentedon 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
trueis 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 commentedon Jan 18, 2018
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 commentedon Jan 21, 2018
Without reading the discussion (so I stay with intuition) - I'd expect
deepEqualto return true if and only if the twoWeakMaporWeakSets are to the same reference.BridgeAR commentedon 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 commentedon Feb 7, 2018
/cc @nodejs/tsc
ofrobots commentedon Feb 7, 2018
I'm +1 to what @benjamingr proposed above.
mcollina commentedon Feb 8, 2018
I am 👍 to:
return trueif the two objects are the same instance andreturn falseotherwiseWe should also document the current behvior in our current/lts release lines.
cjihrig commentedon Feb 8, 2018
I agree with @mcollina.
wza62692371 commentedon Mar 28, 2020
@https://github.com/Trott
BridgeAR commentedon Jun 18, 2024
After looking into this again, I changed my mind / I agree with the suggested behavior change.
yosuke-furukawa commentedon Jun 18, 2024
wow. I am surprised this issue is still open.
#53495 this PR pushed this discussion. I don't change my mind. if the PR is merged, this issue will be closed.
benjamingr commentedon Jun 28, 2024
The PR landed