fast-deep-equal - Githubissues

mindplay-dk commented 3 years ago

I just ran into this issue, which they were never able to fix - or at least not without considerably degrading the performance.

As you can see from my bug report:

Having tested for this issue with both deep-equal, fast-deep-equal and deep-eql, I found that the only library where this works, is deep-eql

The issue I'm trying to debug right now is actually not related to circular references, and I have not been unable to pinpoint what the issue is.

I'm comparing two decoded JSON objects - so these can't contain circular references, and they can't contain anything other than JSON values.

But for some reason, test.equal thinks they are not equal - and still, diffReporter output shows they are completely equal, line for line - nothing red or green, all grey. Very confusing.

To check my own conclusions, I installed fast-deep-equal and deep-eql and manually called them in a test - which confirmed my suspicion: fast-deep-equal does not think these values are equal, whereas deep-eql reports no difference.

I went as far as to dump the two objects to disk with JSON.stringify and open them in a text-editor, and they are identical.

Unfortunately, I can't share the code as the project is proprietary, and I haven't been able to isolate the issue. I can't explain what's happening, only that something is not right. My only remaining guess is property enumeration order? It shouldn't matter, right? Object properties do have order in JS, but it's not significant - I don't know if fast-deep-equal checks property order or not.

I don't particularly want to spend anymore time on it. I know from past experience that deep-eql works and fast-deep-equal has some weird corner cases - including at least in this case here, which zora's diff reporter isn't able to detect either, so deep-eql appears to be more compatible with zora's diff reporter. (I don't know if zora's diff reporter handles circular references - if it does, that would be one more reason to prefer deep-eql.)

I would suggest we switch to deep-eql?

I know fast-deep-equal may be faster, but for testing I think maybe it's more important to be correct?

lorenzofox3 commented 3 years ago

There are two different topics here:

object equality which can tricky in some cases. However it is often related to a prototypal inheritance gotcha should {} equal Object.create(null) ? or even should Object.create({method(){}}, { data1: {value:'foo'}, data2: {value:'bis'}}) equal { data1:'foo', data2: 'bis'}
how objects are serialized (by the reporter for example) but that is orthogonal

I believe the first point is more a matter of opinion and at the end if people want to do fancy stuff with their objects, they should handle object comparison themselves (ie by spreading theirs fancy objects first, etc). The Object.create(null) is well known because of a popular graphQL lib with a clumsy impl.

I went as far as to dump the two objects to disk with JSON.stringify and open them in a text-editor, and they are identical.

In the process of serialize, some non enumarable props or prototype dependencies can be dropped, yet should the object considered equivalent ? That is a matter of opinion.

at the end, I am pretty happy with fast-deep-equal and I don't see for now any reason to switch to another library except accommodating the assert library to questionable choices some libraries made.

Moreover, as zora is easy to extend (or ovewrite here), I don't see the point.

import { Assert } from 'zora';
import deepEql from 'whateverLib';

// overwrite
Assert.equal = (
  actual,
  expected,
  description = 'should be equivalent'
) => ({
  pass: deepEql(actual, expected),
  actual,
  expected,
  description,
  operator: 'equal',
});

// or enhance
Assert.checkProps = (
  actual,
  expected,
  description = 'should be equivalent'
) => ({
  pass: deepEql(actual, expected),
  actual,
  expected,
  description,
  operator: 'equal',
});

and that's it, you have patched zora

lorenzofox3 commented 3 years ago

see #85

mindplay-dk commented 3 years ago

So I overwrote Assert.equal and now that one passes, but now I have other failing tests, haha.

So you were right, one library isn't more correct than another - it's probably mostly a matter of which edge cases you happened to run into with whatever library you picked.

Ugh, now I'm tempted to go down a rabbit-role and run every individual deep-equal library through all the combined test suites of every single library. 🤨

lorenzofox3 / zora

fast-deep-equal #120