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

comparing empty array vs empty object with deepEqual #186

Closed
tcoopman opened this issue Aug 30, 2015 · 11 comments
Closed

comparing empty array vs empty object with deepEqual #186

tcoopman opened this issue Aug 30, 2015 · 11 comments
Labels

Comments

@tcoopman
Copy link

doing t.deepEqual({}, []) evaluates as true. I would expect it to be false.

@ljharb
Copy link
Collaborator

ljharb commented Aug 31, 2015

assert.deepEqual({}, []) also passes - this is admittedly odd, since the "length" property isn't on the object, but all of their enumerable own properties are indeed equal - consider assert.deepEqual({0:1}, [1]). If you want to compare types as well, you'd want a combination of Array.isArray, typeof, etc.

@xeodou
Copy link

xeodou commented Nov 16, 2015

@ljharb Thanks for your work on this project.
Can i ask how is going on for this ticket, it's kind of wired for t.deepEqual({}, []),even they are not the same type in meaning. I know they are both object but in other words they totally different.

@ljharb
Copy link
Collaborator

ljharb commented Nov 16, 2015

deepEqual isn't meant to be a strict comparison. Since assert.deepEqual({}, []) passes as well, I think it's best for tape to stay in line with assert's API.

@xeodou
Copy link

xeodou commented Nov 16, 2015

@ljharb Thanks. That's helpful.

@jmm
Copy link
Contributor

jmm commented Feb 22, 2016

@ljharb

deepEqual isn't meant to be a strict comparison. Since assert.deepEqual({}, []) passes as well, I think it's best for tape to stay in line with assert's API.

It's worth noting that t.deepEqual() already deviates from assert.deepEqual()'s API. Example:

var assert = require("assert");
var test = require("tape");

var a = [1];
var b = ["1"];

test(function (t) {
  assert.deepEqual(a, b, "not assert.deepEqual");
  t.deepEqual(a, b, "not t.deepEqual");
});
// not ok 1 not t.deepEqual

For the other readers it's also notable that the deep equal logic does not actually live in this package, FWIW.

Also, Node has assert.deepStrictEqual() that would throw here, and at least one implementation on npm: deep-strict-equal. (I presume it works in the browser?) However, even that wouldn't reject sparse arrays that only differ by length, example:

var a = [1];
var b = [1];
b.length = 2;

^^ In reference to:

this is admittedly odd, since the "length" property isn't on the object

@jmm
Copy link
Contributor

jmm commented Feb 22, 2016

Oh yeah, forgot to mention why I wound up here in the first place: there are other similar cases, such as t.deepEqual({}, function () {}). Again, assert.deepStrictEqual() rejects that (because it compares prototype equality). EDIT: nope, my mistake -- I had something screwed up.

@ljharb
Copy link
Collaborator

ljharb commented Mar 6, 2016

It sounds like the only issue here is that tape's deepEqual method works like assert.deepStrictEqual, but not the same as assert.deepEqual?

If that is the only deviation from assert, then the remaining question is "should we add deepStrictEqual to tape and make deepEqual loose? Or, should leave it as-is?

@ljharb ljharb added the question label Mar 6, 2016
@BridgeAR
Copy link

BridgeAR commented Aug 19, 2017

@ljharb the deep-equal package is pretty much a copy of a very old Node.js deepEqual version but it was not updated for a very long time now. Quite a few things were improved in the original version since then.

@substack would you mind updating that? I would also be willing to open a PR for it if someone looks into it.

@gitowiec

This comment has been minimized.

@ljharb

This comment has been minimized.

ljharb added a commit to inspect-js/node-deep-equal that referenced this issue Jul 31, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this issue Jul 31, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this issue Jul 31, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this issue Aug 3, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this issue Aug 7, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this issue Aug 9, 2019
ljharb added a commit to inspect-js/node-deep-equal that referenced this issue Aug 9, 2019
@ljharb
Copy link
Collaborator

ljharb commented Dec 27, 2019

With v5, and deep-equal v2, tape now matches node's assert.

@ljharb ljharb closed this as completed Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants