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

Fixes equality checking for Map objects. #52

Merged
merged 2 commits into from
Dec 1, 2019

Conversation

C-Saunders
Copy link

@C-Saunders C-Saunders commented Sep 20, 2017

Currently:

// keys are not checked
const equal = require('./index.js')
const a = new Map([['a', 1]])
const b = new Map([['b', 1]])
equal(a, b) // true
// recursive checking for values doesn't work, either
const x = new Map([['a', [1, 2]]])
const y = new Map([['a', [2, 1]]])
equal(x, y) // true
// empty maps equal those with data b/c the key set is empty and the check falls all the way though the ifs
const z = new Map()
equal(a, z) // true

While Maps don't have good browser support at this time, they are useful in Node, and I've found that I want tape's deepEqual, deepLooseEqual, and friends to work properly with Maps when testing Node applications.

The implementation is a near duplication of the Object handling code, but deduplicating that didn't seem straightforward or all that beneficial to me at the time.

I also made a few small changes to make the style more consistent, and added a .gitignore, but tried to not overreach or get too out of scope. I can revert these changes upon request.

@C-Saunders
Copy link
Author

Updated description to include more details of the issue this addresses.

Updated the Travis config so I could get a nice green checkmark.

@Tyler-Murphy
Copy link

I'd also like to see this feature. Can anyone review it?

index.js Outdated
@@ -122,6 +127,24 @@ function objEquiv(a, b, opts) {
}
}

// Maps
if (typeof Map && a.constructor === Map && b.constructor === Map) {
Copy link
Member

Choose a reason for hiding this comment

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

This check is not robust across realms; it will need a brand check.

index.js Outdated
@@ -122,6 +127,24 @@ function objEquiv(a, b, opts) {
}
}

// Maps
if (typeof Map && a.constructor === Map && b.constructor === Map) {
ka = Array.from(a.keys());
Copy link
Member

Choose a reason for hiding this comment

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

Array.from is not necessarily available; we'll need to use a package or manually implement a while loop to call .next() on the iterator. However, not all Map implementations have keys either, so it's actually a pretty hard problem to robustly iterate Maps and Sets.

@ljharb ljharb force-pushed the master branch 2 times, most recently from 191fb2f to ecd15ae Compare December 1, 2019 06:47
@ljharb ljharb force-pushed the map_value_equality branch 2 times, most recently from 2a4c42d to 47eb288 Compare December 1, 2019 08:47
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

updated this with a robust solution

@ljharb ljharb merged commit 20877c4 into inspect-js:master Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants