Skip to content

Commit

Permalink
Add Reactor#batch functionality
Browse files Browse the repository at this point in the history
  • Loading branch information
jordangarcia committed Jul 3, 2015
1 parent f7f13c8 commit de35e19
Show file tree
Hide file tree
Showing 3 changed files with 146 additions and 46 deletions.
2 changes: 1 addition & 1 deletion src/logging.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ exports.dispatchError = function(error) {
exports.storeHandled = function(id, before, after) {
if (console.group) {
if (before !== after) {
console.debug('Core changed: ' + id)
console.debug('Store ' + id + ' handled action')
}
}
}
Expand Down
139 changes: 94 additions & 45 deletions src/reactor.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ class Reactor {
/**
* The state for the whole cluster
*/
this.__state = Immutable.Map({})
this.state = Immutable.Map({})
/**
* Holds a map of id => reactor instance
* Holds a map of id => store instance
*/
this.__stores = Immutable.Map({})

Expand All @@ -46,7 +46,10 @@ class Reactor {
* Change observer interface to observe certain keypaths
* Created after __initialize so it starts with initialState
*/
this.__changeObserver = new ChangeObserver(this.__state, this.__evaluator)
this.__changeObserver = new ChangeObserver(this.state, this.__evaluator)

this.__isBatching = false;
this.__batchDispatchCount = 0;
}

/**
Expand All @@ -55,7 +58,7 @@ class Reactor {
* @return {*}
*/
evaluate(keyPathOrGetter) {
return this.__evaluator.evaluate(this.__state, keyPathOrGetter)
return this.__evaluator.evaluate(this.state, keyPathOrGetter)
}

/**
Expand Down Expand Up @@ -100,43 +103,26 @@ class Reactor {
* @param {object|undefined} payload
*/
dispatch(actionType, payload) {
var debug = this.debug
var prevState = this.__state

this.__state = this.__state.withMutations(state => {
if (this.debug) {
logging.dispatchStart(actionType, payload)
}

// let each core handle the message
this.__stores.forEach((store, id) => {
var currState = state.get(id)
var newState = store.handle(currState, actionType, payload)

if (debug && newState === undefined) {
var error = 'Store handler must return a value, did you forget a return statement'
logging.dispatchError(error)
throw new Error(error)
}

state.set(id, newState)

if (this.debug) {
logging.storeHandled(id, currState, newState)
}
})

if (this.debug) {
logging.dispatchEnd(state)
}
})
var prevState = this.state
this.state = this.__handleAction(prevState, actionType, payload)

// write the new state to the output stream if changed
if (this.__state !== prevState) {
this.__changeObserver.notifyObservers(this.__state)
if (this.__isBatching) {
this.__batchDispatchCount++
} else if (this.state !== prevState) {
this.__notify()
}
}

/**
* Allows batching of dispatches before notifying change observers
* @param {Function} fn
*/
batch(fn) {
this.__batchStart()
fn()
this.__batchEnd()
}

/**
* @deprecated
* @param {String} id
Expand Down Expand Up @@ -169,10 +155,10 @@ class Reactor {
}

this.__stores = this.__stores.set(id, store)
this.__state = this.__state.set(id, initialState)
this.state = this.state.set(id, initialState)
})

this.__changeObserver.notifyObservers(this.__state)
this.__notify()
}

/**
Expand All @@ -182,7 +168,7 @@ class Reactor {
serialize() {
var serialized = {}
this.__stores.forEach((store, id) => {
var storeState = this.__state.get(id)
var storeState = this.state.get(id)
serialized[id] = store.serialize(storeState)
})
return serialized
Expand All @@ -201,18 +187,18 @@ class Reactor {
})
})

this.__state = this.__state.merge(stateToLoad)
this.__changeObserver.notifyObservers(this.__state)
this.state = this.state.merge(stateToLoad)
this.__notify()
}

/**
* Resets the state of a reactor and returns back to initial state
*/
reset() {
var debug = this.debug
var prevState = this.__state
var prevState = this.state

this.__state = Immutable.Map().withMutations(state => {
this.state = Immutable.Map().withMutations(state => {
this.__stores.forEach((store, id) => {
var storeState = prevState.get(id)
var resetStoreState = store.handleReset(storeState)
Expand All @@ -227,7 +213,70 @@ class Reactor {
})

this.__evaluator.reset()
this.__changeObserver.reset(this.__state)
this.__changeObserver.reset(this.state)
}

/**
* Notifies all change observers with the current state
* @private
*/
__notify() {
this.__changeObserver.notifyObservers(this.state)
}


/**
* Reduces the current state to the new state given actionType / message
* @param {string} actionType
* @param {object|undefined} payload
* @return {Immutable.Map}
*/
__handleAction(state, actionType, payload) {
return state.withMutations(state => {
if (this.debug) {
logging.dispatchStart(actionType, payload)
}

// let each core handle the message
this.__stores.forEach((store, id) => {
var currState = state.get(id)
var newState = store.handle(currState, actionType, payload)

if (this.debug && newState === undefined) {
var error = 'Store handler must return a value, did you forget a return statement'
logging.dispatchError(error)
throw new Error(error)
}

state.set(id, newState)

if (this.debug) {
logging.storeHandled(id, currState, newState)
}
})

if (this.debug) {
logging.dispatchEnd(state)
}
})
}

__batchStart() {
if (this.__isBatching) {
throw new Error('Reactor already in batch mode')
}
this.__isBatching = true
}

__batchEnd() {
if (!this.__isBatching) {
throw new Error('Reactor is not in batch mode')
}

if (this.__batchDispatchCount > 0) {
this.__notify()
this.__batchDispatchCount = 0
}
}
}

Expand Down
51 changes: 51 additions & 0 deletions tests/reactor-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -846,4 +846,55 @@ describe('Reactor', () => {
})
})
})

describe('#batch', () => {
var reactor

beforeEach(() => {
reactor = new Reactor({
debug: true,
})
reactor.registerStores({
listStore: Store({
getInitialState() {
return toImmutable([])
},
initialize() {
this.on('add', (state, item) => state.push(toImmutable(item)))
},
}),
})
})

afterEach(() => {
reactor.reset()
})

it('should execute multiple dispatches within the queue function', () => {
reactor.batch(() => {
reactor.dispatch('add', 'one')
reactor.dispatch('add', 'two')
})

expect(reactor.evaluateToJS(['listStore'])).toEqual(['one', 'two'])
})

it('should notify observers only once', () => {
var observeSpy = jasmine.createSpy()

reactor.observe(['listStore'], list => observeSpy(list.toJS()))

reactor.batch(() => {
reactor.dispatch('add', 'one')
reactor.dispatch('add', 'two')
})

expect(observeSpy.calls.count()).toBe(1)

var firstCallArg = observeSpy.calls.argsFor(0)[0]

expect(observeSpy.calls.count()).toBe(1)
expect(firstCallArg).toEqual(['one', 'two'])
})
})
})

3 comments on commit de35e19

@mindjuice
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now __batchStart() throws an error if a batch is already in progress. This means that if you have two actions that use batch(), then you can't compose them into a higher-level batched action.

I haven't run into this as an issue myself yet, but can see if being a problem.

Perhaps instead of a flag, you can keep a __batchCount of batches in progress and only consider notifying when the __batchCount reaches zero in __batchEnd()? (__batchCount is obviously different than __batchDispatchCount).

@jordangarcia
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this came across. I choose not to use a __batchCount and throw an error to keep things simpler, however the more I am thinking about it the more it seems pretty safe to allow nested batches as people wont be manually calling __batchStart() and __batchEnd()

@mindjuice
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's safe, and I think actions should always be composable.

Please sign in to comment.