Open ryan-roemer opened 4 years ago
Now tracked upstream with a potential bugfix: https://github.com/epoberezkin/fast-deep-equal/issues/50
Looking at the solution comment on the related issue linked by @ryan-roemer above ^, it appears we can solve this issue by requiring fast-deep-equal/es6
instead of fast-deep-equal
.
Is this something we would be willing to change? It seems like there are going to be larger implications updating this dependency, but I don't know that I can immediately identify what they'll be.
cc @chrisbolin
good catch, @kale-stew! I say we don't take it on yet, but that's a path we could go down
@chrisbolin @kale-stew -- Upstream FDE AFAICT incorrectly closed the issue. Their current master
https://github.com/epoberezkin/fast-deep-equal/blob/master/src/index.jst#L34-L39 still does reference comparison and does not do a recursive equality check as my POC experiment code above does.
And as our general note our source already tracks fast-deep-equal/es6
😉
Do we have to way for fast-deep-equal/es6
to fix this before it can be fixed in this repo?
Upstream
fast-deep-equal
can't handleequal(new Set(["1", {}]), new Set(["1", {}]))
because it compares by referencesetA.has(setBElement)
.Note that when running the correctness tests during
yarn benchmark
we get differences withlodash.isEqual
which is correct:Here's a WIP diff against https://github.com/FormidableLabs/react-fast-compare/tree/experiment/es6 branch that could fix that:
Could look to open either upstream or in our project.