Cannot compare object will null prototype

[zenios]

Objects created using Object.create(null) break when used inside isEqual

react-fast-compare/index.js

Line 76 in 8253fc8

if (a.valueOf !== Object.prototype.valueOf) return a.valueOf() === b.valueOf();

[ryan-roemer]

Upstream issue in fast-deep-equal: epoberezkin/fast-deep-equal#49

[kale-stew]

Just checking in on this - fast-deep-equal is still at v3.1.1, so the fix has not yet been introduced.

[NMinhNguyen]

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

[ryan-roemer]

Hi @NMinhNguyen -- we'll look into taking the proposed fix and tests directly when we have a chance (maybe next week?)

[NMinhNguyen]

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.

[ryan-roemer]

#123 is up for review if anyone wants to kick the tires on it?

[NMinhNguyen]

#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?

[ryan-roemer]

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/ 😂

[ryan-roemer]

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...

[NMinhNguyen]

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/ 😂

@ryan-roemer can confirm it's working now :) thanks once again!

[ryan-roemer]

Fixed in react-fast-compare@3.2.1