formidablelabs / react-fast-compare Goto Github PK

The are all dev time alerts, as this package has no dependencies, but we should still clean them up.

https://github.com/FormidableLabs/react-fast-compare/network/alerts

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

I have a props object that's returning false but I have no idea which one.

Having a debug method to tell me that key 'foo' is breaking the comparison would be great

Hello, thanks for the awesome library.

I was wondering if there was an option to ignore arrow / anonymous function values since they always return false (as mentioned by what @chrisbolin wrote here).

Thanks again!

React Popper pulls in via import isEqual from 'react-fast-compare'; and this is failing in Rollupjs which states default is not exported from [module].

Would it be possible for the package that is released to npm contain a cjs and an esm file? This would allow for require() statements as well as import.

I'll happily put a PR together if this is desired, but wanted to know where the thoughts were in the project before putting in the effort.

I have this working now using patch-package with the following patch:

diff --git a/node_modules/react-fast-compare/index.js b/node_modules/react-fast-compare/index.js
index 1301ff0..3fa4874 100644
--- a/node_modules/react-fast-compare/index.js
+++ b/node_modules/react-fast-compare/index.js
@@ -115,7 +115,7 @@ function equal(a, b) {
 }
 // end fast-deep-equal
 
-module.exports = function isEqual(a, b) {
+function isEqual(a, b) {
   try {
     return equal(a, b);
   } catch (error) {
@@ -132,3 +132,5 @@ module.exports = function isEqual(a, b) {
     throw error;
   }
 };
+
+export default isEqual;

Just shaved 5kb off Formik with this. nbd.

react_fast_compare checks for the existence of _store in order to verify the compared object is a React element, and avoid react-specific circular references.

However, this property is not created in production environments (as can be seen at https://github.com/facebook/react/blob/master/packages/react/src/ReactElement.js#L126).

As a result, react_fast_compare doesn't omit circular references in production.

Hi,
I noticed in a bug report from Sentry that /index.js#L67 throws Object doesn't support property or method 'isView' in IE 11.

Broadly speaking, I think the fix would be to check for ArrayBuffer.isView before invoking it:

- if (hasArrayBuffer && ArrayBuffer.isView(a) && ArrayBuffer.isView(b)) {
+ if (hasArrayBuffer && ArrayBuffer.isView && ArrayBuffer.isView(a) && ArrayBuffer.isView(b)) {

I'm happy to submit a PR if you agree to this change.

Thanks.

We have not one but three different ymls with eslint configs. We should re-evaluate these, remove the extras where unnecessary and maybe clean up specific overrides for our test dirs.

1. Root config: here
2. Benchmark test config: here
3. Node/browser test config: here

This could avoid a ton of traversing potentially. Not sure if it's actually a good idea, so opening here for comments but if it is, I'd happily take a stab at it.

Basic idea is if you to save large objects being compared all the time (think fast re-renders with objects), this would use WeakMap.set(objA, aID), and WeakMap.set(objB, bID) and then separately store (aID => bID => true/false). Sound right?

• Replace Travis/Appveyor
• Set up GH actions for linux and windows builds

Sets always evaluate to true, I suspect the behavior is similarly wrong for ES6 Map, WeakMap, and WeakSet:

isEqual(new Set([3]), new Set([6]))
=> true

It seems as if associative arrays are used as properties within an object, changes to the values in the keys of those arrays are not being properly.

For example, in the following code, I have two objects, a and b.
In each object, there is a similarly-named property, columns, which is an associative array.
The associative array in each has a single key, foo, which itself contains an object as the value.
This object is populated with a single key in object a, and two keys in object b.
Comparing these two objects results in a true result from isEqual()

Maintainer's Note: we need help reproducing this bug in react-fast-compare 2.0.4 (or greater). If you have hit this bug, please help us by submitting a repository or code sandbox that shows the behavior. Note, this library does not support circular objects, only React internal circular objects. 😃

Saw a previous issue, but still getting this in 2.0.1. Here's the output:

I'm essentially comparing props, nothing too strange. The objects are all non-circular besides the react children. In dev mode it works just fine, in prod (Uglify compressed) it seems to break.

I'm using these opts:

new UglifyJsPlugin({
        uglifyOptions: {
          ecma: 8,
          toplevel: true,
        },
        sourceMap: true,
        cache: true,
        parallel: true,
      }),

And packages:

[email protected]
[email protected]

Going to look into it further, prod mode is a bit harder to debug obviously but I've isolated where it's happening.

• Hook up coverage in karma (see TODOs)
• Join test coverage lcov output from both node and karma into a joint upload to coveralls.
• Re-enable coveralls badge in README.

I think the recent commit has introduced some compatibility issues in RN.

v2.0.1 - works fine
v2.0.3 - crash

Thanks!

How does the speed of this compare to using JSON.parse(JSON.stringify(obj)) === JSON.parse(JSON.stringify(obj2))?

I use this package in React to detect changes in props. And, I came across a problem that it could not detect the changes of the anonymous function that passed in the props.

Test Case:

{
  description: "same anonymous function is equal",
  value1: () => {
    console.log("func3");
  },
  value2: () => {
    console.log("func3");
  },
  equal: true
},
{
  description: "different anonymous functions are not equal",
  value1: () => {
    console.log("func3");
  },
  value2: () => {
    console.log("func4");
  },
  equal: false
}

To solve this problem, I added the below code before the end of return

  // check for the function
  var funcA = a instanceof Function,
    funcB = b instanceof Function;
  if (funcA != funcB) return false;
  if (funcA && funcB) return a.toString() == b.toString();

  return a !== a && b !== b;

It seem to work fine. But I would like to know if there is a better way to handle this case?

Upstream fast-deep-equal can't handle equal(new Set(["1", {}]), new Set(["1", {}])) because it compares by reference setA.has(setBElement).

Note that when running the correctness tests during yarn benchmark we get differences with lodash.isEqual which is correct:

--- correctness tests: generic and react ---

react-fast-compare
fast-deep-equal
lodash.isEqual
- different result: lodash.isEqual objects objects with different `toString` functions returning same values are equal
- different result: lodash.isEqual Maps empty maps of different class are not equal
- different result: lodash.isEqual Maps not equal maps (same key "order" - instances of different classes)
- different result: lodash.isEqual Sets empty sets of different class are not equal
- different result: lodash.isEqual Sets not equal sets (same value "order" - instances of different classes)
- different result: lodash.isEqual Sets not equal sets (different instances of objects)

Here's a WIP diff against https://github.com/FormidableLabs/react-fast-compare/tree/experiment/es6 branch that could fix that:

diff --git a/index.js b/index.js
index 2f03bfb..6451b7b 100644
--- a/index.js
+++ b/index.js
@@ -56,11 +56,23 @@ function equal(a, b) {
       return true;
     }
 
+    // There's an upstream bug in `fast-deep-equal` for nested `Set`s
+    // which our tests do cover.
+    var bIt, bI;
     if (hasSet && (a instanceof Set) && (b instanceof Set)) {
       if (a.size !== b.size) return false;
       it = a.entries();
-      for (i = it.next(); !i.done; i = it.next())
-        if (!b.has(i.value[0])) return false;
+      aSetLoop: for (i = it.next(); !i.done; i = it.next()) {
+        if (!b.has(i.value[0])) {
+          // NOTE: Modification to fix nested set issue in `fast-deep-equal`
+          // Add manual iteration of all set B values.
+          bIt = b.entries();
+          for (bI = bIt.next(); !bI.done; bI = bIt.next())
+            if (equal(i.value[0], bI.value[0])) continue aSetLoop;
+
+          return false;
+        }
+      }
       return true;
     }
     // END: Modifications
diff --git a/test/node/tests.js b/test/node/tests.js
index 02336e5..3db4c74 100644
--- a/test/node/tests.js
+++ b/test/node/tests.js
@@ -64,9 +64,57 @@ const react = [
   }
 ];
 
+const extraEs6 = [
+  {
+    description: 'Additional es6 tests',
+    reactSpecific: true,
+    tests: [
+      {
+        description: 'nested Maps with same values are equal',
+        value1: new Map([['one', 1], ['map', new Map([['two', 2]])]]),
+        value2: new Map([['one', 1], ['map', new Map([['two', 2]])]]),
+        equal: true
+      },
+      {
+        description: 'nested Maps with different values are not equal',
+        value1: new Map([['one', 1], ['map', new Map([['two', 2]])]]),
+        value2: new Map([['one', 1], ['map', new Map([['three', 3]])]]),
+        equal: false
+      },
+      // Fails in `fast-deep-equal`
+      {
+        description: 'nested Sets with same values are equal',
+        value1: new Set(['one', new Set(['two'])]),
+        value2: new Set(['one', new Set(['two'])]),
+        equal: true
+      },
+      {
+        description: 'nested Sets with different values are not equal',
+        value1: new Set(['one', new Set(['two'])]),
+        value2: new Set(['one', new Set(['three'])]),
+        equal: false
+      },
+      // Fails in `fast-deep-equal`
+      {
+        description: 'nested Maps + Sets with same values are equal',
+        value1: new Map([['one', 1], ['set', new Set(['one', new Set(['two'])])]]),
+        value2: new Map([['one', 1], ['set', new Set(['one', new Set(['two'])])]]),
+        equal: true
+      },
+      {
+        description: 'nested Maps + Sets with different values are not equal',
+        value1: new Map([['one', 1], ['set', new Set(['one', new Set(['two'])])]]),
+        value2: new Map([['one', 1], ['set', new Set(['one', new Set(['three'])])]]),
+        equal: false
+      }
+    ]
+  }
+];
+
 module.exports = {
   generic,
   es6,
+  extraEs6,
   react,
-  all: [...generic, ...es6, ...react],
+  all: [...generic, ...es6, ...extraEs6, ...react],
 };

Could look to open either upstream or in our project.

1. We should use victory-cli to generate an svg based on the benchmarks
2. This should live in the benchmark npm script for easy manual updates
3. The chart should also show which version of each library we are comparing rfc to

Libraries to include in the comparison

• fast-deep-equal (latest v: 3.1.1)
• shallow-equal-fuzzy (latest v: 0.0.2)
• nano-equal (latest v: 2.0.2)
• lodash.isEqual (latest v: 4.5.0)

This fork is currently up to date with [email protected]. Since then there have been two releases (2.0.0 and 2.0.1) and a number of changes that we would benefit from:

• various perf improvements
• bugfix: null and object comparison
• new behavior: different functions are not equal
• new behavior: handle NaN
• misc: update all benchmark packages to the latest versions
• tests: more tests

There are a number of reasons why I think this is a good idea: improved perf and testing, bugfixes, and more accurate results. The most compelling reason is simply to keep up with the forked repo.

I know that the docs suggest isEqual as the default export name for this package, but its not being picked up by my IDE for completion suggestions (PhpStorm)

In

there's a regex comparison with the error message, but it assumes the language of the error is English.
Trying to use the library in IE11, in the event of "out of stack", the error is not captured.

For instance, in a Finnish version of IE11, the message is "Pinotila ei riitä" as seen in the attachment.

One possible fix is to replace the aforementioned line with

if (
(error.message && error.message.match(/stack|recursion/i)) ||
(error.number == -2146828260)
) {

since IE error number for "out of stack" is language-independent.

tip from @marcelmokos:

Since this is a fork we can use https://backstroke.co/ to track changes in the source project.

(in #22)

Hello and thank you for your work. I wanted to ask what do you think about narrowing the type definition in such a way that compared entities would have to conform to a common interface in the first place?

-    const equal: (a: any, b: any) => boolean;
+    const equal: <T>(a: T, b: T) => boolean;

We know that if they are not of the same runtime type, the comparison does not make sense. It would allow the consumers to exlude such cases at compile-time:

type Equal = <T>(a: T, b: T) => boolean;
const equal: Equal = (a, b) => a === b;

declare let someNumber: number;

equal(1, someNumber); // Makes sense
equal(1, ''); // Doesn't make sense

You can play with the idea in TypeScript playground.

Summary

The project where I'd like to use this package uses ImmutableJS for many of its data collections, which are often passed to React as props.

If I were to use this in that library, what would be the best way to handle ImmutableJS comparisons, where the Immutable.is method is preferred.

Thanks!

It seems the typing for react redux hook useSelector relies on the types of the equality function to infer the return type:

// @types/react-redux/index.d.ts
export function useSelector<TState, TSelected>(
    selector: (state: TState) => TSelected,
    equalityFn?: (left: TSelected, right: TSelected) => boolean
): TSelected;

Using isEqual as equality function the compiler will infer any as TSelected type because:

declare module 'react-fast-compare' {
  const isEqual: (a: any, b: any) => boolean
  export default isEqual
}

So

const selector = (state: State): DeepObject => {a: { b: [1,2,3] } }
const value = useSelector(selector) // typed correctly as DeepObject
const value = useSelector(selector, isEqual) // typed as any

Declaring isEqual with generic types should solve the issue

declare module 'react-fast-compare' {
  const isEqual: <A, B>(a: A, b: B) => boolean
  export default isEqual
}

Uncaught SyntaxError: The requested module '/node_modules/react-fast-compare/index.js?v=4009a837' does not provide an export named 'default' (at usePopper.js?v=4009a837:4:8)

Rel formidable.com#1454, we need to update the path to the benchmark chart in our Readme to reference github directly. This will allow for the mirror of our docs on formidable.com to properly import the chart.

Perhaps using Function.toString(), just need to verify that two functions are the same through non-reference equality means since a lot of people write inline anonymous functions these days

• Hook karma to sauce labs. Possible write up something like karma.conf.sauce.js
• Implement full browser matrix tests

This would help for the pattern of getDerivedStateFromProps not having to duplicate the children object:

import { key } from 'react-fast-compare'

  static getDerivedStateFromProps(props, state) {
    if (key(props.children) !== state.childKey) {
      return { childKey: key(props.children) }
    }
    return null
  }

The Object.is class method is very close to === but differs in two ways:

• NaN vs. NaN - === treats these as unequal, but Object.is treats them as equal
• +0 vs. 0 - === treats these as equal, but Object.is treats them as unequal

> Object.is(NaN, NaN)
true
> NaN === NaN
false
> Object.is(+0, -0)
false
> +0 === -0
true

Benefits

• A few unnecessary re-renders could probably be avoided if we treated NaN as equal to itself
• We could be more correct if we treated +0 and -0 as separate

Both of these are pretty minor cases

Downsides

This check doesn't come for free.

Benchmarks on node 8.3.0 using master, Object.is, and the MDN polyfill.

The native method is about 17% slower, and the polyfill is about 7.5% slower.

--- speed tests: generic usage ---

react-fast-compare x 175,552 ops/sec ±1.63% (87 runs sampled)
react-fast-compare-object-is x 146,104 ops/sec ±2.18% (85 runs sampled)
react-fast-compare-object-is-polyfill x 162,487 ops/sec ±1.62% (84 runs sampled)
  fastest: react-fast-compare

--- speed tests: generic and react ---

react-fast-compare x 140,986 ops/sec ±1.54% (85 runs sampled)
react-fast-compare-object-is x 122,360 ops/sec ±1.07% (89 runs sampled)
react-fast-compare-object-is-polyfill x 138,398 ops/sec ±1.42% (85 runs sampled)
  fastest: react-fast-compare,react-fast-compare-object-is-polyfill

Code for benchmarks is here: #9

BLOCKERS: Fix #61 and #62 in a beta release first.

• in the README document react-fast-compare's approach to typing: we need to support a huge variety of usages, and therefore the typing must be simple and should probably not be changed 😈. Reference all of the issues and PRs we've already had.
• while the #61 fix is still in beta, direct users interested in types to that beta.
• make a github issue template. Explicitly mention types and direct the filer to the README documentation
• create TypeScript compilation tests that cover the issues/PRs we've seen so far
• ensure any other open issues are PRs are updated (probably closed) accordingly

Any of these can be made into separate issues to make tracking easier.