Merge pull request #75 from HubSpot/adam/handle-immutable-compares

fix(useStoreDependency): use shallowCompare to compare results

Author: aem

CHANGELOG.md

@@ -1,3 +1,6 @@
+## 3.2.0
+* Fix potential infinite loop with Immutable dependency values (#74)
+
 ## 3.1.0
 * Use `UNSAFE_` prefix for `componentWillMount`, `componentWillReceiveProps` usages in `connect` HOC.
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "general-store",
-  "version": "3.1.0",
+  "version": "3.2.0",
   "description": "Simple, flexible store implementation for Flux.",
   "main": "lib/GeneralStore.js",
   "scripts": {
@@ -59,6 +59,7 @@
     "eslint-plugin-react-app": "^4.0.1",
     "eslint-plugin-react-hooks": "^1.5.1",
     "flux": "^2.0.1",
+    "immutable": "3.8.1",
     "immutable-is": "^3.7.4",
     "invariant": "^2.2.1",
     "jest": "24.5.0",

src/dependencies/__tests__/useStoreDependency-test.js

@@ -7,6 +7,7 @@ import useStoreDependency from '../useStoreDependency';
 import { Dispatcher } from 'flux';
 import { set as setDispatcherInstance } from '../../dispatcher/DispatcherInstance';
 import TestUtils from 'react-dom/test-utils';
+import { List, Map } from 'immutable';
 
 configure({ adapter: new Adapter() });
 
@@ -159,6 +160,30 @@ describe('useStoreDependency', () => {
       rendered.update();
       expect(renders).toBe(2);
     });
+
+    it("doesn't trigger an infinite loop when using immutable objects", () => {
+      const store = new StoreFactory({
+        getter: state => state,
+        getInitialState: () => List([Map({ a: 1 })]),
+        responses: {
+          updateImmutable: (state, newValue) => newValue,
+        },
+      }).register();
+      const Component = () => {
+        useStoreDependency({
+          stores: [store],
+          deref: () => List([Map({ a: 5 })]),
+        });
+
+        return null;
+      };
+      mount(<Component />);
+      // no assertions to make, but this test will fail if equality is
+      // not implemented correctly, as the immutables will not be strictly
+      // equal, sending useStoreDependency into an infinite loop
+      // https://github.com/HubSpot/general-store/issues/74
+      expect(true).toBe(true);
+    });
   });
 
   describe('with props', () => {

src/dependencies/useStoreDependency.ts

@@ -10,6 +10,7 @@ import { get as getDispatcherInstance } from '../dispatcher/DispatcherInstance';
 import { enforceDispatcher } from '../dispatcher/DispatcherInterface';
 import { handleDispatch } from './Dispatch';
 import { Dispatcher } from 'flux';
+import { shallowEqual } from '../utils/ObjectUtils';
 
 type SingleDependency = {
   [key: string]: Dependency;
@@ -50,7 +51,7 @@ function useStoreDependency<Props>(
             entry,
             currProps.current
           );
-          if (newValue !== dependencyValue) {
+          if (!shallowEqual(newValue, dependencyValue)) {
             setDependencyValue(newValue);
           }
         }
@@ -62,7 +63,7 @@ function useStoreDependency<Props>(
   }, [dispatcher, dependencyValue, dependency, currProps]);
 
   const newValue = calculate(dependency, props);
-  if (newValue !== dependencyValue) {
+  if (!shallowEqual(newValue, dependencyValue)) {
     setDependencyValue(newValue);
   }
   return dependencyValue;

src/utils/ObjectUtils.ts

@@ -63,6 +63,27 @@ export function shallowEqual(obj1: any, obj2: any): Boolean {
   if (obj1 === obj2) {
     return true;
   }
+
+  // Special handling for Immutables, as they must be handled specially.
+  // While this technically means this is deep equality for Immutables,
+  // it's better to have a more specific check than an entirely incorrect one.
+  if (
+    typeof obj1.hashCode === 'function' &&
+    typeof obj2.hashCode === 'function'
+  ) {
+    // `hashCode` is guaranteed to be the same if the objects are the same, but
+    // is NOT guaranteed to be different if the objects are different (hash
+    // collisions are possible). If the hash codes are different, then we can
+    // preemptively return false as a performance optimization. Otherwise,
+    // return the result of a call to `equals`.
+    // https://github.com/immutable-js/immutable-js/blob/59c291a2b37693198a0950637c5d55cd14dd6bc4/src/is.js#L52-L55
+    if (obj1.hashCode() !== obj2.hashCode()) {
+      return false;
+    } else if (typeof obj1.equals === 'function') {
+      return obj1.equals(obj2);
+    }
+  }
+
   if (typeof obj1 !== typeof obj2) {
     return false;
   }

src/utils/__tests__/ObjectUtils-test.js

@@ -1,5 +1,6 @@
 jest.unmock('../ObjectUtils');
 
+import { Map } from 'immutable';
 import {
   oForEach,
   oFilterMap,
@@ -86,5 +87,27 @@ describe('ObjectUtils', () => {
         false
       );
     });
+
+    it('shallowly compares immutable values', () => {
+      expect(shallowEqual(Map({ a: 1 }), Map({ a: 1 }))).toBe(true);
+      const mockImmutable1 = {
+        hashCode() {
+          return 1;
+        },
+        equals() {
+          return false;
+        },
+      };
+      const mockImmutable2 = {
+        hashCode() {
+          return 1;
+        },
+        equals() {
+          return false;
+        },
+      };
+      // force hash collision - should fall back to .equals
+      expect(shallowEqual(mockImmutable1, mockImmutable2)).toBe(false);
+    });
   });
 });

yarn.lock

@@ -2371,6 +2371,11 @@ immutable-is@^3.7.4:
   version "3.7.6"
   resolved "https://registry.yarnpkg.com/immutable-is/-/immutable-is-3.7.6.tgz#efad8bcf21443392402e10fa08b7aa91b65f6c30"
 
+immutable@3.8.1:
+  version "3.8.1"
+  resolved "https://registry.yarnpkg.com/immutable/-/immutable-3.8.1.tgz#200807f11ab0f72710ea485542de088075f68cd2"
+  integrity sha1-IAgH8Rqw9ycQ6khVQt4IgHX2jNI=
+
 immutable@^3.7.4:
   version "3.8.2"
   resolved "https://registry.yarnpkg.com/immutable/-/immutable-3.8.2.tgz#c2439951455bb39913daf281376f1530e104adf3"