recursion in deepEqual() on object graphs with cycles

[mindplay-dk]

I have an object graph with cycles, and I'm testing with tape, and it keeps on throwing RangeError: Maximum call stack size exceeded.

I traced it back to this issue in deep-equal.

It was reported march'15, and it doesn't look like deep-equal has received much maintenance, so maybe it's time to consider switching to either fast-deep-equal or deep-eql, both of which seem to be more popular and more recently active?

Although I didn't see a test-case with circular references in any of those two projects' tests either.

If you let me know how you'd like to address this issue, I can try to help?

[mindplay-dk]

Ugh, tested with fast-deep-equal and it also doesn't break cycles :-(

[mindplay-dk]

Tested with deep-eql as well, and it does work! :-)

Let me know if you'd accept a PR to switch to that?

[ORESoftware]

One thing you can do, is call a custom stringify on both objects, and then compare them after calling JSON parse on them.

const obj1 = {};   // has circular references
const obj2 = {};  // has circular references

const parsed1 = JSON.parse(customStringify(obj1));
const parsed2 = JSON.parse(customStringify(obj2));

// then compare the two parsed objects, with a deep equals routine

Using a custom stringify function like this, which ignores circular references:

export const customStringify = function (v: any) {
  const cache = new Map();
  return JSON.stringify(v, function (key, value) {
    if (typeof value === 'object' && value !== null) {
      if (cache.get(value)) {
        // Circular reference found, discard key
        return;
      }
      // Store value in our collection
      cache.set(value, true);
    }
    return value;
  });
};

[mindplay-dk]

My work-around for now is just to import deep-eql and then t.true(deepEqual(a, b)).

A work-around won't provide good feedback though, for example tap-difflet won't be able to diff.

I could just overwrite Test.prototype.deepEqual, but it would probably be better for everyone if we addressed the problem instead? :-)

[mindplay-dk]

For starters, I wanted to see if deep-eql would pass the test-suite of deep-equals, since, if we were to switch to this package, we don't want to cause breaking changes in people's test-suites.

The deep-eql package doesn't support loose comparison, but I shoehorned it in, and the test-suite of deep-equals is all green.

[ljharb]

I'm really not a fan of switching which dep we're using here. I think the appropriate path is continuing to pursue the relevant change in deep-equal.

[mindplay-dk]

@ljharb why?

[mindplay-dk]

@ljharb I wasn't trying to be short with you, if I came off that way :-)

I am genuinely asking, why wouldn't you switch to a more current/maintained package?

From my perspective, deep-eql handles a far greater number of corner cases, in ways that are particularly meaningful in a testing context - it was built by the chai team, so likely this was built purposefully with a focus on testing. It also has really excellent test coverage.

What's better or more appropriate about enhancing deep-equal instead?

[ORESoftware]

why not both?

use both libs? / make both libs available/exposed

[mindplay-dk]

@ORESoftware how? (and why?)

[ljharb]

@mindplay-dk mainly because there could be any number of edge cases the tests (unfortunately) don't cover, so it could break people, and because the original author of tape also authored deep-equal, which is nice to ensure that issues can get fixed in an appropriate manner, and also to ensure that platform support remains the same (tape still works on node 0.8, for example, and that's unlikely to ever change - does deep-eql provide the same commitment to maintaining backwards compatibility?).

I'd like to see a PR open on deep-equal, which could be reviewed, and then let's see if we can get it merged and released - before trying to make a potentially risky change in tape.

[ljharb]

I now maintain deep-equal, so there's a huge reason to never switch and virtually no reason to ever do so.

Follow inspect-js/node-deep-equal#19 for an underlying fix to support circular references.

[ljharb]

v2 of deep-equal will resolve this, which will be included in v5 of tape.

[ljharb]

deep-equal is upgraded in 898a6e7; closing.

Once v5 is out, this will be resolved.