-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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 support for Map and Set in deepEqual #12142
Changes from 1 commit
561561a
f051840
1d6cda6
800ae46
031f6f3
d6baaee
8fb6ebf
acef701
ee131e8
7bc29b0
6bdfcaf
fc5196a
7f9d4d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -161,6 +161,14 @@ function isArguments(tag) { | |
return tag === '[object Arguments]'; | ||
} | ||
|
||
function isMap(object) { | ||
return object.constructor === Map; | ||
} | ||
|
||
function isSet(object) { | ||
return object.constructor === Set; | ||
} | ||
|
||
function _deepEqual(actual, expected, strict, memos) { | ||
// All identical values are equivalent, as determined by ===. | ||
if (actual === expected) { | ||
|
@@ -262,11 +270,18 @@ function _deepEqual(actual, expected, strict, memos) { | |
} | ||
} | ||
|
||
// For all other Object pairs, including Array objects, | ||
if (isSet(actual)) { | ||
return isSet(expected) && setEquiv(actual, expected); | ||
} else if (isSet(expected)) { | ||
return false; | ||
} | ||
|
||
// For all other Object pairs, including Array objects and Maps, | ||
// equivalence is determined by having: | ||
// a) The same number of owned enumerable properties | ||
// b) The same set of keys/indexes (although not necessarily the same order) | ||
// c) Equivalent values for every corresponding key/index | ||
// d) For Maps, strict-equal keys mapping to deep-equal values | ||
// Note: this accounts for both named and indexed properties on Arrays. | ||
|
||
// Use memos to handle cycles. | ||
|
@@ -283,6 +298,26 @@ function _deepEqual(actual, expected, strict, memos) { | |
return objEquiv(actual, expected, strict, memos); | ||
} | ||
|
||
function setEquiv(a, b) { | ||
// This behaviour will work for any sets with contents that have | ||
// strict-equality. That is, it will return false if the two sets contain | ||
// equivalent objects that aren't reference-equal. We could support that, but | ||
// it would require scanning each pairwise set of not strict-equal items, | ||
// which is O(n^2), and would get exponentially worse if sets are nested. So | ||
// for now we simply won't support deep equality checking set items or map | ||
// keys. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I would actually prefer the awful performance over always using strict equality… @nodejs/collaborators thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, I think I'd value correct over performant, so yeah, I agree with @addaleax. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For tests, I agree. I'm nervous people might be using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... Another strong argument in favor of making it correct > fast is that it'll be more compatible with the current implementation. If someone currently has a test that reads There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @josephg Maybe just update this PR and see if somebody objects? If we need to go back, the current HEAD is at f051840. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #14258 just landed and reduces the complexity to O(n log n). Therefore the performance should not be of much concern anymore. |
||
if (a.size !== b.size) | ||
return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I think |
||
|
||
var val; | ||
for (val of a) { | ||
if (!b.has(val)) | ||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
function objEquiv(a, b, strict, actualVisitedObjects) { | ||
// If one of them is a primitive, the other must be the same. | ||
if (util.isPrimitive(a) || util.isPrimitive(b)) | ||
|
@@ -307,6 +342,25 @@ function objEquiv(a, b, strict, actualVisitedObjects) { | |
return false; | ||
} | ||
|
||
if (isMap(a)) { | ||
if (!isMap(b)) | ||
return false; | ||
|
||
if (a.size !== b.size) | ||
return false; | ||
|
||
var item; | ||
for ([key, item] of a) { | ||
if (!b.has(key)) | ||
return false; | ||
|
||
if (!_deepEqual(item, b.get(key), strict, actualVisitedObjects)) | ||
return false; | ||
} | ||
} else if (isMap(b)) { | ||
return false; | ||
} | ||
|
||
// The pair must have equivalent values for every corresponding key. | ||
// Possibly expensive deep test: | ||
for (i = aKeys.length - 1; i >= 0; i--) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use
isMap
andisSet
fromprocess.binding('util')
instead? These checks won’t work for subclasses of Map/Set or Maps/Sets from other VM contexts.