Bug: Set compares by reference not value

[ryan-roemer]

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.

[ryan-roemer]

Now tracked upstream with a potential bugfix: epoberezkin/fast-deep-equal#50

[kale-stew]

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

[chrisbolin]

good catch, @kale-stew! I say we don't take it on yet, but that's a path we could go down

[ryan-roemer]

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

[ryan-roemer]

And as our general note our source already tracks fast-deep-equal/es6 😉

[kevinbarabash]

Do we have to way for fast-deep-equal/es6 to fix this before it can be fixed in this repo?