Merge pull request #224 from optimizely/jordan/add-notify-logging

Add logger hooks for notify

Author: jordangarcia

src/logging.js

@@ -39,7 +39,8 @@ export const ConsoleGroupLogger = {
   /**
    * @param {ReactorState} reactorState
    * @param {Map} state
-   * @param {Set} dirtyStores
+   * @param {List} dirtyStores
+   * @param {Map} previousState
    */
   dispatchEnd: function(reactorState, state, dirtyStores, previousState) {
     if (!getOption(reactorState, 'logDispatches')) {
@@ -57,29 +58,33 @@ export const ConsoleGroupLogger = {
       console.groupEnd()
     }
   },
-}
-
-/* eslint-enable no-console */
-
-export const NoopLogger = {
   /**
    * @param {ReactorState} reactorState
-   * @param {String} type
-   * @param {*} payload
+   * @param {ObserverState} observerState
    */
-  dispatchStart: function(reactorState, type, payload) {
+  notifyStart: function(reactorState, observerState) {
   },
   /**
    * @param {ReactorState} reactorState
-   * @param {Error} error
+   * @param {Getter} getter
    */
-  dispatchError: function(reactorState, error) {
+  notifyEvaluateStart: function(reactorState, getter) {
   },
   /**
    * @param {ReactorState} reactorState
-   * @param {Map} state
-   * @param {Set} dirtyStores
+   * @param {Getter} getter
+   * @param {Boolean} didCall
+   * @param {*} currValue
+   */
+  notifyEvaluateEnd: function(reactorState, getter, didCall, currValue) {
+  },
+  /**
+   * @param {ReactorState} reactorState
+   * @param {ObserverState} observerState
    */
-  dispatchEnd: function(reactorState, state, dirtyStores) {
+  notifyEnd: function(reactorState, observerState) {
   },
 }
+
+/* eslint-enable no-console */
+

src/reactor.js

@@ -2,7 +2,7 @@ import Immutable from 'immutable'
 import createReactMixin from './create-react-mixin'
 import * as fns from './reactor/fns'
 import { DefaultCache } from './reactor/cache'
-import { NoopLogger, ConsoleGroupLogger } from './logging'
+import { ConsoleGroupLogger } from './logging'
 import { isKeyPath } from './key-path'
 import { isGetter } from './getter'
 import { toJS } from './immutable-helpers'
@@ -29,7 +29,7 @@ class Reactor {
     const baseOptions = debug ? DEBUG_OPTIONS : PROD_OPTIONS
     // if defined, merge the custom implementation over the noop logger to avoid undefined lookups,
     // otherwise, just use the built-in console group logger
-    let logger = config.logger ? extend({}, NoopLogger, config.logger) : NoopLogger
+    let logger = config.logger ? config.logger : {}
     if (!config.logger && debug) {
       logger = ConsoleGroupLogger
     }
@@ -225,6 +225,8 @@ class Reactor {
       return
     }
 
+    fns.getLoggerFunction(this.reactorState, 'notifyStart')(this.reactorState, this.observerState)
+
     let observerIdsToNotify = Immutable.Set().withMutations(set => {
       // notify all observers
       set.union(this.observerState.get('any'))
@@ -244,10 +246,13 @@ class Reactor {
         // don't notify here in the case a handler called unobserve on another observer
         return
       }
+      let didCall = false
 
       const getter = entry.get('getter')
       const handler = entry.get('handler')
 
+      fns.getLoggerFunction(this.reactorState, 'notifyEvaluateStart')(this.reactorState, getter)
+
       const prevEvaluateResult = fns.evaluate(this.prevReactorState, getter)
       const currEvaluateResult = fns.evaluate(this.reactorState, getter)
 
@@ -259,13 +264,17 @@ class Reactor {
 
       if (!Immutable.is(prevValue, currValue)) {
         handler.call(null, currValue)
+        didCall = true
       }
+      fns.getLoggerFunction(this.reactorState, 'notifyEvaluateEnd')(this.reactorState, getter, didCall, currValue)
     })
 
     const nextReactorState = fns.resetDirtyStores(this.reactorState)
 
     this.prevReactorState = nextReactorState
     this.reactorState = nextReactorState
+
+    fns.getLoggerFunction(this.reactorState, 'notifyEnd')(this.reactorState, this.observerState)
   }
 
   /**

src/reactor/fns.js

@@ -73,8 +73,6 @@ export function replaceStores(reactorState, stores) {
  * @return {ReactorState}
  */
 export function dispatch(reactorState, actionType, payload) {
-  let logging = reactorState.get('logger')
-
   if (actionType === undefined && getOption(reactorState, 'throwOnUndefinedActionType')) {
     throw new Error('`dispatch` cannot be called with an `undefined` action type.')
   }
@@ -83,7 +81,7 @@ export function dispatch(reactorState, actionType, payload) {
   let dirtyStores = reactorState.get('dirtyStores')
 
   const nextState = currState.withMutations(state => {
-    logging.dispatchStart(reactorState, actionType, payload)
+    getLoggerFunction(reactorState, 'dispatchStart')(reactorState, actionType, payload)
 
     // let each store handle the message
     reactorState.get('stores').forEach((store, id) => {
@@ -94,13 +92,13 @@ export function dispatch(reactorState, actionType, payload) {
         newState = store.handle(currState, actionType, payload)
       } catch(e) {
         // ensure console.group is properly closed
-        logging.dispatchError(reactorState, e.message)
+        getLoggerFunction(reactorState, 'dispatchError')(reactorState, e.message)
         throw e
       }
 
       if (newState === undefined && getOption(reactorState, 'throwOnUndefinedStoreReturnValue')) {
         const errorMsg = 'Store handler must return a value, did you forget a return statement'
-        logging.dispatchError(reactorState, errorMsg)
+        getLoggerFunction(reactorState, 'dispatchError')(reactorState, errorMsg)
         throw new Error(errorMsg)
       }
 
@@ -112,7 +110,7 @@ export function dispatch(reactorState, actionType, payload) {
       }
     })
 
-    logging.dispatchEnd(reactorState, state, dirtyStores, currState)
+    getLoggerFunction(reactorState, 'dispatchEnd')(reactorState, state, dirtyStores, currState)
   })
 
   const nextReactorState = reactorState
@@ -376,6 +374,17 @@ export function resetDirtyStores(reactorState) {
   return reactorState.set('dirtyStores', Immutable.Set())
 }
 
+export function getLoggerFunction(reactorState, fnName) {
+  const logger = reactorState.get('logger')
+  if (!logger) {
+    return noop
+  }
+  const fn = logger[fnName]
+  return (fn)
+    ? fn.bind(logger)
+    : noop
+}
+
 /**
  * @param {ReactorState} reactorState
  * @param {CacheEntry} cacheEntry
@@ -438,3 +447,5 @@ function incrementStoreStates(storeStates, storeIds) {
     })
   })
 }
+
+function noop() {}

src/reactor/records.js

@@ -1,6 +1,5 @@
 import { Map, Set, Record } from 'immutable'
 import { DefaultCache } from './cache'
-import { NoopLogger } from '../logging'
 
 export const PROD_OPTIONS = Map({
   // logs information for each dispatch
@@ -41,7 +40,7 @@ export const ReactorState = Record({
   state: Map(),
   stores: Map(),
   cache: DefaultCache(),
-  logger: NoopLogger,
+  logger: {},
   // maintains a mapping of storeId => state id (monotomically increasing integer whenever store state changes)
   storeStates: Map(),
   dirtyStores: Set(),

tests/reactor-tests.js

@@ -67,8 +67,6 @@ describe('Reactor', () => {
         spyOn(handler, 'dispatchStart')
         spyOn(handler, 'dispatchError')
         spyOn(handler, 'dispatchEnd')
-
-        spyOn(NoopLogger, 'dispatchError')
       })
 
       afterEach(() => {
@@ -110,25 +108,44 @@ describe('Reactor', () => {
           expect(handler.dispatchError).toHaveBeenCalled()
         }
       })
-      it('should use the NoopLogger implementation when a logging function is not defined on the custom implementation', () => {
+      it('should noop when a logging function is not defined on the custom implementation', () => {
         var reactor = new Reactor({
           debug: true,
           logger: {
             dispatchStart() {},
-            dispatchEnd() {},
           },
           options: {
             throwOnUndefinedActionType: false,
           },
         })
 
-        try {
-          reactor.dispatch(undefined)
-        } catch (e) {
-          expect(NoopLogger.dispatchError).toHaveBeenCalled()
-        }
+        expect(() => {
+          reactor.dispatch('setTax', 5)
+        }).not.toThrow()
       })
 
+      it('should properly bind context to the logging function', () => {
+        const loggerSpy = jasmine.createSpy()
+
+        function Logger() {
+        }
+        Logger.prototype.log = function() {
+          loggerSpy()
+        }
+
+        Logger.prototype.dispatchStart = function() {
+          this.log()
+        }
+
+        var reactor = new Reactor({
+          debug: true,
+          logger: new Logger(),
+        })
+
+        reactor.dispatch('setTax', 5)
+
+        expect(loggerSpy).toHaveBeenCalled()
+      })
     })
   })