All tests passing for reactor refactor

Author: jordangarcia

src/key-path.js

@@ -1,3 +1,4 @@
+import Immutable from 'immutable'
 var isArray = require('./utils').isArray
 var isFunction = require('./utils').isFunction
 
@@ -12,3 +13,16 @@ exports.isKeyPath = function(toTest) {
     !isFunction(toTest[toTest.length - 1])
   )
 }
+
+/**
+ * Checks if two keypaths are equal by value
+ * @param {KeyPath} a
+ * @param {KeyPath} a
+ * @return {Boolean}
+ */
+exports.isEqual = function(a, b) {
+  const iA = Immutable.List(a)
+  const iB = Immutable.List(b)
+
+  return Immutable.is(iA, iB)
+}

src/reactor.js

@@ -7,8 +7,6 @@ import { toJS } from './immutable-helpers'
 import { isArray, toFactory } from './utils'
 import { ReactorState, ObserverStoreMap } from './reactor/records'
 
-// keep a singleton app state identity getter so we can leverage by reference keying
-const APP_STATE_IDENTITY_GETTER = [[], x => x]
 /**
  * State is stored in NuclearJS Reactors.  Reactors
  * contain a 'state' object which is an Immutable.Map
@@ -19,12 +17,7 @@ const APP_STATE_IDENTITY_GETTER = [[], x => x]
  * state based on the message
  */
 class Reactor {
-  constructor(config) {
-    if (!(this instanceof Reactor)) {
-      return new Reactor(config)
-    }
-    config = config || {}
-
+  constructor(config = {}) {
     var initialReactorState = new ReactorState({
       debug: config.debug
     })
@@ -37,8 +30,6 @@ class Reactor {
 
     // keep track of the depth of batch nesting
     this.__batchDepth = 0
-    // number of dispatches in the top most batch cycle
-    this.__batchDispatchCount = 0
 
     // keep track if we are currently dispatching
     this.__isDispatching = false
@@ -83,20 +74,12 @@ class Reactor {
   observe(getter, handler) {
     if (arguments.length === 1) {
       handler = getter
-      getter = APP_STATE_IDENTITY_GETTER
-    }
-    // by value check if getter === []
-    if (isArray(getter) && getter.length === 0) {
-      getter = APP_STATE_IDENTITY_GETTER
+      getter = []
     }
-    // TODO dont mutate getter to allow by reference unobservation of keypaths (since they are getting transformed into identity getter)
-    let { observerStoreMap, unwatchEntry } = fns.addObserver(this.observerStoreMap, getter, handler)
+    let { observerStoreMap, entry } = fns.addObserver(this.observerStoreMap, getter, handler)
     this.observerStoreMap = observerStoreMap;
     return () => {
-      this.unobserve(
-        unwatchEntry.get('getter'),
-        unwatchEntry.get('hander')
-      )
+      this.observerStoreMap = fns.removeObserverByEntry(this.observerStoreMap, entry)
     }
   }
 
@@ -108,11 +91,6 @@ class Reactor {
       throw new Error('Must call unobserve with a Getter')
     }
 
-    // by value check if getter === []
-    if (isArray(getter) && getter.length === 0) {
-      getter = APP_STATE_IDENTITY_GETTER
-    }
-
     this.observerStoreMap = fns.removeObserver(this.observerStoreMap, getter, handler)
   }
 
@@ -137,14 +115,9 @@ class Reactor {
       throw e
     }
 
-    if (this.__batchDepth > 0) {
-      this.__batchDispatchCount++
-    }
-
     try {
       this.__notify()
     } finally {
-      console.log('DISPATCHING FALSE')
       this.__isDispatching = false
     }
   }
@@ -154,9 +127,9 @@ class Reactor {
    * @param {Function} fn
    */
   batch(fn) {
-    this.__batchStart()
+    this.batchStart()
     fn()
-    this.__batchEnd()
+    this.batchEnd()
   }
 
   /**
@@ -188,7 +161,7 @@ class Reactor {
    * @return {Object}
    */
   serialize() {
-    return this.reactorState.serialize()
+    return fns.serialize(this.reactorState)
   }
 
   /**
@@ -217,30 +190,22 @@ class Reactor {
       // in the middle of batch, dont notify
       return
     }
-    const dirtyStores = this.reactorState.get('dirtyStores')
 
+    const dirtyStores = this.reactorState.get('dirtyStores')
     if (dirtyStores.size === 0) {
       return
     }
 
     let observerIdsToNotify = Immutable.Set().withMutations(set => {
       // notify all observers
-      this.observerStoreMap.get('any').forEach(handlerMap => {
-        handlerMap.forEach((observerId) => {
-          set.add(observerId);
-        })
-      })
+      set.union(this.observerStoreMap.get('any'))
 
       dirtyStores.forEach(id => {
         const entries = this.observerStoreMap.getIn(['stores', id])
         if (!entries) {
           return
         }
-        entries.forEach((handlerMap, getter) => {
-          handlerMap.forEach((observerId) => {
-            set.add(observerId)
-          })
-        })
+        set.union(entries)
       })
     })
 
@@ -271,26 +236,31 @@ class Reactor {
     this.reactorState = nextReactorState
   }
 
-  __batchStart() {
+  /**
+   * Starts batching, ie pausing notifies and batching up changes
+   * to be notified when batchEnd() is called
+   */
+  batchStart() {
     this.__batchDepth++
   }
 
-  __batchEnd() {
+  /**
+   * Ends a batch cycle and will notify obsevers of all changes if
+   * the batch depth is back to 0 (outer most batch completed)
+   */
+  batchEnd() {
     this.__batchDepth--
 
     if (this.__batchDepth <= 0) {
-      if (this.__batchDispatchCount > 0) {
-        // set to true to catch if dispatch called from observer
-        this.__isDispatching = true
-        try {
-          this.__notify()
-        } catch (e) {
-          this.__isDispatching = false
-          throw e
-        }
+      // set to true to catch if dispatch called from observer
+      this.__isDispatching = true
+      try {
+        this.__notify()
+      } catch (e) {
         this.__isDispatching = false
+        throw e
       }
-      this.__batchDispatchCount = 0
+      this.__isDispatching = false
     }
   }
 }

src/reactor/fns.js

@@ -3,9 +3,8 @@ import logging from '../logging'
 import { isImmutableValue } from '../immutable-helpers'
 import { toImmutable } from '../immutable-helpers'
 import { fromKeyPath, getStoreDeps, getComputeFn, getDeps, isGetter } from '../getter'
-import { isKeyPath } from '../key-path'
+import { isEqual, isKeyPath } from '../key-path'
 import { each } from '../utils'
-import isEqual from '../is-equal'
 
 /**
  * Immutable Types
@@ -96,20 +95,24 @@ exports.dispatch = function(reactorState, actionType, payload) {
  * @return {ReactorState}
  */
 exports.loadState = function(reactorState, state) {
-  var stateToLoad = toImmutable({}).withMutations(stateToLoad => {
+  let dirtyStores = []
+  const stateToLoad = toImmutable({}).withMutations(stateToLoad => {
     each(state, (serializedStoreState, storeId) => {
       var store = reactorState.getIn(['stores', storeId])
       if (store) {
         var storeState = store.deserialize(serializedStoreState)
         if (storeState !== undefined) {
           stateToLoad.set(storeId, storeState)
+          dirtyStores.push(storeId)
         }
       }
     })
   })
 
-  var newState = reactorState.get('state').merge(stateToLoad)
-  return reactorState.set('state', newState)
+  var dirtyStoresSet = Immutable.Set(dirtyStores)
+  return reactorState
+    .update('state', state => state.merge(stateToLoad))
+    .update('dirtyStores', stores => stores.union(dirtyStoresSet))
 }
 
 /**
@@ -171,33 +174,35 @@ exports.addObserver = function(observerStoreMap, getter, handler) {
   const currId = observerStoreMap.get('nextId')
   const storeDeps = getStoreDeps(getter)
   const entry = Immutable.Map({
+    id: currId,
     storeDeps: storeDeps,
     getterKey: getterKey,
     getter: getter,
     handler: handler,
-    unwatched: false,
   })
 
   let updatedObserverStoreMap
   if (storeDeps.size === 0) {
-    updatedObserverStoreMap = observerStoreMap.setIn(['any', getterKey, handler], currId)
+    updatedObserverStoreMap = observerStoreMap.update('any', observerIds => observerIds.add(currId))
   } else {
     updatedObserverStoreMap = observerStoreMap.withMutations(map => {
       storeDeps.forEach(storeId => {
-        map.setIn(['stores', storeId, getterKey, handler], currId);
+        let path = ['stores', storeId]
+        if (!map.hasIn(path)) {
+          map.setIn(path, Immutable.Set([]))
+        }
+        map.updateIn(['stores', storeId], observerIds => observerIds.add(currId));
       })
     })
   }
 
   updatedObserverStoreMap = updatedObserverStoreMap
     .set('nextId', currId + 1)
-    .update('observersMap', entries => {
-      return entries.set(currId, entry)
-    })
+    .setIn(['observersMap', currId], entry)
 
   return {
     observerStoreMap: updatedObserverStoreMap,
-    unwatchEntry: entry,
+    entry: entry,
   }
 }
 
@@ -211,43 +216,47 @@ exports.addObserver = function(observerStoreMap, getter, handler) {
  * removeObserver(observerStoreMap, getter, handler)
  * @param {ObserverStoreMap} observerStoreMap
  * @param {KeyPath|Getter} getter
- * @param {function} handler
+ * @param {Function} handler
  * @return {ObserverStoreMap}
  */
 exports.removeObserver = function(observerStoreMap, getter, handler) {
-  debugger;
-  const getterKey = getter
-  if (isKeyPath(getter)) {
-    getter = fromKeyPath(getter)
-  }
-  const storeDeps = getStoreDeps(getter)
-  let pathsToRemove = []
-
-  if (storeDeps.size === 0) {
-    let path = ['any', getterKey]
-    if (handler) {
-      path.push(handler)
+  const entriesToRemove = observerStoreMap.get('observersMap').filter(entry => {
+    // use the getterKey in the case of a keyPath is transformed to a getter in addObserver
+    let entryGetter = entry.get('getterKey')
+    let handlersMatch = (!handler || entry.get('handler') === handler)
+    if (!handlersMatch) {
+      return false
     }
-    pathsToRemove.push(path)
-  } else {
-    storeDeps.forEach(storeId => {
-      let path = ['stores', storeId, getterKey]
-      if (handler) {
-        path.push(handler)
-      }
-      pathsToRemove.push(path)
-    })
-  }
+    // check for a by-value equality of keypaths
+    if (isKeyPath(getter) && isKeyPath(entryGetter)) {
+      return isEqual(getter, entryGetter)
+    }
+    // we are comparing two getters do it by reference
+    return (getter === entryGetter)
+  })
 
   return observerStoreMap.withMutations(map => {
-    pathsToRemove.forEach(path => {
-      var observerId = observerStoreMap.getIn(path)
+    entriesToRemove.forEach(entry => exports.removeObserverByEntry(map, entry))
+  })
+}
 
-      map.removeIn(path)
-      if (observerId) {
-        map.removeIn(['observersMap', observerId])
-      }
-    })
+/**
+ * Removes an observer entry by id from the observerStoreMap
+ */
+exports.removeObserverByEntry = function(observerStoreMap, entry) {
+  return observerStoreMap.withMutations(map => {
+    const id = entry.get('id')
+    const storeDeps = entry.get('storeDeps')
+
+    if (storeDeps.size === 0) {
+      map.update('any', anyObsevers => anyObsevers.remove(id))
+    } else {
+      storeDeps.forEach(storeId => {
+        map.updateIn(['stores', storeId], observers => observers.remove(id))
+      })
+    }
+
+    map.removeIn(['observersMap', id])
   })
 }
 
@@ -334,6 +343,23 @@ exports.evaluate = function evaluate(reactorState, keyPathOrGetter) {
   )
 }
 
+/**
+ * Returns serialized state for all stores
+ * @param {ReactorState} reactorState
+ * @return {Object}
+ */
+exports.serialize = function(reactorState) {
+  let serialized = {}
+  reactorState.get('stores').forEach((store, id) => {
+    let storeState = reactorState.getIn(['state', id])
+    let serializedState = store.serialize(storeState)
+    if (serializedState !== undefined) {
+      serialized[id] = serializedState
+    }
+  })
+  return serialized
+}
+
 /**
  * @param {ReactorState} reactorState
  * @param {Getter} getter

src/reactor/records.js

@@ -11,7 +11,7 @@ const ReactorState = Immutable.Record({
 
 const ObserverStoreMap = Immutable.Record({
   // observers registered to any store change
-  any: Immutable.Map({}),
+  any: Immutable.Set([]),
   // observers registered to specific store changes
   stores: Immutable.Map({}),