Cannot compare object will null prototype about react-fast-compare HOT 11 CLOSED

Fixed in [email protected]

from react-fast-compare.

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

from react-fast-compare.

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

from react-fast-compare.

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

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

from react-fast-compare.

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

from react-fast-compare.

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

from react-fast-compare.

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

from react-fast-compare.

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.

from react-fast-compare.

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

from react-fast-compare.

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!

from react-fast-compare.