Replace fast-deep-equal with deep-eql

[mateenkasim]

Related Issue

#111 (recent comment on a closed issue)

Description

• Replace fast-deep-equal with deep-eql for testing object equality
• @material-table/core currently uses fast-deep-equal to test for deep equality, but this package crashes with a call stack overflow when non-React objects with circular references are passed in (i.e. object A references object B which references object A).
• deep-eql is robust against this situation.

Related PRs

List related PRs against other branches:

• None

Impacted Areas in Application

• MaterialTable: doesn't crash with circular objects now

Additional Notes

• Happy to make the same PR into the next branch if necessary, I just can't test it with my current set up.

[Domino987]

What are the cost paying for changing the lib? Did you check those?
It looks like those are 100x slower.

[mateenkasim]

Yup, did my own benchmarks using hyperfine and this gigantic json file, testing all of the following: deep-eql, deep-equal, deep-is, dequal, fast-deep-equal, fast-equals, lodash.isEqual, and react-fast-compare.

Here's the summary of testing non-circular objects:

Summary
  'node benchmark.js dequal' ran
    1.01 ± 0.02 times faster than 'node benchmark.js fast-equals'
    1.02 ± 0.02 times faster than 'node benchmark.js fast-deep-equal'
    1.03 ± 0.02 times faster than 'node benchmark.js react-fast-compare'
    1.21 ± 0.01 times faster than 'node benchmark.js deep-is'
    1.24 ± 0.02 times faster than 'node benchmark.js lodash'
    2.12 ± 0.03 times faster than 'node benchmark.js deep-eql'
   40.71 ± 0.66 times faster than 'node benchmark.js deep-equal'

Most of these packages run around the same time, with deep-eql running twice as long and deep-equal running 40 times as long. I never saw anything indicating a 100x slowdown, let me know if you've seen otherwise.

For circular objects, the only packages that didn't crash or error were lodash, deep-eql, and deep-equal. To be fair, I could change this PR to use lodash.isEqual, but I thought I'd keep with the dedicated equality package since that's what was already here. deep-equal is obviously a poor choice.

I would fully expect a slight performance hit that comes with checking for circular references, since it's a notable addition to the algorithm. That's probably why the 3 working packages are the 3 slowest in the above summary. I just think it's worth it to add robustness to material-table-core in this situation, instead of crashing and requiring users to do some kind of filtering. In my case, such filtering would be a much larger performance hit and would kill some functionality in my app.

PS: Thanks for maintaining this package, the existence of this fork has been a huge help.

[mateenkasim]

Any update on how the mods feel about this one? Anything I can discuss to help get it through? For my use case, it's an absolutely crucial bug fix, I have to imagine others will hit it too.

[Domino987]

Ill merge it and will be monitoring it if it will slow down.