Refactor evaluator to cache getters by reference, not hash code

Author: jordangarcia

src/evaluator.js

@@ -1,6 +1,5 @@
 var Immutable = require('immutable')
 var toImmutable = require('./immutable-helpers').toImmutable
-var hashCode = require('./hash-code')
 var isEqual = require('./is-equal')
 var getComputeFn = require('./getter').getComputeFn
 var getDeps = require('./getter').getDeps
@@ -14,8 +13,8 @@ class Evaluator {
   constructor() {
     /**
      * {
-     *   <hashCode>: {
-     *     stateHashCode: number,
+     *   <Getter>: {
+     *     state: Immutable.Map,
      *     args: Immutable.List,
      *     value: any,
      *   }
@@ -46,12 +45,10 @@ class Evaluator {
     }
 
     // Must be a Getter
-    var code = hashCode(keyPathOrGetter)
-
     // if the value is cached for this dispatch cycle, return the cached value
     if (this.__isCached(state, keyPathOrGetter)) {
       // Cache hit
-      return this.__cachedGetters.getIn([code, 'value'])
+      return this.__cachedGetters.getIn([keyPathOrGetter, 'value'])
 
     }
 
@@ -60,11 +57,11 @@ class Evaluator {
 
     if (this.__hasStaleValue(state, keyPathOrGetter)) {
       // getter deps could still be unchanged since we only looked at the unwrapped (keypath, bottom level) deps
-      var prevArgs = this.__cachedGetters.getIn([code, 'args'])
+      var prevArgs = this.__cachedGetters.getIn([keyPathOrGetter, 'args'])
 
       // since Getter is a pure functions if the args are the same its a cache hit
       if (isEqual(prevArgs, toImmutable(args))) {
-        var prevValue = this.__cachedGetters.getIn([code, 'value'])
+        var prevValue = this.__cachedGetters.getIn([keyPathOrGetter, 'value'])
         this.__cacheValue(state, keyPathOrGetter, prevArgs, prevValue)
         return prevValue
       }
@@ -97,11 +94,10 @@ class Evaluator {
    * @param {Getter} getter
    */
   __hasStaleValue(state, getter) {
-    var code = hashCode(getter)
     var cache = this.__cachedGetters
     return (
-      cache.has(code) &&
-      cache.getIn([code, 'stateHashCode']) !== state.hashCode()
+      cache.has(getter) &&
+      !Immutable.is(cache.getIn([getter, 'state']), state)
     )
   }
 
@@ -113,11 +109,10 @@ class Evaluator {
    * @param {any} value
    */
   __cacheValue(state, getter, args, value) {
-    var code = hashCode(getter)
-    this.__cachedGetters = this.__cachedGetters.set(code, Immutable.Map({
+    this.__cachedGetters = this.__cachedGetters.set(getter, Immutable.Map({
       value: value,
       args: toImmutable(args),
-      stateHashCode: state.hashCode(),
+      state: state,
     }))
   }
 
@@ -128,10 +123,9 @@ class Evaluator {
    * @return {boolean}
    */
   __isCached(state, getter) {
-    var code = hashCode(getter)
     return (
-      this.__cachedGetters.hasIn([code, 'value']) &&
-      this.__cachedGetters.getIn([code, 'stateHashCode']) === state.hashCode()
+      this.__cachedGetters.hasIn([getter, 'value']) &&
+      Immutable.is(this.__cachedGetters.getIn([getter, 'state']), state)
     )
   }
 

test.js

@@ -0,0 +1,53 @@
+var Nuclear = require("./dist/nuclear"); // v1.1.1
+
+var SET_YEAR_GROUP = "SET_YEAR_GROUP";
+var LOADED = "LOADED";
+
+function runTest(value) {
+
+  var store = Nuclear.Store({
+    getInitialState: function() {
+      return Nuclear.toImmutable({
+        yearGroup: 0,
+        shouldLoaded: true
+      });
+    },
+
+    initialize: function() {
+      this.on(SET_YEAR_GROUP, setYearGroup);
+      this.on(LOADED, loaded);
+    }
+  });
+
+  function setYearGroup(store, payload) {
+    return store
+      .set("yearGroup", payload.yearGroup)
+      .set("shouldLoad", true);
+  }
+
+  function loaded(store) {
+    return store.set("shouldLoad", false);
+  }
+
+  var reactor = new Nuclear.Reactor();
+
+  reactor.registerStores({ uiStore: store });
+
+  var output = [];
+  // Record changes to yearGroup
+  reactor.observe(["uiStore", "yearGroup"], function(y) { output.push(y) });
+
+  reactor.dispatch(SET_YEAR_GROUP, {yearGroup: 6});
+  reactor.dispatch(LOADED);
+  reactor.dispatch(SET_YEAR_GROUP, {yearGroup: value});
+
+  return output;
+}
+
+//var output1 = runTest(1);
+//console.log(output1); // OK - logs [ 6, 1 ]
+
+var output5 = runTest(5);
+console.log(output5); // WRONG! Logs [ 6 ] instead of [ 6, 5 ] !
+
+// Removing the reactor.dispatch("LOADED"); call seems to fix things, but why?!

tests/reactor-tests.js

@@ -1144,4 +1144,49 @@ describe('Reactor', () => {
       }).not.toThrow()
     })
   })
+
+  describe('issue #140 - change observer error case dealing with hashCode collisions', () => {
+    it('observer should be called correctly', () => {
+      var SET_YEAR_GROUP = 'SET_YEAR_GROUP'
+      var LOADED = 'LOADED'
+
+      var store = Store({
+        getInitialState: function() {
+          return toImmutable({
+            yearGroup: 0,
+            shouldLoaded: true,
+          })
+        },
+
+        initialize: function() {
+          this.on(SET_YEAR_GROUP, setYearGroup)
+          this.on(LOADED, loaded)
+        },
+      })
+
+      function setYearGroup(store, payload) {
+        return store
+          .set('yearGroup', payload.yearGroup)
+          .set('shouldLoad', true)
+      }
+
+      function loaded(store) {
+        return store.set('shouldLoad', false)
+      }
+
+      var reactor = new Reactor()
+
+      reactor.registerStores({ uiStore: store })
+
+      var output = []
+      // Record changes to yearGroup
+      reactor.observe(['uiStore', 'yearGroup'], function(y) { output.push(y) })
+
+      reactor.dispatch(SET_YEAR_GROUP, {yearGroup: 6})
+      reactor.dispatch(LOADED)
+      reactor.dispatch(SET_YEAR_GROUP, {yearGroup: 5})
+
+      expect(output).toEqual([6, 5])
+    })
+  })
 })