Tape: Support for comparing ES2015 Maps and Sets

[js-choi]

As noted in substack/node-deep-equal#28, Node’s deepEqual algorithm does not support comparing ES2015/ES6 Maps and Sets, and, as noted in nodejs/node#2309 and nodejs/node#2315, there is no intent by Node’s staff to support them. According to @domenic and @Trott, the Node standard assert module is intended for only testing Node itself, and assertions in user land should be addressed by user-land assertion libraries. For this same reason (mjackson/expect#47), along with circular references (mjackson/expect#50), the expect library has switched from node-deep-equal to is-equal (mjackson/expect@32e2169d238d479289145aa692d8e44f1a6316bc).

Given all this, it would make sense for Tape to add another recursive comparison function – alternative to t.deepEqual, t.deepLooseEqual, etc. – that would support Maps and Sets. This function too might use is-equal, which also supports comparing Dates and RegExps, and which might be called something like t.jsEquiv, since it would test whether two objects represent the same JavaScript-standard-library data.

Alternatively, it may be simpler and more generally useful to support Maps and Sets by simply comparing their iterators’ items. Shallow and deep versions might be called t.iterEquiv and t.deepIterEquiv (alternative names include t.itemsEqual, t.iterEqual, and so on). For instance, t.deepIterEquiv(new Map([ [ 3, 2 ], [ 5, 4 ] ]), [ [ 3, 2 ], [ 5, 4 ] ] would test as true, because the map and the array’s iterators both yield the same entries in the same order, and their entries’ iterators also yield the same integers in the same order.

In this case, t.iterEquiv and t.deepIterEquiv might also allow additional optional arguments that let the user customize what predicates are used to compare items. t.iterEquiv would take one optional predicate, for comparing items shallowly. t.deepIterEquiv would take two optional predicates: one for comparing leaf-node items and the other for comparing branch-node items without regard to the branches’ children.

These two options for comparing Maps and Sets – t.jsEquiv versus t.iterEquiv / t.deepIterEquiv – are not mutually exclusive, of course. Both are useful, and neither should also be difficult to implement; I might be open to doing the work myself. The question here is more about whether @substack thinks either or both of them would fit with his vision for Tape.

[ljharb]

I think these things could indeed be useful, but the goal of tape is generally to mirror the small API of assert. I'll wait to see what other collabs think.

In the meantime, something like this should work just fine:

var test = require('tape');
var isEqual = require('is-equal');

test('something', function (t) {
  t.ok(isEqual(a, b), 'a and b are equal');
  t.end();
});

[Trott]

there is no intent by Node’s staff to support them.

Unfortunately, you are correct about the current official position of the project.

However, as a potential ray of hope: My view on assert has evolved since https://github.com/nodejs/node/issues/2309. https://github.com/nodejs/node/pull/10282 and one or two other current PRs may force the issue a bit. (See my only-mildly-incoherent comment at https://github.com/nodejs/node/pull/10282#issuecomment-268721839.)

But yeah, there's no guarantees on any of that stuff and a change in the situation may take a while.

On the upside, the Node.js Core Technical Committee is scheduled to discuss this issue at this week's meeting. See https://github.com/nodejs/CTC/issues/63 for info if interested.

[js-choi]

@Trott Thanks; good to know. It’ll be slow but interesting for the whole Node ecosystem to see what ends up happening.

@ljharb In lieu of a change, it may be most efficient to put a note in the readme and whatever other documentation, saying that the intent of the tape API is to mirror node's assert, and that users should generally use other libraries such as is-equal or write their own comparator functions if they wish to compare items deeply. It would be usefully clarifying and might save the time of new users.

[ljharb]

@js-choi thanks, that's a great idea. PRs are appreciated, especially for docs improvements.

[Trott]

At the last CTC meeting, it was agreed that the Locked API status for assert should not be used as a reason to decline to fix bugs. We also agreed to remove the doc material that says assert is intended for Node.js internal use only.

Based on the conversation, I believe we would now accept a PR to (for example) fix deepEqual() to work as expected with Maps and Sets. In fact, I believe there already may be a PR to do that (but I could be wrong about that).

The API is still Locked, which means no new features. But adjusting the API to work with Maps, Sets, Symbols, etc. should be allowable. (There's a chance people will argue that such changes should only be released in a major version, which goes out every six months. We'll see.)

Hope this helps.

[ljharb]

@Trott awesome - if you could link me to that PR, i'd be very interested in ensuring it behaves properly.

[Trott]

@ljharb Found the PR. It's from 1.5 years ago and is closed. https://github.com/nodejs/node/pull/2315 I'll update my comment above to strike out the part where I say that I think there's already a PR open for it.

[Trott]

@ljharb Other open PRs on assert that might be worth your time reviewing:

• assert: enforce type check in deepStrictEqual
• assert: fix _deepEqual and improve assert.md

[substack]

Once this is merged into core I will release a new major version of deep-equal based on the new algorithm that tape can be updated to support.

[Sawtaytoes]

As listed in https://github.com/substack/node-deep-equal/issues/54, it looks like support was added to Node.js in v8. Is there an ETA for this change coming to tape?

[kumavis]

+1 to this

[lfurzewaddock]

Borrow solution from node-tap? https://github.com/tapjs/node-tap/issues/372

[ljharb]

@lfurzewaddock that doesn't match node's assert module, and is both not robust and has a bug.

[lfurzewaddock]

Thanks @ljharb, I can confirm t.deepEqual for an object containing Maps with cyclical references worked successfully.

[ljharb]

This should be resolved in the v5.0.0-next.0 prerelease of tape, via deep-equal v2.