Sets/Maps silently pass

[keithamus]

As @hildjj and @rocketraman point out in chaijs/chai#394, deep equality for Maps and Sets doesn't work (because their keys are not exposed like a normal object, which is what we check for).

x = new Map
x.set('foo', 'bar');
y = new Map
y.set('bar', 'baz');
chai.expect(y).to.deep.equal(x) // this passes, but should fail.

There's also not any particularly convenient ways to get all of the values out in a succinct way, except maybe the spread operator, but this is only available if transpiling ES6.

chai.expect([...y]).to.deep.equal([...x])

With Maps and Sets now a part of Node v4.0, deep-eql should definitely support Maps and Sets.

To implement this, we'd just need to add some extra conditions within around L56 to detect Maps and Sets (and WeakMaps and WeakSets), and call out to functions such as mapEqual and setEqual.

When done, we should PR upstream to chai proper.

[keithamus]

Because Maps & Sets' keys aren't enumerable, one would need to specifically type detect if they are a Map or Set. To do this, you'd need to use type(a) === 'map' || type(a) === 'set' || type(a) === 'weakmap' || type(a) === 'weakset' || type(b) === 'map' || type(b) === 'set' || type(b) === 'weakmap' || type(b) === 'weakset' (type is a function already available) around L51, similar to how arguments is detected (L49-50). If you can do this in a cleaner way, great 😄

You'd then need to create a mapSetEqual function which compares them (see dateEqual for an example signature). mapSetEquals algo should probably go something like this:

// if the prototypes dont match, these wont be deep equal. return false early
if (a.prototype !== b.prototype) {
  return false;
}
ka = Array.from(a.keys()).sort(); // get the sorted keys for Map a
kb = Array.from(b.keys()).sort(); // get the sorted keys for Map b

if (iterableEqual(ka, kb) === false) { // iterableEqual already exists
  return false;
}

// iterate over the first maps keys
for (i = ka.length - 1; i >= 0; i--) {
  key = ka[i];
  // do a deep equality check on a and b's values of this key
  if (!deepEqual(a.get(key), b.get(key), m)) {
    return false;
  }
}

Tests would also need to be added to the test file for a complete PR.

[lucasfcosta]

Hi @keithamus, I will start working on this so we can solve chaijs/chai#394 next.

But I think we have to define what will be the behavior when it comes to WeakMaps and WeakSets. These data-structures are not iterable and we cannot get all elements from them.

This is what MDN says:

Because of references being weak, WeakMap/WeakSet keys are not enumerable (i.e. there is no method giving you a list of the keys). If they were, the list would depend on the state of garbage collection, introducing non-determinism. If you want to have a list of keys, you should maintain it yourself.

Are we going to use strict (===) comparison for these types?
What are your thoughts on this?

[keithamus]

@lucasfcosta hold off on this - sorry. I've actually got a big refactor of deep-eql which should also solve this issue. I'll probably be submitting a PR in about 24 hours.

[lucasfcosta]

Ah, okay, no problem.
I hope I'm not bothering you with all those comments.

Let me know if you need any help with it 😃

[keithamus]

Absolutely not bothering me, it's awesome that you're so keen to work on this all! I'm just bad at being organised at assigning myself to tickets.

If you are looking for stuff to work on, a couple of tickets you could definitely close down are:

chaijs/chai#407 - Add Proxy support to interfaces, so that unknown property access throws an error.
chaijs/chai#281 - Make ownProperty assertion more like property - change the context (flag(this, 'obj')) just like property does, and add a value second argument.

I will finish up with type-detect and deep-eql this weekend, hopefully get some time to make a same-error lib which will close a bunch of issues - and at some point in the week we could have a chat over Slack or Gitter or IRC about the roadmap, and what the rest of the plans are. Sound good?

[lucasfcosta]

Sounds awesome!
I'll definitely take a look at these issues.
Thanks!

[pigulla]

I see this issue is closed, but

expect(new Set(['a'])).to.deep.equal(new Set(['b']))

still passes in the current version of Chai :(

[keithamus]

@pigulla this issue is still open. We're working on it 😄

[pigulla]

Duh. Can't read. My apologies.

[keithamus]

@pigulla deep-eql 1.0 has been released, which fixes this, and will be used in chai 4.0 which is being released soon. Thanks for the issue!