Closed zenios closed 1 year ago
Upstream issue in fast-deep-equal
: https://github.com/epoberezkin/fast-deep-equal/issues/49
Just checking in on this - fast-deep-equal
is still at v3.1.1, so the fix has not yet been introduced.
Hey, I do realise that this PR hasn't been merged upstream, but do you have any objections to applying this diff to react-fast-compare
? https://github.com/epoberezkin/fast-deep-equal/pull/134/files#diff-443d5464f90756fa4d7b70e3b5232f7b86ddeeca5d98581a0450a6ce08033e0d
Hi @NMinhNguyen -- we'll look into taking the proposed fix and tests directly when we have a chance (maybe next week?)
Hi @NMinhNguyen -- we'll look into taking the proposed fix and tests directly when we have a chance (maybe next week?)
Sure, no rush! Thank you for getting back to me :) My main hesitation with raising a PR was that I'm not sure if there's any significant performance impact from the additional typeof
checks. That being said, perhaps not throwing errors is more important.
https://github.com/FormidableLabs/react-fast-compare/pull/123 is up for review if anyone wants to kick the tires on it?
https://github.com/FormidableLabs/react-fast-compare/pull/123 is up for review if anyone wants to kick the tires on it?
Thank you so much for this! What's the best way to do so? By patching the dependency with the diff from the PR?
If you're using yarn
, just checkout this git repo and then yarn link the repo in your project.
Or, worst case just manually replace index.js
from this project in your node_modules/react-fast-compare/
😂
One more note about behavior here which is now called out in the tests:
// See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object#null-prototype_objects
{
description: 'Object.create(null) equal to vanilla null prototype deep objects',
value1: Object.assign(Object.create(null), { a: 1, b: { c: true } }),
value2: { __proto__: null, a: 1, b: { c: true } },
equal: true
},
// Object.create(null) has a different `constructor` than a vanilla, non-null object.
{
description: 'Object.create(null) unequal to vanilla deep objects',
value1: Object.assign(Object.create(null), { a: 1, b: { c: true } }),
value2: { a: 1, b: { c: true } },
equal: false
},
Namely, Object.create(null)
has a different .constructor
field than a normal non-null object. I think this is the correct approach for the library, but we could open it up to discussion -- I think the cases in which two objects are created and compared but coming from different original sources (non-null vs null) should be rare. I think "not blowing up" on null prototype should be the focus. And in our case, the library errs to default on the side of returning false
which may be incorrect, while true
should always be correct in terms of usage behavior...
If you're using
yarn
, just checkout this git repo and then yarn link the repo in your project.Or, worst case just manually replace
index.js
from this project in yournode_modules/react-fast-compare/
😂
@ryan-roemer can confirm it's working now :) thanks once again!
Fixed in react-fast-compare@3.2.1
Objects created using Object.create(null) break when used inside isEqual
https://github.com/FormidableLabs/react-fast-compare/blob/8253fc884bd4c5f88e251ea2b80d33be0547376c/index.js#L76