Primitive wrappers are always considered equal

[haggholm]

const x = new Number(1);
const y = new Number(1);
console.log({ x, y, equal: require("fast-equals").circularDeepEqual(x, y) });

{x: Number { 1 }, y: Number { 1 }, equal: true}

OK. But...

const x = new Number(1);
const y = new Number(2);
console.log({ x, y, equal: require("fast-equals").circularDeepEqual(x, y) });

{x: Number { 1 }, y: Number { 2 }, equal: true}

That doesn't seem right. And...

const x = new Number(1);
const y = new Boolean(false);
console.log({ x, y, equal: require("fast-equals").circularDeepEqual(x, y) });

{x: Number { 1 }, y: Boolean { false }, equal: true}

...Well, huh.

This strikes me as counterintuitive and seems incorrect. I could perhaps live with case 1 returning false, or null, or undefined: if comparing two objects of some type the library isn't prepared to handle, I wouldn't be too surprised if it refused to return a boolean result, or just couldn't 'guarantee' that they're the same. But stating that new Number(1) is equal to new Number(2) seems wrong in a surprising way.

I thought about creating a custom comparator, but then I'd have to reimplement the circular check, as the library doesn't expose the ./utils code.

It would be nice if maybe

• primitive wrappers were detected and unwrapped
• the circularDeepEqual function could accept some optional parameter to provide a comparison, or a flag, or a preprocessor callback (though that would presumably be slower).

[planttheidea]

To level-set ... you keep using the term "primitive wrappers", but these are neither primitives nor wrapping primitive values.

TIL that "primitive wrapper" is actually the term to use. Would not have expected that, but I stand corrected!

const number = new Number(1);

console.log(typeof number); // object
console.log(number === 1); // false

Nonetheless, this is an interesting issue. The reason these are always equal is because the value of the constructed object is not stored on an own property, but rather a hidden slot whose value is only accessible via .valueOf(). When the object comparison is done, structurally they are considered equal because all "own" properties (none, AFAICT) are equal.

I'll need to consider how to approach this, because this new String|Number|Boolean pattern has long fallen out of favor with the JS community, so I consider it an edge case. I'll need to figure out an approach that doesn't punish performance for more common use-cases. My initial reaction is to change the ultimate object comparison fallback to something like:

return a.valueOf() === b.valueOf() || areObjectsEqual(a, b, isEqual, meta);

This should solve the comparisons you are talking about with little additional cost, with the one caveat that true primitives would not be considered equal to their object class instance comparables (1 !== new Number(1)). I think this is an acceptable trade-off, but it may be worth doing a simple normalization, something like:

// original
if (a && b && typeof a === 'object' && typeof b === 'object') {

// change
if (a && b && typeof a === 'object' && typeof b === 'object') {
  a = a.valueOf();
  b = b.valueOf();

This would allow comparisons like 1 === new Number(1), but would have the cost of the valueOf() check for every single non-nullish value. It also wouldn't impact object equality comparison unless the extreme edge case of custom valueOf overrides.

I'll noodle over this.

[haggholm]

I'm not quite sure what the right way is -- if I felt sure, I'd have made a concrete suggestion.

---

...The one caveat that true primitives would not be considered equal to their object class instance comparables (1 !== new Number(1)). I think this is an acceptable trade-off...

I think that would be acceptable (and as it happens, it would also suit my use case). It's the false equality that strikes me as downright misleading (as well as making our use case problematic).

---

but it may be worth doing a simple normalization, something like:

In some code I had, I was just explicitly checking for the wrapper types to unwrap (obviously it wouldn't be exactly the same here):

if (a !== null && typeof a === 'object') {
  if (a instanceof Number || a instanceof Boolean || a instanceof String || a instanceof BigInt) {
    a = a.valueOf();
  }
}

But of course this is fast-equals, which is supposed to be fast, and that seems like a lot of extra work for every object: four instanceof checks?! Though it does seemfaster than unconditionally calling .valueOf() (https://jsbench.me/f8l2giwtnf/1 -- I omitted calling .valueOf() in the instanceof check because I expect that edge case to be vanishingly rare; I added a case omitting BigInt because it's not even constructable, so I have no idea how you'd get a wrapped value).

But still, this is why I suggested that it could be gated by a flag or something to opt into, or opt out of, the mode where it checks for primitive wrappers, which after all are very rarely used. (We don't plan to use them, we just need this part to be bulletproof.)

I guess one could just unconditionally call .valueOf() if it exists; but is it a safe assumption that every object with a .valueOf() method should be considered equivalent to any other object that has one returning an equivalent value? I feel that Number(5) and new Number(5) are equivalent, but primitive wrappers are a rather special case.

I don't know...I expect you know rather better than I do what is fast (and what speed tradeoffs are acceptable).

[planttheidea]

Yeah this is a tough one. I'll need to noodle on it for a bit.

Out of curiosity, do you know how other equality libraries handle this scenario? Lodash, ramda, etc?

[haggholm]

I don't, alas. The only one I've compared this library to so far is fast-deep-equal, and...for the most part, the impression I get is that it gives up earlier. It is what I'm using right now because it better fits the use case where I discovered this, which is particularly sensitive to false positives but not very sensitive to false negatives (in terms of the question of "are A and B equal"?). But even there I have no real insight into the how, just how it relates to our test suite.

[planttheidea]

Alright, after some digging I went with a more targeted valueOf-driven fix. Primitive wrappers have a custom valueOf function, whereas all other object types inherit the one from the Object prototype (unless explicitly overridden), so I just did a simple check if the valueOf on the objects were different, and if so then use it as comparison.

This fix is included in 3.0.2. If you have any more issues, let me know!