From e6338d9930a60214a9dad31b6b3ec8472915e6be Mon Sep 17 00:00:00 2001 From: jordangarcia Date: Tue, 20 Sep 2016 11:29:41 -0700 Subject: [PATCH 1/6] Refactor getter caching based on keypath state The current version of NuclearJS uses a cache key consisting of store states (monotomically incresing ID per store). This has the disadvantage of allowing only a single level of depth when figuring out if a cache entry is stale. This leads to poor performance when the shape of a Reactor's state is more deep than wide, ie a store having multiple responsibilities for state tracking. The implementation is as follows: - Consumer can set the maxCacheDepth when instantiating a reactor - Getters are broken down into the canonical set of keypaths based on the maxCacheDepth - Add a keypath tracker abstraction to maintain the state value of all tracked keypaths - After any state change (`Reactor.__notify`) dirty keypaths are resolved and then based on which keypaths have changed any dependent observers are called --- src/getter.js | 43 ++- src/key-path.js | 12 - src/reactor.js | 79 +++-- src/reactor/cache.js | 2 +- src/reactor/fns.js | 307 +++++++++++------- src/reactor/keypath-tracker.js | 179 ++++++++++ src/reactor/records.js | 32 +- tests/getter-tests.js | 85 ++++- tests/keypath-tracker-tests.js | 577 +++++++++++++++++++++++++++++++++ tests/reactor-fns-tests.js | 421 +++++++++++++----------- tests/reactor-tests.js | 139 +++++++- 11 files changed, 1523 insertions(+), 353 deletions(-) create mode 100644 src/reactor/keypath-tracker.js create mode 100644 tests/keypath-tracker-tests.js diff --git a/src/getter.js b/src/getter.js index aece455..f4be2db 100644 --- a/src/getter.js +++ b/src/getter.js @@ -54,7 +54,7 @@ function getFlattenedDeps(getter, existing) { getDeps(getter).forEach(dep => { if (isKeyPath(dep)) { - set.add(List(dep)) + set.add(Immutable.List(dep)) } else if (isGetter(dep)) { set.union(getFlattenedDeps(dep)) } else { @@ -66,6 +66,45 @@ function getFlattenedDeps(getter, existing) { return existing.union(toAdd) } +/** + * Returns a set of deps that have been flattened and expanded + * expanded ex: ['store1', 'key1'] => [['store1'], ['store1', 'key1']] + * + * Note: returns a keypath as an Immutable.List(['store1', 'key1') + * @param {Getter} getter + * @param {Number} maxDepth + * @return {Immutable.Set} + */ +function getCanonicalKeypathDeps(getter, maxDepth) { + if (maxDepth === undefined) { + throw new Error('Must supply maxDepth argument') + } + + const cacheKey = `__storeDeps_${maxDepth}` + if (getter.hasOwnProperty(cacheKey)) { + return getter[cacheKey] + } + + const deps = Immutable.Set().withMutations(set => { + getFlattenedDeps(getter).forEach(keypath => { + if (keypath.size <= maxDepth) { + set.add(keypath) + } else { + set.add(keypath.slice(0, maxDepth)) + } + }) + }) + + Object.defineProperty(getter, cacheKey, { + enumerable: false, + configurable: false, + writable: false, + value: deps, + }) + + return deps +} + /** * @param {KeyPath} * @return {Getter} @@ -88,7 +127,6 @@ function getStoreDeps(getter) { } const storeDeps = getFlattenedDeps(getter) - .map(keyPath => keyPath.first()) .filter(x => !!x) @@ -106,6 +144,7 @@ export default { isGetter, getComputeFn, getFlattenedDeps, + getCanonicalKeypathDeps, getStoreDeps, getDeps, fromKeyPath, diff --git a/src/key-path.js b/src/key-path.js index 6cc3319..f8a677d 100644 --- a/src/key-path.js +++ b/src/key-path.js @@ -13,15 +13,3 @@ export function isKeyPath(toTest) { ) } -/** - * Checks if two keypaths are equal by value - * @param {KeyPath} a - * @param {KeyPath} a - * @return {Boolean} - */ -export function isEqual(a, b) { - const iA = Immutable.List(a) - const iB = Immutable.List(b) - - return Immutable.is(iA, iB) -} diff --git a/src/reactor.js b/src/reactor.js index 4b95a13..e88182f 100644 --- a/src/reactor.js +++ b/src/reactor.js @@ -4,7 +4,7 @@ import * as fns from './reactor/fns' import { DefaultCache } from './reactor/cache' import { ConsoleGroupLogger } from './logging' import { isKeyPath } from './key-path' -import { isGetter } from './getter' +import { isGetter, getCanonicalKeypathDeps } from './getter' import { toJS } from './immutable-helpers' import { extend, toFactory } from './utils' import { @@ -60,7 +60,20 @@ class Reactor { * @return {*} */ evaluate(keyPathOrGetter) { - let { result, reactorState } = fns.evaluate(this.reactorState, keyPathOrGetter) + // look through the keypathStates and see if any of the getters dependencies are dirty, if so resolve + // against the previous reactor state + let updatedReactorState = this.reactorState + if (!isKeyPath(keyPathOrGetter)) { + const maxCacheDepth = fns.getOption(updatedReactorState, 'maxCacheDepth') + let res = fns.resolveDirtyKeypathStates( + this.prevReactorState, + this.reactorState, + getCanonicalKeypathDeps(keyPathOrGetter, maxCacheDepth) + ) + updatedReactorState = res.reactorState + } + + let { result, reactorState } = fns.evaluate(updatedReactorState, keyPathOrGetter) this.reactorState = reactorState return result } @@ -95,10 +108,10 @@ class Reactor { handler = getter getter = [] } - let { observerState, entry } = fns.addObserver(this.observerState, getter, handler) + let { observerState, entry } = fns.addObserver(this.reactorState, this.observerState, getter, handler) this.observerState = observerState return () => { - this.observerState = fns.removeObserverByEntry(this.observerState, entry) + this.observerState = fns.removeObserverByEntry(this.reactorState, this.observerState, entry) } } @@ -110,7 +123,7 @@ class Reactor { throw new Error('Must call unobserve with a Getter') } - this.observerState = fns.removeObserver(this.observerState, getter, handler) + this.observerState = fns.removeObserver(this.reactorState, this.observerState, getter, handler) } /** @@ -130,6 +143,7 @@ class Reactor { } try { + this.prevReactorState = this.reactorState this.reactorState = fns.dispatch(this.reactorState, actionType, payload) } catch (e) { this.__isDispatching = false @@ -171,6 +185,7 @@ class Reactor { * @param {Object} stores */ registerStores(stores) { + this.prevReactorState = this.reactorState this.reactorState = fns.registerStores(this.reactorState, stores) this.__notify() } @@ -196,6 +211,7 @@ class Reactor { * @param {Object} state */ loadState(state) { + this.prevReactorState = this.reactorState this.reactorState = fns.loadState(this.reactorState, state) this.__notify() } @@ -210,6 +226,15 @@ class Reactor { this.observerState = new ObserverState() } + /** + * Denotes a new state, via a store registration, dispatch or some other method + * Resolves any outstanding keypath states and sets a new reactorState + * @private + */ + __nextState(newState) { + // TODO(jordan): determine if this is actually needed + } + /** * Notifies all change observers with the current state * @private @@ -220,36 +245,36 @@ class Reactor { return } - const dirtyStores = this.reactorState.get('dirtyStores') - if (dirtyStores.size === 0) { - 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')) + const keypathsToResolve = this.observerState.get('trackedKeypaths') + const { reactorState, changedKeypaths } = fns.resolveDirtyKeypathStates( + this.prevReactorState, + this.reactorState, + keypathsToResolve, + true // increment all dirty states (this should leave no unknown state in the keypath tracker map): + ) + this.reactorState = reactorState - dirtyStores.forEach(id => { - const entries = this.observerState.getIn(['stores', id]) - if (!entries) { - return + // get observers to notify based on the keypaths that changed + let observersToNotify = Immutable.Set().withMutations(set => { + changedKeypaths.forEach(keypath => { + const entries = this.observerState.getIn(['keypathToEntries', keypath]) + if (entries && entries.size > 0) { + set.union(entries) } - set.union(entries) }) }) - observerIdsToNotify.forEach((observerId) => { - const entry = this.observerState.getIn(['observersMap', observerId]) - if (!entry) { - // don't notify here in the case a handler called unobserve on another observer + observersToNotify.forEach((observer) => { + if (!this.observerState.get('observers').has(observer)) { + // the observer was removed in a hander function return } let didCall = false - const getter = entry.get('getter') - const handler = entry.get('handler') + const getter = observer.get('getter') + const handler = observer.get('handler') fns.getLoggerFunction(this.reactorState, 'notifyEvaluateStart')(this.reactorState, getter) @@ -262,6 +287,7 @@ class Reactor { const prevValue = prevEvaluateResult.result const currValue = currEvaluateResult.result + // TODO pull some comparator function out of the reactorState if (!Immutable.is(prevValue, currValue)) { handler.call(null, currValue) didCall = true @@ -269,11 +295,6 @@ class Reactor { 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) } diff --git a/src/reactor/cache.js b/src/reactor/cache.js index d202631..18978fe 100644 --- a/src/reactor/cache.js +++ b/src/reactor/cache.js @@ -2,7 +2,7 @@ import { Map, OrderedSet, Record } from 'immutable' export const CacheEntry = Record({ value: null, - storeStates: Map(), + states: Map(), dispatchId: null, }) diff --git a/src/reactor/fns.js b/src/reactor/fns.js index 27733ef..27afc83 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -2,8 +2,9 @@ import Immutable from 'immutable' import { CacheEntry } from './cache' import { isImmutableValue } from '../immutable-helpers' import { toImmutable } from '../immutable-helpers' -import { fromKeyPath, getStoreDeps, getComputeFn, getDeps, isGetter } from '../getter' +import { fromKeyPath, getStoreDeps, getComputeFn, getDeps, isGetter, getCanonicalKeypathDeps } from '../getter' import { isEqual, isKeyPath } from '../key-path' +import * as KeypathTracker from './keypath-tracker' import { each } from '../utils' /** @@ -44,8 +45,9 @@ export function registerStores(reactorState, stores) { reactorState .update('stores', stores => stores.set(id, store)) .update('state', state => state.set(id, initialState)) - .update('dirtyStores', state => state.add(id)) - .update('storeStates', storeStates => incrementStoreStates(storeStates, [id])) + .update('keypathStates', keypathStates => { + return KeypathTracker.changed(keypathStates, [id]) + }) }) incrementId(reactorState) }) @@ -78,7 +80,7 @@ export function dispatch(reactorState, actionType, payload) { } const currState = reactorState.get('state') - let dirtyStores = reactorState.get('dirtyStores') + let dirtyStores = [] const nextState = currState.withMutations(state => { getLoggerFunction(reactorState, 'dispatchStart')(reactorState, actionType, payload) @@ -106,17 +108,20 @@ export function dispatch(reactorState, actionType, payload) { if (currState !== newState) { // if the store state changed add store to list of dirty stores - dirtyStores = dirtyStores.add(id) + dirtyStores.push(id) } }) - getLoggerFunction(reactorState, 'dispatchEnd')(reactorState, state, dirtyStores, currState) + getLoggerFunction(reactorState, 'dispatchEnd')(reactorState, state, toImmutable(dirtyStores), currState) }) const nextReactorState = reactorState .set('state', nextState) - .set('dirtyStores', dirtyStores) - .update('storeStates', storeStates => incrementStoreStates(storeStates, dirtyStores)) + .update('keypathStates', k => k.withMutations(keypathStates => { + dirtyStores.forEach(storeId => { + KeypathTracker.changed(keypathStates, [storeId]) + }) + })) return incrementId(nextReactorState) } @@ -144,8 +149,11 @@ export function loadState(reactorState, state) { const dirtyStoresSet = Immutable.Set(dirtyStores) return reactorState .update('state', state => state.merge(stateToLoad)) - .update('dirtyStores', stores => stores.union(dirtyStoresSet)) - .update('storeStates', storeStates => incrementStoreStates(storeStates, dirtyStores)) + .update('keypathStates', k => k.withMutations(keypathStates => { + dirtyStoresSet.forEach(storeId => { + KeypathTracker.changed(keypathStates, [storeId]) + }) + })) } /** @@ -160,50 +168,45 @@ export function loadState(reactorState, state) { * If only one argument is passed invoked the handler whenever * the reactor state changes * + * @param {ReactorState} reactorState * @param {ObserverState} observerState * @param {KeyPath|Getter} getter * @param {function} handler * @return {ObserveResult} */ -export function addObserver(observerState, getter, handler) { +export function addObserver(reactorState, observerState, getter, handler) { // use the passed in getter as the key so we can rely on a byreference call for unobserve - const getterKey = getter + const rawGetter = getter if (isKeyPath(getter)) { getter = fromKeyPath(getter) } - const currId = observerState.get('nextId') - const storeDeps = getStoreDeps(getter) + const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') + const keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) const entry = Immutable.Map({ - id: currId, - storeDeps: storeDeps, - getterKey: getterKey, getter: getter, handler: handler, }) - let updatedObserverState - if (storeDeps.size === 0) { - // no storeDeps means the observer is dependent on any of the state changing - updatedObserverState = observerState.update('any', observerIds => observerIds.add(currId)) - } else { - updatedObserverState = observerState.withMutations(map => { - storeDeps.forEach(storeId => { - let path = ['stores', storeId] - if (!map.hasIn(path)) { - map.setIn(path, Immutable.Set()) - } - map.updateIn(['stores', storeId], observerIds => observerIds.add(currId)) + let updatedObserverState = observerState.withMutations(map => { + keypathDeps.forEach(keypath => { + map.updateIn(['keypathToEntries', keypath], entries => { + return (entries) + ? entries.add(entry) + : Immutable.Set().add(entry) }) }) - } + }) + + const getterKey = createGetterKey(getter); - updatedObserverState = updatedObserverState - .set('nextId', currId + 1) - .setIn(['observersMap', currId], entry) + const finalObserverState = updatedObserverState + .update('trackedKeypaths', keypaths => keypaths.union(keypathDeps)) + .setIn(['observersMap', getterKey, handler], entry) + .update('observers', observers => observers.add(entry)) return { - observerState: updatedObserverState, + observerState: finalObserverState, entry: entry, } } @@ -234,24 +237,25 @@ export function getOption(reactorState, option) { * @param {Function} handler * @return {ObserverState} */ -export function removeObserver(observerState, getter, handler) { - const entriesToRemove = observerState.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 - } - // 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) - }) +export function removeObserver(reactorState, observerState, getter, handler) { + if (isKeyPath(getter)) { + getter = fromKeyPath(getter) + } + let entriesToRemove; + const getterKey = createGetterKey(getter) + const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') + const keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) + + if (handler) { + entriesToRemove = Immutable.List([ + observerState.getIn(['observersMap', getterKey, handler]), + ]) + } else { + entriesToRemove = observerState.getIn(['observersMap', getterKey], Immutable.Map({})).toList() + } return observerState.withMutations(map => { - entriesToRemove.forEach(entry => removeObserverByEntry(map, entry)) + entriesToRemove.forEach(entry => removeObserverByEntry(reactorState, map, entry, keypathDeps)) }) } @@ -261,26 +265,42 @@ export function removeObserver(observerState, getter, handler) { * @param {Immutable.Map} entry * @return {ObserverState} */ -export function removeObserverByEntry(observerState, entry) { +export function removeObserverByEntry(reactorState, observerState, entry, keypathDeps = null) { return observerState.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 => { - if (observers) { - // check for observers being present because reactor.reset() can be called before an unwatch fn - return observers.remove(id) - } - return observers - }) - }) + const getter = entry.get('getter') + if (!keypathDeps) { + const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') + keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) } - map.removeIn(['observersMap', id]) + map.update('observers', observers => observers.remove(entry)) + + // update the keypathToEntries + keypathDeps.forEach(keypath => { + const kp = ['keypathToEntries', keypath] + map.updateIn(kp, entries => { + // check for observers being present because reactor.reset() can be called before an unwatch fn + return (entries) + ? entries.remove(entry) + : entries + }) + // protect against unwatch after reset + if (map.hasIn(kp) && + map.getIn(kp).size === 0) { + map.removeIn(kp) + map.update('trackedKeypaths', keypaths => keypaths.remove(keypath)) + } + }) + + // remove entry from observersMap + const getterKey = createGetterKey(getter) + const handler = entry.get('handler') + map.removeIn(['observersMap', getterKey, handler]) + // protect against unwatch after reset + if (map.hasIn(['observersMap', getterKey]) && + map.getIn(['observersMap', getterKey]).size === 0) { + map.removeIn(['observersMap', getterKey]) + } }) } @@ -306,11 +326,57 @@ export function reset(reactorState) { reactorState.setIn(['state', id], resetStoreState) }) - reactorState.update('storeStates', storeStates => incrementStoreStates(storeStates, storeIds)) - resetDirtyStores(reactorState) + reactorState.update('keypathStates', k => k.withMutations(keypathStates => { + storeIds.forEach(id => { + KeypathTracker.changed(keypathStates, [id]) + }) + })) }) } +/** + * @param {ReactorState} prevReactorState + * @param {ReactorState} currReactorState + * @param {Array} keyPathOrGetter + * @return {Object} + */ +export function resolveDirtyKeypathStates(prevReactorState, currReactorState, keypaths, cleanAll = false) { + const prevState = prevReactorState.get('state') + const currState = currReactorState.get('state') + + // TODO(jordan): allow store define a comparator function + function equals(a, b) { + return Immutable.is(a, b) + } + + let changedKeypaths = []; + + const newReactorState = currReactorState.update('keypathStates', k => k.withMutations(keypathStates => { + keypaths.forEach(keypath => { + if (KeypathTracker.isClean(keypathStates, keypath)) { + return + } + + if (equals(prevState.getIn(keypath), currState.getIn(keypath))) { + KeypathTracker.unchanged(keypathStates, keypath) + } else { + KeypathTracker.changed(keypathStates, keypath) + changedKeypaths.push(keypath) + } + }) + + if (cleanAll) { + // TODO(jordan): this can probably be a single traversal + KeypathTracker.incrementAndClean(keypathStates) + } + })) + + return { + changedKeypaths, + reactorState: newReactorState, + } +} + /** * @param {ReactorState} reactorState * @param {KeyPath|Gettter} keyPathOrGetter @@ -328,22 +394,23 @@ export function evaluate(reactorState, keyPathOrGetter) { } else if (!isGetter(keyPathOrGetter)) { throw new Error('evaluate must be passed a keyPath or Getter') } - // Must be a Getter const cache = reactorState.get('cache') - var cacheEntry = cache.lookup(keyPathOrGetter) + let cacheEntry = cache.lookup(keyPathOrGetter) const isCacheMiss = !cacheEntry || isDirtyCacheEntry(reactorState, cacheEntry) if (isCacheMiss) { - cacheEntry = createCacheEntry(reactorState, keyPathOrGetter) + const cacheResult = createCacheEntry(reactorState, keyPathOrGetter) + cacheEntry = cacheResult.entry + reactorState = cacheResult.reactorState } return evaluateResult( cacheEntry.get('value'), reactorState.update('cache', cache => { - return isCacheMiss ? - cache.miss(keyPathOrGetter, cacheEntry) : - cache.hit(keyPathOrGetter) + return isCacheMiss + ? cache.miss(keyPathOrGetter, cacheEntry) + : cache.hit(keyPathOrGetter) }) ) } @@ -365,15 +432,6 @@ export function serialize(reactorState) { return serialized } -/** - * Returns serialized state for all stores - * @param {ReactorState} reactorState - * @return {ReactorState} - */ -export function resetDirtyStores(reactorState) { - return reactorState.set('dirtyStores', Immutable.Set()) -} - export function getLoggerFunction(reactorState, fnName) { const logger = reactorState.get('logger') if (!logger) { @@ -391,14 +449,27 @@ export function getLoggerFunction(reactorState, fnName) { * @return {boolean} */ function isDirtyCacheEntry(reactorState, cacheEntry) { - const storeStates = cacheEntry.get('storeStates') + if (reactorState.get('dispatchId') === cacheEntry.get('dispatchId')) { + return false + } + + const cacheStates = cacheEntry.get('states') + const keypathStates = reactorState.get('keypathStates') - // if there are no store states for this entry then it was never cached before - return !storeStates.size || storeStates.some((stateId, storeId) => { - return reactorState.getIn(['storeStates', storeId]) !== stateId + return cacheEntry.get('states').some((value, keypath) => { + return !KeypathTracker.isEqual(keypathStates, keypath, value) }) } +/** + * Creates an immutable key for a getter + * @param {Getter} getter + * @return {Immutable.List} + */ +function createGetterKey(getter) { + return toImmutable(getter) +} + /** * Evaluates getter for given reactorState and returns CacheEntry * @param {ReactorState} reactorState @@ -407,22 +478,45 @@ function isDirtyCacheEntry(reactorState, cacheEntry) { */ function createCacheEntry(reactorState, getter) { // evaluate dependencies - const args = getDeps(getter).map(dep => evaluate(reactorState, dep).result) + const initial = { + reactorState: reactorState, + args: [], + } + // reduce here and capture updates to the ReactorState + const argResults = getDeps(getter).reduce((memo, dep) => { + const evaluateResult = evaluate(memo.reactorState, dep) + return { + reactorState: evaluateResult.get('reactorState'), + args: memo.args.concat(evaluateResult.get('result')), + } + }, initial) + const args = argResults.args + const newReactorState = argResults.reactorState + const value = getComputeFn(getter).apply(null, args) - const storeDeps = getStoreDeps(getter) - const storeStates = toImmutable({}).withMutations(map => { - storeDeps.forEach(storeId => { - const stateId = reactorState.getIn(['storeStates', storeId]) - map.set(storeId, stateId) + const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') + const keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) + const keypathStates = reactorState.get('keypathStates') + + const cacheStates = toImmutable({}).withMutations(map => { + keypathDeps.forEach(keypath => { + const keypathState = KeypathTracker.get(keypathStates, keypath) + // The -1 case happens when evaluating soemthing against a previous reactorState + // where the getter's keypaths were never registered and the old keypathState is undefined + // for particular keypaths, this shouldn't matter because we can cache hit by dispatchId + map.set(keypath, keypathState ? keypathState : -1) }) }) - return CacheEntry({ - value: value, - storeStates: storeStates, - dispatchId: reactorState.get('dispatchId'), - }) + return { + reactorState: newReactorState, + entry: CacheEntry({ + value: value, + states: cacheStates, + dispatchId: reactorState.get('dispatchId'), + }) + } } /** @@ -433,19 +527,4 @@ function incrementId(reactorState) { return reactorState.update('dispatchId', id => id + 1) } - -/** - * @param {Immutable.Map} storeStates - * @param {Array} storeIds - * @return {Immutable.Map} - */ -function incrementStoreStates(storeStates, storeIds) { - return storeStates.withMutations(map => { - storeIds.forEach(id => { - const nextId = map.has(id) ? map.get(id) + 1 : 1 - map.set(id, nextId) - }) - }) -} - function noop() {} diff --git a/src/reactor/keypath-tracker.js b/src/reactor/keypath-tracker.js new file mode 100644 index 0000000..c966201 --- /dev/null +++ b/src/reactor/keypath-tracker.js @@ -0,0 +1,179 @@ +/** + * KeyPath Tracker + * + * St + * { + * entityCache: { + * status: 'CLEAN', + * k + * + */ +import { Map, Record } from 'immutable' +import { toImmutable, toJS } from '../immutable-helpers' + +export const status = { + CLEAN: 0, + DIRTY: 1, + UNKNOWN: 2, +} + +export const Node = Record({ + status: status.CLEAN, + state: 1, + children: Map(), +}) + +export function unchanged(map, keypath) { + const childKeypath = getChildKeypath(keypath) + if (!map.hasIn(childKeypath)) { + return map.update('children', children => recursiveRegister(children, keypath)) + } + + return map.updateIn(childKeypath, entry => { + return entry + .set('status', status.CLEAN) + .update('children', children => recursiveSetStatus(children, status.CLEAN)) + }) +} + +export function changed(map, keypath) { + const childrenKeypath = getChildKeypath(keypath).concat('children') + return map + .update('children', children => recursiveIncrement(children, keypath)) + // handle the root node + .update('state', val => val + 1) + .set('status', status.DIRTY) + .updateIn(childrenKeypath, entry => recursiveSetStatus(entry, status.UNKNOWN)) +} + +/** + * @param {Immutable.Map} map + * @param {Keypath} keypath + * @return {Status} + */ +export function isEqual(map, keypath, value) { + const entry = map.getIn(getChildKeypath(keypath)) + + if (!entry) { + return false; + } + if (entry.get('status') === status.UNKNOWN) { + return false + } + return entry.get('state') === value; +} + +/** + * Increments all unknown states and sets everything to CLEAN + * @param {Immutable.Map} map + * @return {Status} + */ +export function incrementAndClean(map) { + if (map.size === 0) { + return map + } + + const rootStatus = map.get('status') + if (rootStatus === status.DIRTY) { + map = setClean(map) + } else if (rootStatus === status.UNKNOWN) { + map = setClean(increment(map)) + } + return map + .update('children', c => c.withMutations(m => { + m.keySeq().forEach(key => { + m.update(key, incrementAndClean) + }) + })) +} + +export function get(map, keypath) { + return map.getIn(getChildKeypath(keypath).concat('state')) +} + +export function isClean(map, keypath) { + return map.getIn(getChildKeypath(keypath).concat('status')) === status.CLEAN +} + +function increment(node) { + return node.update('state', val => val + 1) +} + +function setClean(node) { + return node.set('status', status.CLEAN) +} + +function setDirty(node) { + return node.set('status', status.DIRTY) +} + +function recursiveIncrement(map, keypath) { + keypath = toImmutable(keypath) + if (keypath.size === 0) { + return map + } + + return map.withMutations(map => { + const key = keypath.first() + const entry = map.get(key) + + if (!entry) { + map.set(key, new Node({ + status: status.DIRTY, + })) + } else { + map.update(key, node => setDirty(increment(node))) + } + + map.updateIn([key, 'children'], children => recursiveIncrement(children, keypath.rest())) + }) +} + +function recursiveRegister(map, keypath) { + keypath = toImmutable(keypath) + if (keypath.size === 0) { + return map + } + + return map.withMutations(map => { + const key = keypath.first() + const entry = map.get(key) + + if (!entry) { + map.set(key, new Node()) + } + map.updateIn([key, 'children'], children => recursiveRegister(children, keypath.rest())) + }) +} + +/** + * Turns ['foo', 'bar', 'baz'] -> ['foo', 'children', 'bar', 'children', 'baz'] + * @param {Keypath} keypath + * @return {Keypath} + */ +function getChildKeypath(keypath) { + // TODO(jordan): handle toJS more elegantly + keypath = toJS(keypath) + let ret = [] + for (var i = 0; i < keypath.length; i++) { + ret.push('children') + ret.push(keypath[i]) + } + return ret +} + +function recursiveSetStatus(map, status) { + if (map.size === 0) { + return map + } + + return map.withMutations(map => { + map.keySeq().forEach(key => { + return map.update(key, entry => { + return entry + .update('children', children => recursiveSetStatus(children, status)) + .set('status', status) + }) + }) + }) +} diff --git a/src/reactor/records.js b/src/reactor/records.js index 642f8fc..4c563ff 100644 --- a/src/reactor/records.js +++ b/src/reactor/records.js @@ -1,5 +1,6 @@ import { Map, Set, Record } from 'immutable' import { DefaultCache } from './cache' +import { Node as KeypathTrackerNode } from './keypath-tracker' export const PROD_OPTIONS = Map({ // logs information for each dispatch @@ -16,6 +17,8 @@ export const PROD_OPTIONS = Map({ throwOnNonImmutableStore: false, // if true, throws when dispatching in dispatch throwOnDispatchInDispatch: false, + // how many levels deep should getter keypath dirty states be tracked + maxCacheDepth: 3, }) export const DEBUG_OPTIONS = Map({ @@ -33,6 +36,8 @@ export const DEBUG_OPTIONS = Map({ throwOnNonImmutableStore: true, // if true, throws when dispatching in dispatch throwOnDispatchInDispatch: true, + // how many levels deep should getter keypath dirty states be tracked + maxCacheDepth: 3, }) export const ReactorState = Record({ @@ -41,21 +46,32 @@ export const ReactorState = Record({ stores: Map(), cache: DefaultCache(), logger: {}, - // maintains a mapping of storeId => state id (monotomically increasing integer whenever store state changes) - storeStates: Map(), - dirtyStores: Set(), + keypathStates: new KeypathTrackerNode(), debug: false, // production defaults options: PROD_OPTIONS, }) export const ObserverState = Record({ - // observers registered to any store change - any: Set(), - // observers registered to specific store changes - stores: Map({}), + /* + { + : Set + } + */ + keypathToEntries: Map({}), + /* + { + : { + : + } + } + */ observersMap: Map({}), - nextId: 1, + trackedKeypaths: Set(), + + // keep a flat set of observers to know when one is removed during a handler + observers: Set(), }) + diff --git a/tests/getter-tests.js b/tests/getter-tests.js index 93d89f6..8eeb328 100644 --- a/tests/getter-tests.js +++ b/tests/getter-tests.js @@ -1,4 +1,4 @@ -import { isGetter, getFlattenedDeps, fromKeyPath } from '../src/getter' +import { isGetter, getFlattenedDeps, fromKeyPath, getCanonicalKeypathDeps } from '../src/getter' import { Set, List, is } from 'immutable' describe('Getter', () => { @@ -78,4 +78,87 @@ describe('Getter', () => { }) }) }) + + describe('getCanonicalKeypathDeps', function() { + describe('when passed the identity getter', () => { + it('should return a set with only an empty list', () => { + var getter = [[], (x) => x] + var result = getCanonicalKeypathDeps(getter, 3) + var expected = Set().add(List()) + expect(is(result, expected)).toBe(true) + }) + }) + + describe('when passed a flat getter with maxDepth greater than each keypath' , () => { + it('return all keypaths', () => { + var getter = [ + ['store1', 'key1'], + ['store2', 'key2'], + (a, b) => 1, + ] + var result = getCanonicalKeypathDeps(getter, 3) + var expected = Set() + .add(List(['store1', 'key1'])) + .add(List(['store2', 'key2'])) + expect(is(result, expected)).toBe(true) + }) + }) + + describe('when passed a flat getter with maxDepth less than each keypath' , () => { + it('return all shortened keypaths', () => { + var getter = [ + ['store1', 'key1', 'prop1', 'bar1'], + ['store2', 'key2'], + (a, b) => 1, + ] + var result = getCanonicalKeypathDeps(getter, 3) + var expected = Set() + .add(List(['store1', 'key1', 'prop1'])) + .add(List(['store2', 'key2'])) + expect(is(result, expected)).toBe(true) + }) + }) + + describe('when passed getter with a getter dependency', () => { + it('should return flattened keypaths', () => { + var getter1 = [ + ['store1', 'key1'], + ['store2', 'key2'], + (a, b) => 1, + ] + var getter2 = [ + getter1, + ['store3', 'key3'], + (a, b) => 1, + ] + var result = getFlattenedDeps(getter2) + var expected = Set() + .add(List(['store1', 'key1'])) + .add(List(['store2', 'key2'])) + .add(List(['store3', 'key3'])) + expect(is(result, expected)).toBe(true) + }) + }) + + describe('when passed getter with a getter dependency with long keypaths', () => { + it('should return flattened and shortened keypaths', () => { + var getter1 = [ + ['store1', 'key1', 'bar', 'baz'], + ['store2', 'key2'], + (a, b) => 1, + ] + var getter2 = [ + getter1, + ['store3', 'key3'], + (a, b) => 1, + ] + var result = getCanonicalKeypathDeps(getter2, 3) + var expected = Set() + .add(List(['store1', 'key1', 'bar'])) + .add(List(['store2', 'key2'])) + .add(List(['store3', 'key3'])) + expect(is(result, expected)).toBe(true) + }) + }) + }) }) diff --git a/tests/keypath-tracker-tests.js b/tests/keypath-tracker-tests.js new file mode 100644 index 0000000..38510a1 --- /dev/null +++ b/tests/keypath-tracker-tests.js @@ -0,0 +1,577 @@ +/*eslint-disable one-var, comma-dangle*/ +import { Map, Set, is } from 'immutable' +import * as KeypathTracker from '../src/reactor/keypath-tracker' +import { toImmutable } from '../src/immutable-helpers' + +const { status, Node } = KeypathTracker + +describe('Keypath Tracker', () => { + describe('unchanged', () => { + it('should properly register ["foo"]', () => { + const keypath = ['foo'] + const state = new Node() + let tracker = KeypathTracker.unchanged(state, keypath) + + const expected = new Node({ + status: status.CLEAN, + state: 1, + children: toImmutable({ + foo: { + status: status.CLEAN, + state: 1, + children: {}, + }, + }) + }) + expect(is(tracker, expected)).toBe(true) + }) + + it('should properly register ["foo", "bar"]', () => { + const keypath = ['foo', 'bar'] + const state = new Node() + let tracker = KeypathTracker.unchanged(state, keypath) + + const expected = new Node({ + status: status.CLEAN, + state: 1, + children: toImmutable({ + foo: { + status: status.CLEAN, + state: 1, + children: { + bar: { + status: status.CLEAN, + state: 1, + children: {}, + }, + } + }, + }) + }) + + expect(is(tracker, expected)).toBe(true) + }) + + it('should register ["foo", "bar"] when ["foo"] is already registered', () => { + const keypath = ['foo', 'bar'] + const origTracker = new Node({ + status: status.CLEAN, + state: 1, + children: toImmutable({ + foo: { + status: status.UNKNOWN, + state: 2, + children: {}, + }, + }) + }) + const tracker = KeypathTracker.unchanged(origTracker, keypath) + const expected = new Node({ + status: status.CLEAN, + state: 1, + children: toImmutable({ + foo: { + status: status.UNKNOWN, + state: 2, + children: { + bar: { + status: status.CLEAN, + state: 1, + children: {}, + }, + } + }, + }) + }) + + expect(is(tracker, expected)).toBe(true) + }) + + it('should mark something as unchanged', () => { + const keypath = ['foo', 'bar'] + const orig = new Node({ + status: status.CLEAN, + state: 1, + children: toImmutable({ + foo: { + status: status.DIRTY, + state: 2, + children: { + bar: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + } + }, + }), + }) + const tracker = KeypathTracker.unchanged(orig, keypath) + const expected = new Node({ + status: status.CLEAN, + state: 1, + children: toImmutable({ + foo: { + status: status.DIRTY, + state: 2, + children: { + bar: { + status: status.CLEAN, + state: 1, + children: {}, + }, + } + }, + }), + }) + + expect(is(tracker, expected)).toBe(true) + }) + + it('should mark the root node as unchanged', () => { + const orig = new Node({ + status: status.UNKNOWN, + state: 1, + children: toImmutable({ + foo: { + status: status.UNKNOWN, + state: 2, + children: {} + }, + }), + }) + const tracker = KeypathTracker.unchanged(orig, []) + const expected = new Node({ + status: status.CLEAN, + state: 1, + children: toImmutable({ + foo: { + status: status.CLEAN, + state: 2, + children: {}, + }, + }), + }) + + expect(is(tracker, expected)).toBe(true) + }) + }) + + + describe('changed', () => { + it('should initialize a node with a DIRTY status', () => { + const orig = new Node({ + status: status.CLEAN, + state: 1, + }) + const result = KeypathTracker.changed(orig, ['foo']) + const expected = new Node({ + status: status.DIRTY, + state: 2, + children: toImmutable({ + foo: { + status: status.DIRTY, + state: 1, + children: {}, + }, + }), + }) + + expect(is(result, expected)).toBe(true) + }) + it('should traverse and increment for parents and mark children clean', () => { + const orig = new Node({ + status: status.CLEAN, + state: 1, + children: toImmutable({ + foo: { + state: 1, + children: { + bar: { + status: status.CLEAN, + state: 1, + children: { + baz: { + status: status.CLEAN, + state: 1, + children: {}, + }, + bat: { + status: status.CLEAN, + state: 1, + children: {}, + }, + }, + }, + }, + }, + }), + }) + const result = KeypathTracker.changed(orig, ['foo', 'bar']) + const expected = new Node({ + status: status.DIRTY, + state: 2, + children: toImmutable({ + foo: { + status: status.DIRTY, + state: 2, + children: { + bar: { + status: status.DIRTY, + state: 2, + children: { + baz: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + bat: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + }, + }, + }, + }, + }), + }) + + expect(is(result, expected)).toBe(true) + }) + + it('should handle the root node', () => { + const orig = new Node({ + status: status.CLEAN, + state: 1, + children: toImmutable({ + foo: { + status: status.UNKNOWN, + state: 1, + children: { + bar: { + status: status.CLEAN, + state: 1, + children: { + baz: { + status: status.CLEAN, + state: 1, + children: {}, + }, + bat: { + status: status.CLEAN, + state: 1, + children: {}, + }, + }, + }, + }, + }, + }), + }) + const result = KeypathTracker.changed(orig, []) + const expected = new Node({ + status: status.DIRTY, + state: 2, + children: toImmutable({ + foo: { + status: status.UNKNOWN, + state: 1, + children: { + bar: { + status: status.UNKNOWN, + state: 1, + children: { + baz: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + bat: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + }, + }, + }, + }, + }), + }) + + expect(is(result, expected)).toBe(true) + }) + }) + + describe('isEqual', () => { + const state = new Node({ + state: 1, + status: status.DIRTY, + children: toImmutable({ + foo: { + status: status.DIRTY, + state: 2, + children: { + bar: { + status: status.DIRTY, + state: 2, + children: { + baz: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + bat: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + }, + }, + }, + }, + }) + }) + + it('should return false for a mismatch on the root node', () => { + const result = KeypathTracker.isEqual(state, [], 2) + expect(result).toBe(false) + }) + + it('should return false with an invalid keypath', () => { + const result = KeypathTracker.isEqual(state, ['foo', 'wat'], 2) + expect(result).toBe(false) + }) + + it('should return false when values dont match', () => { + const result = KeypathTracker.isEqual(state, ['foo', 'bar'], 1) + expect(result).toBe(false) + }) + + it('should return false when node is unknown', () => { + const result = KeypathTracker.isEqual(state, ['foo', 'bar', 'baz'], 1) + expect(result).toBe(false) + }) + + it('should return true when values match and node is clean', () => { + const result = KeypathTracker.isEqual(state, ['foo', 'bar'], 2) + expect(result).toBe(true) + }) + }) + + describe('get', () => { + const state = new Node({ + state: 1, + status: status.DIRTY, + children: toImmutable({ + foo: { + status: status.DIRTY, + state: 2, + children: { + bar: { + status: status.DIRTY, + state: 2, + children: { + baz: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + bat: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + }, + }, + }, + }, + }) + }) + + it('should return undefined with an invalid keypath', () => { + const result = KeypathTracker.get(state, ['foo', 'wat']) + expect(result).toBe(undefined) + }) + + it('should return a value for a single depth', () => { + const result = KeypathTracker.get(state, ['foo']) + expect(result).toBe(2) + }) + + it('should return a value for a deeper keypath', () => { + const result = KeypathTracker.get(state, ['foo', 'bar', 'baz']) + expect(result).toBe(1) + }) + }) + + describe('isClean', () => { + const state = new Node({ + state: 1, + status: status.DIRTY, + children: toImmutable({ + foo: { + status: status.DIRTY, + state: 2, + children: { + bar: { + status: status.DIRTY, + state: 2, + children: { + baz: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + bat: { + status: status.CLEAN, + state: 1, + children: {}, + }, + }, + }, + }, + }, + }) + }) + + it('should return false with an invalid keypath', () => { + const result = KeypathTracker.isClean(state, ['foo', 'wat']) + expect(result).toBe(false) + }) + + it('should return false for a DIRTY value', () => { + const result = KeypathTracker.isClean(state, ['foo']) + expect(result).toBe(false) + }) + + it('should return false for an UNKNOWN value', () => { + const result = KeypathTracker.isClean(state, ['foo', 'bar', 'baz']) + expect(result).toBe(false) + }) + + it('should return true for an CLEAN value', () => { + const result = KeypathTracker.isClean(state, ['foo', 'bar', 'bat']) + expect(result).toBe(true) + }) + }) + + describe('incrementAndClean', () => { + it('should work when the root node is clean', () => { + const state = new Node({ + state: 2, + status: status.CLEAN, + chidlren: toImmutable({ + foo: { + status: status.DIRTY, + state: 2, + children: { + bar: { + status: status.UNKNOWN, + state: 1, + children: {} + }, + }, + }, + }), + }) + + const expected = new Node({ + state: 2, + status: status.CLEAN, + chidlren: toImmutable({ + foo: { + status: status.CLEAN, + state: 2, + children: { + bar: { + status: status.CLEAN, + state: 2, + children: {} + }, + }, + }, + }), + }) + + + const result = KeypathTracker.incrementAndClean(state) + expect(is(result, expected)).toBe(true) + }) + it('should traverse the tree and increment any state value thats UNKNOWN and mark everything CLEAN', () => { + const state = new Node({ + state: 2, + status: status.DIRTY, + chidlren: toImmutable({ + foo: { + status: status.DIRTY, + state: 2, + children: { + bar: { + status: status.UNKNOWN, + state: 1, + children: { + baz: { + status: status.UNKNOWN, + state: 1, + children: {}, + }, + bat: { + status: status.DIRTY, + state: 2, + children: {}, + }, + }, + }, + }, + }, + top: { + status: status.CLEAN, + state: 1, + children: {}, + }, + }), + }) + + const expected = new Node({ + state: 2, + status: status.CLEAN, + chidlren: toImmutable({ + foo: { + status: status.CLEAN, + state: 2, + children: { + bar: { + status: status.CLEAN, + state: 2, + children: { + baz: { + status: status.UNKNOWN, + state: 2, + children: {}, + }, + bat: { + status: status.CLEAN, + state: 2, + children: {}, + }, + }, + }, + }, + }, + top: { + status: status.CLEAN, + state: 1, + children: {}, + }, + }), + }) + + + const result = KeypathTracker.incrementAndClean(state) + expect(is(result, expected)).toBe(true) + }) + }) +}) +/*eslint-enable one-var, comma-dangle*/ + diff --git a/tests/reactor-fns-tests.js b/tests/reactor-fns-tests.js index ce73e10..33a22a9 100644 --- a/tests/reactor-fns-tests.js +++ b/tests/reactor-fns-tests.js @@ -1,10 +1,13 @@ /*eslint-disable one-var, comma-dangle*/ -import { Map, Set, is } from 'immutable' +import { Map, Set, OrderedSet, List, is } from 'immutable' import { Store } from '../src/main' import * as fns from '../src/reactor/fns' +import * as KeypathTracker from '../src/reactor/keypath-tracker' import { ReactorState, ObserverState } from '../src/reactor/records' import { toImmutable } from '../src/immutable-helpers' +const status = KeypathTracker.status + describe('reactor fns', () => { describe('#registerStores', () => { let reactorState @@ -52,17 +55,23 @@ describe('reactor fns', () => { expect(is(result, expected)).toBe(true) }) - it('should update reactorState.dirtyStores', () => { - const result = nextReactorState.get('dirtyStores') - const expected = Set.of('store1', 'store2') - expect(is(result, expected)).toBe(true) - }) - - it('should update reactorState.dirtyStores', () => { - const result = nextReactorState.get('storeStates') - const expected = Map({ - store1: 1, - store2: 1, + it('should update keypathStates', () => { + const result = nextReactorState.get('keypathStates') + const expected = new KeypathTracker.Node({ + state: 3, + status: status.DIRTY, + children: toImmutable({ + store1: { + state: 1, + status: status.DIRTY, + children: {}, + }, + store2: { + state: 1, + status: status.DIRTY, + children: {}, + }, + }), }) expect(is(result, expected)).toBe(true) }) @@ -74,7 +83,7 @@ describe('reactor fns', () => { }) }) - describe('#registerStores', () => { + describe('#replaceStores', () => { let reactorState let store1 let store2 @@ -149,9 +158,8 @@ describe('reactor fns', () => { }, }) - initialReactorState = fns.resetDirtyStores( - fns.registerStores(reactorState, { store1, store2 }) - ) + initialReactorState = fns.registerStores(reactorState, { store1, store2 }) + .update('keypathStates', KeypathTracker.incrementAndClean) }) describe('when dispatching an action that updates 1 store', () => { @@ -176,17 +184,23 @@ describe('reactor fns', () => { expect(is(result, expected)).toBe(true) }) - it('should update dirtyStores', () => { - const result = nextReactorState.get('dirtyStores') - const expected = Set.of('store2') - expect(is(result, expected)).toBe(true) - }) - - it('should update storeStates', () => { - const result = nextReactorState.get('storeStates') - const expected = Map({ - store1: 1, - store2: 2, + it('should update keypathStates', () => { + const result = nextReactorState.get('keypathStates') + const expected = new KeypathTracker.Node({ + state: 4, + status: status.DIRTY, + children: toImmutable({ + store1: { + state: 1, + status: status.CLEAN, + children: {}, + }, + store2: { + state: 2, + status: status.DIRTY, + children: {}, + }, + }), }) expect(is(result, expected)).toBe(true) }) @@ -214,17 +228,67 @@ describe('reactor fns', () => { expect(is(result, expected)).toBe(true) }) - it('should not update dirtyStores', () => { - const result = nextReactorState.get('dirtyStores') - const expected = Set() + it('should update keypathStates', () => { + const result = nextReactorState.get('keypathStates') + const expected = new KeypathTracker.Node({ + state: 3, + status: status.CLEAN, + children: toImmutable({ + store1: { + state: 1, + status: status.CLEAN, + children: {}, + }, + store2: { + state: 1, + status: status.CLEAN, + children: {}, + }, + }), + }) expect(is(result, expected)).toBe(true) }) + }) - it('should not update storeStates', () => { - const result = nextReactorState.get('storeStates') - const expected = Map({ - store1: 1, - store2: 1, + describe('when a deep keypathState exists and dispatching an action that changes non-leaf node', () => { + beforeEach(() => { + // add store2, prop1, prop2 entries to the keypath state + // this is similiar to someone observing this keypath before it's defined + const newReactorState = initialReactorState.update('keypathStates', k => { + return KeypathTracker.unchanged(k, ['store2', 'prop1', 'prop2']) + }) + nextReactorState = fns.dispatch(newReactorState, 'set2', 3) + }) + + it('should update keypathStates', () => { + const result = nextReactorState.get('keypathStates') + const expected = new KeypathTracker.Node({ + state: 4, + status: status.DIRTY, + children: toImmutable({ + store1: { + state: 1, + status: status.CLEAN, + children: {}, + }, + store2: { + state: 2, + status: status.DIRTY, + children: { + prop1: { + state: 1, + status: status.UNKNOWN, + children: { + prop2: { + state: 1, + status: status.UNKNOWN, + children: {}, + }, + }, + }, + }, + }, + }), }) expect(is(result, expected)).toBe(true) }) @@ -261,9 +325,8 @@ describe('reactor fns', () => { }, }) - initialReactorState = fns.resetDirtyStores( - fns.registerStores(reactorState, { store1, store2 }) - ) + initialReactorState = fns.registerStores(reactorState, { store1, store2 }) + .update('keypathStates', KeypathTracker.incrementAndClean) nextReactorState = fns.loadState(initialReactorState, stateToLoad) }) @@ -279,20 +342,27 @@ describe('reactor fns', () => { expect(is(expected, result)).toBe(true) }) - it('should update dirtyStores', () => { - const result = nextReactorState.get('dirtyStores') - const expected = Set.of('store1') - expect(is(expected, result)).toBe(true) - }) - - it('should update storeStates', () => { - const result = nextReactorState.get('storeStates') - const expected = Map({ - store1: 2, - store2: 1, + it('should update keypathStates', () => { + const result = nextReactorState.get('keypathStates') + const expected = new KeypathTracker.Node({ + state: 4, + status: status.DIRTY, + children: toImmutable({ + store1: { + state: 2, + status: status.DIRTY, + children: {}, + }, + store2: { + state: 1, + status: status.CLEAN, + children: {}, + }, + }), }) - expect(is(expected, result)).toBe(true) + expect(is(result, expected)).toBe(true) }) + }) describe('#reset', () => { @@ -320,9 +390,8 @@ describe('reactor fns', () => { }, }) - initialReactorState = fns.resetDirtyStores( - fns.registerStores(reactorState, { store1, store2, }) - ) + initialReactorState = fns.registerStores(reactorState, { store1, store2, }) + .update('keypathStates', KeypathTracker.incrementAndClean) // perform a dispatch then reset nextReactorState = fns.reset( @@ -341,69 +410,63 @@ describe('reactor fns', () => { expect(is(expected, result)).toBe(true) }) - it('should reset dirtyStores', () => { - const result = nextReactorState.get('dirtyStores') - const expected = Set() - expect(is(expected, result)).toBe(true) - }) - - it('should update storeStates', () => { - const result = nextReactorState.get('storeStates') - const expected = Map({ - store1: 3, - store2: 2, + it('should update keypathStates', () => { + const result = nextReactorState.get('keypathStates') + const expected = new KeypathTracker.Node({ + state: 6, + status: status.DIRTY, + children: toImmutable({ + store1: { + state: 3, + status: status.DIRTY, + children: {}, + }, + store2: { + state: 2, + status: status.DIRTY, + children: {}, + }, + }), }) - expect(is(expected, result)).toBe(true) + expect(is(result, expected)).toBe(true) }) }) describe('#addObserver', () => { - let initialObserverState, nextObserverState, entry, handler, getter + let initialObserverState, nextObserverState, entry, handler, getter, nextReactorState describe('when observing the identity getter', () => { beforeEach(() => { getter = [[], x => x] handler = function() {} + const reactorState = new ReactorState() initialObserverState = new ObserverState() - const result = fns.addObserver(initialObserverState, getter, handler) + const result = fns.addObserver(reactorState, initialObserverState, getter, handler) nextObserverState = result.observerState entry = result.entry - }) - it('should update the "any" observers', () => { - const expected = Set.of(1) - const result = nextObserverState.get('any') - expect(is(expected, result)).toBe(true) - }) - it('should not update the "store" observers', () => { - const expected = Map({}) - const result = nextObserverState.get('stores') - expect(is(expected, result)).toBe(true) - }) - it('should increment the nextId', () => { - const expected = 2 - const result = nextObserverState.get('nextId') - expect(is(expected, result)).toBe(true) - }) - it('should update the observerMap', () => { - const expected = Map([ - [1, Map({ - id: 1, - storeDeps: Set(), - getterKey: getter, - getter: getter, - handler: handler, - })], + it('should properly update the observer state', () => { + const expectedTrackedKeypaths = Set.of(List([])) + expect(is(expectedTrackedKeypaths, nextObserverState.get('trackedKeypaths'))).toBe(true) + + const expectedObserversMap = Map().setIn( + [toImmutable(getter), handler], + Map({ getter, handler }) + ) + expect(is(expectedObserversMap, nextObserverState.get('observersMap'))).toBe(true) + + const expectedKeypathToEntries = Map([ + [toImmutable([]), Set.of(entry)] ]) - const result = nextObserverState.get('observersMap') - expect(is(expected, result)).toBe(true) + + expect(is(expectedKeypathToEntries, nextObserverState.get('keypathToEntries'))).toBe(true) + + const expectedObservers = Set.of(entry) + expect(is(expectedObservers, nextObserverState.get('observers'))).toBe(true) }) it('should return a valid entry', () => { const expected = Map({ - id: 1, - storeDeps: Set(), - getterKey: getter, getter: getter, handler: handler, }) @@ -414,54 +477,42 @@ describe('reactor fns', () => { describe('when observing a store backed getter', () => { beforeEach(() => { getter = [ - ['store1'], + ['store1', 'prop1', 'prop2', 'prop3'], ['store2'], (a, b) => a + b ] handler = function() {} + const reactorState = new ReactorState() initialObserverState = new ObserverState() - const result = fns.addObserver(initialObserverState, getter, handler) + const result = fns.addObserver(reactorState, initialObserverState, getter, handler) nextObserverState = result.observerState entry = result.entry }) - it('should not update the "any" observers', () => { - const expected = Set.of() - const result = nextObserverState.get('any') - expect(is(expected, result)).toBe(true) - }) - it('should not update the "store" observers', () => { - const expected = Map({ - store1: Set.of(1), - store2: Set.of(1), - }) - - const result = nextObserverState.get('stores') - expect(is(expected, result)).toBe(true) - }) - it('should increment the nextId', () => { - const expected = 2 - const result = nextObserverState.get('nextId') - expect(is(expected, result)).toBe(true) - }) - it('should update the observerMap', () => { - const expected = Map([ - [1, Map({ - id: 1, - storeDeps: Set.of('store1', 'store2'), - getterKey: getter, - getter: getter, - handler: handler, - })] + it('should properly update the observer state', () => { + const expectedTrackedKeypaths = Set.of( + List(['store1', 'prop1', 'prop2']), + List(['store2']) + ) + expect(is(expectedTrackedKeypaths, nextObserverState.get('trackedKeypaths'))).toBe(true) + + const expectedObserversMap = Map().setIn( + [toImmutable(getter), handler], + Map({ getter, handler }) + ) + expect(is(expectedObserversMap, nextObserverState.get('observersMap'))).toBe(true) + + const expectedKeypathToEntries = Map([ + [toImmutable(['store1', 'prop1', 'prop2']), Set.of(entry)], + [toImmutable(['store2']), Set.of(entry)], ]) - const result = nextObserverState.get('observersMap') - expect(is(expected, result)).toBe(true) + expect(is(expectedKeypathToEntries, nextObserverState.get('keypathToEntries'))).toBe(true) + + const expectedObservers = Set.of(entry) + expect(is(expectedObservers, nextObserverState.get('observers'))).toBe(true) }) it('should return a valid entry', () => { const expected = Map({ - id: 1, - storeDeps: Set.of('store1', 'store2'), - getterKey: getter, getter: getter, handler: handler, }) @@ -471,7 +522,7 @@ describe('reactor fns', () => { }) describe('#removeObserver', () => { - let initialObserverState, nextObserverState, getter1, getter2, handler1, handler2, handler3 + let reactorState, initialObserverState, nextObserverState, getter1, getter2, handler1, handler2, handler3 beforeEach(() => { handler1 = () => 1 @@ -479,75 +530,79 @@ describe('reactor fns', () => { handler3 = () => 3 getter1 = [ - ['store1'], + ['store1', 'prop1', 'prop2', 'prop3'], ['store2'], (a, b) => a + b ] getter2 = [[], x => x] + reactorState = new ReactorState() const initialObserverState1 = new ObserverState() - const result1 = fns.addObserver(initialObserverState1, getter1, handler1) + const result1 = fns.addObserver(reactorState, initialObserverState1, getter1, handler1) const initialObserverState2 = result1.observerState - const result2 = fns.addObserver(initialObserverState2, getter1, handler2) + const result2 = fns.addObserver(reactorState,initialObserverState2, getter1, handler2) const initialObserverState3 = result2.observerState - const result3 = fns.addObserver(initialObserverState3, getter2, handler3) + const result3 = fns.addObserver(reactorState,initialObserverState3, getter2, handler3) initialObserverState = result3.observerState }) describe('when removing by getter', () => { it('should return a new ObserverState with all entries containing the getter removed', () => { - nextObserverState = fns.removeObserver(initialObserverState, getter1) - const expected = Map({ - any: Set.of(3), - stores: Map({ - store1: Set(), - store2: Set(), - }), - nextId: 4, - observersMap: Map([ - [3, Map({ - id: 3, - storeDeps: Set(), - getterKey: getter2, - getter: getter2, - handler: handler3, - })] - ]) + const result = fns.removeObserver(reactorState, initialObserverState, getter1) + + const expectedObserversMap = Map().setIn( + [toImmutable(getter2), handler3], + Map({ getter: getter2, handler: handler3 }) + ) + expect(is(expectedObserversMap, result.get('observersMap'))).toBe(true) + + const entry = Map({ + getter: getter2, + handler: handler3, }) - const result = nextObserverState - expect(is(expected, result)).toBe(true) + const expectedKeypathToEntries = Map().set(toImmutable([]), Set.of(entry)) + expect(is(expectedKeypathToEntries, result.get('keypathToEntries'))).toBe(true) + + const expectedTrackedKeypaths = Set.of(toImmutable([])) + expect(is(expectedTrackedKeypaths, result.get('trackedKeypaths'))).toBe(true) + + const expectedObservers = Set.of(entry) + expect(is(expectedObservers, result.get('observers'))).toBe(true) }) }) describe('when removing by getter / handler', () => { it('should return a new ObserverState with all entries containing the getter removed', () => { - nextObserverState = fns.removeObserver(initialObserverState, getter2, handler3) - const expected = Map({ - any: Set(), - stores: Map({ - store1: Set.of(1, 2), - store2: Set.of(1, 2), - }), - nextId: 4, - observersMap: Map([ - [1, Map({ - id: 1, - storeDeps: Set.of('store1', 'store2'), - getterKey: getter1, - getter: getter1, - handler: handler1, - })], - [2, Map({ - id: 2, - storeDeps: Set.of('store1', 'store2'), - getterKey: getter1, - getter: getter1, - handler: handler2, - })] - ]) - }) - const result = nextObserverState - expect(is(expected, result)).toBe(true) + const result = fns.removeObserver(reactorState, initialObserverState, getter1, handler1) + + const entry1 = Map({ getter: getter2, handler: handler3 }) + const entry2 = Map({ getter: getter1, handler: handler2 }) + const expectedObserversMap = Map() + .setIn( + [toImmutable(getter2), handler3], + entry1 + ) + .setIn( + [toImmutable(getter1), handler2], + entry2 + ) + expect(is(expectedObserversMap, result.get('observersMap'))).toBe(true) + + const expectedKeypathToEntries = Map() + .set(toImmutable(['store1', 'prop1', 'prop2']), Set.of(Map({ getter: getter1, handler: handler2 }))) + .set(toImmutable(['store2']), Set.of(Map({ getter: getter1, handler: handler2 }))) + .set(toImmutable([]), Set.of(Map({ getter: getter2, handler: handler3 }))) + expect(is(expectedKeypathToEntries, result.get('keypathToEntries'))).toBe(true) + + const expectedTrackedKeypaths = Set.of( + toImmutable([]), + toImmutable(['store1', 'prop1', 'prop2']), + toImmutable(['store2']) + ) + expect(is(expectedTrackedKeypaths, result.get('trackedKeypaths'))).toBe(true) + + const expectedObservers = Set.of(entry1, entry2) + expect(is(expectedObservers, result.get('observers'))).toBe(true) }) }) }) diff --git a/tests/reactor-tests.js b/tests/reactor-tests.js index bc107bf..66acb67 100644 --- a/tests/reactor-tests.js +++ b/tests/reactor-tests.js @@ -4,6 +4,7 @@ import { getOption } from '../src/reactor/fns' import { toImmutable } from '../src/immutable-helpers' import { PROD_OPTIONS, DEBUG_OPTIONS } from '../src/reactor/records' import { NoopLogger, ConsoleGroupLogger } from '../src/logging' +import * as utils from '../src/utils' describe('Reactor', () => { it('should construct without \'new\'', () => { @@ -522,10 +523,10 @@ describe('Reactor', () => { it('should update all state', () => { checkoutActions.addItem(item.name, item.price) - expect(reactor.evaluateToJS(['items', 'all'])).toEqual([item]) + //expect(reactor.evaluateToJS(['items', 'all'])).toEqual([item]) - expect(reactor.evaluate(['taxPercent'])).toEqual(0) - expect(reactor.evaluate(taxGetter)).toEqual(0) + //expect(reactor.evaluate(['taxPercent'])).toEqual(0) + //expect(reactor.evaluate(taxGetter)).toEqual(0) expect(reactor.evaluate(totalGetter)).toEqual(10) }) @@ -1964,4 +1965,136 @@ describe('Reactor', () => { expect(reactor.evaluate(['counter2'])).toBe(21) }) }) + + describe('caching', () => { + let reactor + + beforeEach(() => { + reactor = new Reactor({ + debug: true, + }) + + const entity = new Store({ + getInitialState() { + return toImmutable({}) + }, + + initialize() { + this.on('loadEntities', (state, payload) => { + return state.withMutations(s => { + utils.each(payload.data, (val, key) => { + const id = Number(val.id) + s.setIn([payload.entity, id], toImmutable(val)) + }) + }) + }) + }, + }) + + const currentProjectId = new Store({ + getInitialState() { + return null + }, + + initialize() { + this.on('setCurrentProjectId', (state, payload) => payload) + }, + }) + + reactor.registerStores({ + entity, + currentProjectId + }) + }) + + describe('when observing the current project', () => { + let projectsGetter, currentProjectGetter + let projectsGetterSpy, currentProjectGetterSpy, currentProjectObserverSpy + + beforeEach(() => { + projectsGetterSpy = jasmine.createSpy() + currentProjectGetterSpy = jasmine.createSpy() + currentProjectObserverSpy = jasmine.createSpy() + + projectsGetter = [ + ['entity', 'projects'], + (projects) => { + projectsGetterSpy() + if (!projects) { + return toImmutable({}) + } + + return projects + } + ] + + currentProjectGetter = [ + projectsGetter, + ['currentProjectId'], + (projects, id) => { + currentProjectGetterSpy() + return projects.get(id) + } + ] + + // load initial data + reactor.dispatch('loadEntities', { + entity: 'projects', + data: { + 1: { id: 1, name: 'proj1' }, + 2: { id: 2, name: 'proj2' }, + 3: { id: 3, name: 'proj3' }, + }, + }) + + reactor.dispatch('setCurrentProjectId', 1) + + reactor.observe(currentProjectGetter, currentProjectObserverSpy) + }) + + + it('should not re-evaluate for the same dispatch cycle when using evaluate', () => { + const expected = toImmutable({ id: 1, name: 'proj1' }) + const result1 = reactor.evaluate(currentProjectGetter) + + expect(is(result1, expected)).toBe(true) + expect(currentProjectGetterSpy.calls.count()).toEqual(1) + + const result2 = reactor.evaluate(currentProjectGetter) + + expect(is(result2, expected)).toBe(true) + expect(currentProjectGetterSpy.calls.count()).toEqual(1) + expect(projectsGetterSpy.calls.count()).toEqual(1) + }) + + it('should not re-evaluate when another entity is loaded', () => { + expect(projectsGetterSpy.calls.count()).toEqual(0) + expect(currentProjectGetterSpy.calls.count()).toEqual(0) + reactor.dispatch('setCurrentProjectId', 2) + + // both getter spies are called twice, once with the prevReactorState and once with the currReactorState + expect(projectsGetterSpy.calls.count()).toEqual(2) + expect(currentProjectGetterSpy.calls.count()).toEqual(2) + expect(currentProjectObserverSpy.calls.count()).toEqual(1) + + reactor.dispatch('loadEntities', { + entity: 'other', + data: { + 11: { id: 11, name: 'other 11' }, + }, + }) + + // modifying a piece of the state map that isn't a dependencey should have no getter re-evaluation + expect(projectsGetterSpy.calls.count()).toEqual(2) + expect(currentProjectGetterSpy.calls.count()).toEqual(2) + expect(currentProjectObserverSpy.calls.count()).toEqual(1) + + reactor.dispatch('setCurrentProjectId', 3) + // ['entity', 'projects'] didn't change so projectsGetter should be cached + expect(projectsGetterSpy.calls.count()).toEqual(2) + expect(currentProjectGetterSpy.calls.count()).toEqual(3) + expect(currentProjectObserverSpy.calls.count()).toEqual(2) + }) + }) + }) }) From 284b0039f6aa45846b5f4f1be46563b3ff76d717 Mon Sep 17 00:00:00 2001 From: jordangarcia Date: Fri, 30 Sep 2016 13:27:15 -0700 Subject: [PATCH 2/6] Improve cleaning phase of keypath tracker cache Instead of doing a full recursive traversal of the keypathStates tree, keep track of all changed paths as use that as an indicator of the paths the need to be cleaned --- src/reactor/keypath-tracker.js | 119 +++++++++++++++++++++++++++++---- src/reactor/records.js | 2 +- tests/keypath-tracker-tests.js | 71 +++++++++++--------- tests/reactor-fns-tests.js | 18 +++-- 4 files changed, 156 insertions(+), 54 deletions(-) diff --git a/src/reactor/keypath-tracker.js b/src/reactor/keypath-tracker.js index c966201..ed0848b 100644 --- a/src/reactor/keypath-tracker.js +++ b/src/reactor/keypath-tracker.js @@ -8,7 +8,7 @@ * k * */ -import { Map, Record } from 'immutable' +import { Map, Record, Set } from 'immutable' import { toImmutable, toJS } from '../immutable-helpers' export const status = { @@ -17,12 +17,26 @@ export const status = { UNKNOWN: 2, } -export const Node = Record({ +export const RootNode = Record({ status: status.CLEAN, state: 1, children: Map(), + changedPaths: Set(), }) +const Node = Record({ + status: status.CLEAN, + state: 1, + children: Map(), +}) + +/** + * Denotes that a keypath hasn't changed + * Makes the Node at the keypath as CLEAN and recursively marks the children as CLEAN + * @param {Immutable.Map} map + * @param {Keypath} keypath + * @return {Status} + */ export function unchanged(map, keypath) { const childKeypath = getChildKeypath(keypath) if (!map.hasIn(childKeypath)) { @@ -36,14 +50,24 @@ export function unchanged(map, keypath) { }) } +/** + * Denotes that a keypath has changed + * Traverses to the Node at the keypath and marks as DIRTY, marks all children as UNKNOWN + * @param {Immutable.Map} map + * @param {Keypath} keypath + * @return {Status} + */ export function changed(map, keypath) { const childrenKeypath = getChildKeypath(keypath).concat('children') - return map - .update('children', children => recursiveIncrement(children, keypath)) + // TODO(jordan): can this be optimized + return map.withMutations(m => { + m.update('changedPaths', p => p.add(toImmutable(keypath))) + m.update('children', children => recursiveIncrement(children, keypath)) // handle the root node - .update('state', val => val + 1) - .set('status', status.DIRTY) - .updateIn(childrenKeypath, entry => recursiveSetStatus(entry, status.UNKNOWN)) + m.update('state', val => val + 1) + m.set('status', status.DIRTY) + m.updateIn(childrenKeypath, entry => recursiveSetStatus(entry, status.UNKNOWN)) + }) } /** @@ -63,12 +87,7 @@ export function isEqual(map, keypath, value) { return entry.get('state') === value; } -/** - * Increments all unknown states and sets everything to CLEAN - * @param {Immutable.Map} map - * @return {Status} - */ -export function incrementAndClean(map) { +function recursiveClean(map) { if (map.size === 0) { return map } @@ -82,11 +101,37 @@ export function incrementAndClean(map) { return map .update('children', c => c.withMutations(m => { m.keySeq().forEach(key => { - m.update(key, incrementAndClean) + m.update(key, recursiveClean) }) })) } +/** + * Increments all unknown states and sets everything to CLEAN + * @param {Immutable.Map} map + * @return {Status} + */ +export function incrementAndClean(map) { + if (map.size === 0) { + return map + } + const changedPaths = map.get('changedPaths') + // TODO(jordan): can this be optimized + return map.withMutations(m => { + changedPaths.forEach(path => { + m.update('children', c => traverseAndMarkClean(c, path)) + }) + + m.set('changedPaths', Set()) + const rootStatus = m.get('status') + if (rootStatus === status.DIRTY) { + setClean(m) + } else if (rootStatus === status.UNKNOWN) { + setClean(increment(m)) + } + }) +} + export function get(map, keypath) { return map.getIn(getChildKeypath(keypath).concat('state')) } @@ -129,6 +174,52 @@ function recursiveIncrement(map, keypath) { }) } +/** + * Traverses up to a keypath and marks all entries as clean along the way, then recursively traverses over all children + * @param {Immutable.Map} map + * @param {Immutable.List} keypath + * @return {Status} + */ +function traverseAndMarkClean(map, keypath) { + if (keypath.size === 0) { + return recursiveCleanChildren(map) + } + return map.withMutations(map => { + const key = keypath.first() + + map.update(key, incrementAndCleanNode) + map.updateIn([key, 'children'], children => traverseAndMarkClean(children, keypath.rest())) + }) +} + +function recursiveCleanChildren(children) { + if (children.size === 0) { + return children + } + + return children.withMutations(c => { + c.keySeq().forEach(key => { + c.update(key, incrementAndCleanNode) + c.updateIn([key, 'children'], recursiveCleanChildren) + }) + }) +} + +/** + * Takes a node, marks it CLEAN, if it was UNKNOWN it increments + * @param {Node} node + * @return {Status} + */ +function incrementAndCleanNode(node) { + const nodeStatus = node.get('status') + if (nodeStatus === status.DIRTY) { + return setClean(node) + } else if (nodeStatus === status.UNKNOWN) { + return setClean(increment(node)) + } + return node +} + function recursiveRegister(map, keypath) { keypath = toImmutable(keypath) if (keypath.size === 0) { diff --git a/src/reactor/records.js b/src/reactor/records.js index 4c563ff..20c62c1 100644 --- a/src/reactor/records.js +++ b/src/reactor/records.js @@ -1,6 +1,6 @@ import { Map, Set, Record } from 'immutable' import { DefaultCache } from './cache' -import { Node as KeypathTrackerNode } from './keypath-tracker' +import { RootNode as KeypathTrackerNode } from './keypath-tracker' export const PROD_OPTIONS = Map({ // logs information for each dispatch diff --git a/tests/keypath-tracker-tests.js b/tests/keypath-tracker-tests.js index 38510a1..3e36268 100644 --- a/tests/keypath-tracker-tests.js +++ b/tests/keypath-tracker-tests.js @@ -1,18 +1,18 @@ /*eslint-disable one-var, comma-dangle*/ -import { Map, Set, is } from 'immutable' +import { Map, List, Set, is } from 'immutable' import * as KeypathTracker from '../src/reactor/keypath-tracker' import { toImmutable } from '../src/immutable-helpers' -const { status, Node } = KeypathTracker +const { status, RootNode } = KeypathTracker describe('Keypath Tracker', () => { describe('unchanged', () => { it('should properly register ["foo"]', () => { const keypath = ['foo'] - const state = new Node() + const state = new RootNode() let tracker = KeypathTracker.unchanged(state, keypath) - const expected = new Node({ + const expected = new RootNode({ status: status.CLEAN, state: 1, children: toImmutable({ @@ -28,10 +28,10 @@ describe('Keypath Tracker', () => { it('should properly register ["foo", "bar"]', () => { const keypath = ['foo', 'bar'] - const state = new Node() + const state = new RootNode() let tracker = KeypathTracker.unchanged(state, keypath) - const expected = new Node({ + const expected = new RootNode({ status: status.CLEAN, state: 1, children: toImmutable({ @@ -54,7 +54,7 @@ describe('Keypath Tracker', () => { it('should register ["foo", "bar"] when ["foo"] is already registered', () => { const keypath = ['foo', 'bar'] - const origTracker = new Node({ + const origTracker = new RootNode({ status: status.CLEAN, state: 1, children: toImmutable({ @@ -66,7 +66,7 @@ describe('Keypath Tracker', () => { }) }) const tracker = KeypathTracker.unchanged(origTracker, keypath) - const expected = new Node({ + const expected = new RootNode({ status: status.CLEAN, state: 1, children: toImmutable({ @@ -89,7 +89,7 @@ describe('Keypath Tracker', () => { it('should mark something as unchanged', () => { const keypath = ['foo', 'bar'] - const orig = new Node({ + const orig = new RootNode({ status: status.CLEAN, state: 1, children: toImmutable({ @@ -107,7 +107,7 @@ describe('Keypath Tracker', () => { }), }) const tracker = KeypathTracker.unchanged(orig, keypath) - const expected = new Node({ + const expected = new RootNode({ status: status.CLEAN, state: 1, children: toImmutable({ @@ -129,7 +129,7 @@ describe('Keypath Tracker', () => { }) it('should mark the root node as unchanged', () => { - const orig = new Node({ + const orig = new RootNode({ status: status.UNKNOWN, state: 1, children: toImmutable({ @@ -141,7 +141,7 @@ describe('Keypath Tracker', () => { }), }) const tracker = KeypathTracker.unchanged(orig, []) - const expected = new Node({ + const expected = new RootNode({ status: status.CLEAN, state: 1, children: toImmutable({ @@ -160,12 +160,13 @@ describe('Keypath Tracker', () => { describe('changed', () => { it('should initialize a node with a DIRTY status', () => { - const orig = new Node({ + const orig = new RootNode({ status: status.CLEAN, state: 1, }) const result = KeypathTracker.changed(orig, ['foo']) - const expected = new Node({ + const expected = new RootNode({ + changedPaths: Set.of(toImmutable(['foo'])), status: status.DIRTY, state: 2, children: toImmutable({ @@ -179,8 +180,8 @@ describe('Keypath Tracker', () => { expect(is(result, expected)).toBe(true) }) - it('should traverse and increment for parents and mark children clean', () => { - const orig = new Node({ + it('should traverse and increment for parents and mark children UNKNOWN', () => { + const orig = new RootNode({ status: status.CLEAN, state: 1, children: toImmutable({ @@ -208,7 +209,8 @@ describe('Keypath Tracker', () => { }), }) const result = KeypathTracker.changed(orig, ['foo', 'bar']) - const expected = new Node({ + const expected = new RootNode({ + changedPaths: Set.of(toImmutable(['foo', 'bar'])), status: status.DIRTY, state: 2, children: toImmutable({ @@ -241,7 +243,7 @@ describe('Keypath Tracker', () => { }) it('should handle the root node', () => { - const orig = new Node({ + const orig = new RootNode({ status: status.CLEAN, state: 1, children: toImmutable({ @@ -270,7 +272,8 @@ describe('Keypath Tracker', () => { }), }) const result = KeypathTracker.changed(orig, []) - const expected = new Node({ + const expected = new RootNode({ + changedPaths: Set.of(toImmutable([])), status: status.DIRTY, state: 2, children: toImmutable({ @@ -304,7 +307,7 @@ describe('Keypath Tracker', () => { }) describe('isEqual', () => { - const state = new Node({ + const state = new RootNode({ state: 1, status: status.DIRTY, children: toImmutable({ @@ -360,7 +363,7 @@ describe('Keypath Tracker', () => { }) describe('get', () => { - const state = new Node({ + const state = new RootNode({ state: 1, status: status.DIRTY, children: toImmutable({ @@ -406,7 +409,7 @@ describe('Keypath Tracker', () => { }) describe('isClean', () => { - const state = new Node({ + const state = new RootNode({ state: 1, status: status.DIRTY, children: toImmutable({ @@ -458,10 +461,11 @@ describe('Keypath Tracker', () => { describe('incrementAndClean', () => { it('should work when the root node is clean', () => { - const state = new Node({ + const state = new RootNode({ + changedPaths: Set.of(List(['foo'])), state: 2, status: status.CLEAN, - chidlren: toImmutable({ + children: toImmutable({ foo: { status: status.DIRTY, state: 2, @@ -476,10 +480,10 @@ describe('Keypath Tracker', () => { }), }) - const expected = new Node({ + const expected = new RootNode({ state: 2, status: status.CLEAN, - chidlren: toImmutable({ + children: toImmutable({ foo: { status: status.CLEAN, state: 2, @@ -499,16 +503,17 @@ describe('Keypath Tracker', () => { expect(is(result, expected)).toBe(true) }) it('should traverse the tree and increment any state value thats UNKNOWN and mark everything CLEAN', () => { - const state = new Node({ + const state = new RootNode({ + changedPaths: Set.of(List(['foo', 'bar']), List(['foo', 'bar', 'bat'])), state: 2, status: status.DIRTY, - chidlren: toImmutable({ + children: toImmutable({ foo: { status: status.DIRTY, state: 2, children: { bar: { - status: status.UNKNOWN, + status: status.DIRTY, state: 1, children: { baz: { @@ -533,20 +538,20 @@ describe('Keypath Tracker', () => { }), }) - const expected = new Node({ + const expected = new RootNode({ state: 2, status: status.CLEAN, - chidlren: toImmutable({ + children: toImmutable({ foo: { status: status.CLEAN, state: 2, children: { bar: { status: status.CLEAN, - state: 2, + state: 1, children: { baz: { - status: status.UNKNOWN, + status: status.CLEAN, state: 2, children: {}, }, diff --git a/tests/reactor-fns-tests.js b/tests/reactor-fns-tests.js index 33a22a9..1c2227d 100644 --- a/tests/reactor-fns-tests.js +++ b/tests/reactor-fns-tests.js @@ -57,7 +57,8 @@ describe('reactor fns', () => { it('should update keypathStates', () => { const result = nextReactorState.get('keypathStates') - const expected = new KeypathTracker.Node({ + const expected = new KeypathTracker.RootNode({ + changedPaths: Set.of(List(['store1']), List(['store2'])), state: 3, status: status.DIRTY, children: toImmutable({ @@ -186,7 +187,8 @@ describe('reactor fns', () => { it('should update keypathStates', () => { const result = nextReactorState.get('keypathStates') - const expected = new KeypathTracker.Node({ + const expected = new KeypathTracker.RootNode({ + changedPaths: Set.of(List(['store2'])), state: 4, status: status.DIRTY, children: toImmutable({ @@ -202,6 +204,7 @@ describe('reactor fns', () => { }, }), }) + expect(is(result, expected)).toBe(true) }) }) @@ -230,7 +233,7 @@ describe('reactor fns', () => { it('should update keypathStates', () => { const result = nextReactorState.get('keypathStates') - const expected = new KeypathTracker.Node({ + const expected = new KeypathTracker.RootNode({ state: 3, status: status.CLEAN, children: toImmutable({ @@ -262,7 +265,8 @@ describe('reactor fns', () => { it('should update keypathStates', () => { const result = nextReactorState.get('keypathStates') - const expected = new KeypathTracker.Node({ + const expected = new KeypathTracker.RootNode({ + changedPaths: Set.of(List(['store2'])), state: 4, status: status.DIRTY, children: toImmutable({ @@ -344,7 +348,8 @@ describe('reactor fns', () => { it('should update keypathStates', () => { const result = nextReactorState.get('keypathStates') - const expected = new KeypathTracker.Node({ + const expected = new KeypathTracker.RootNode({ + changedPaths: Set.of(List(['store1'])), state: 4, status: status.DIRTY, children: toImmutable({ @@ -412,7 +417,8 @@ describe('reactor fns', () => { it('should update keypathStates', () => { const result = nextReactorState.get('keypathStates') - const expected = new KeypathTracker.Node({ + const expected = new KeypathTracker.RootNode({ + changedPaths: Set.of(List(['store1']), List(['store2'])), state: 6, status: status.DIRTY, children: toImmutable({ From 1184c03331521a8ccc32d9619e439beacf8d0d35 Mon Sep 17 00:00:00 2001 From: jordangarcia Date: Fri, 30 Sep 2016 16:57:39 -0700 Subject: [PATCH 3/6] Improve performance of LRU cache --- src/reactor/cache.js | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/reactor/cache.js b/src/reactor/cache.js index 18978fe..2be3f96 100644 --- a/src/reactor/cache.js +++ b/src/reactor/cache.js @@ -93,6 +93,21 @@ export class BasicCache { evict(item) { return new BasicCache(this.cache.remove(item)) } + + /** + * Removes entry from cache + * @param {Iterable} items + * @return {BasicCache} + */ + evictMany(items) { + const newCache = this.cache.withMutations(c => { + items.forEach(item => { + c.remove(item) + }) + }) + + return new BasicCache(newCache) + } } const DEFAULT_LRU_LIMIT = 1000 @@ -173,15 +188,12 @@ export class LRUCache { ) } - const cache = (this.lru - .take(this.evictCount) - .reduce((c, evictItem) => c.evict(evictItem), this.cache) - .miss(item, entry)) + const itemsToRemove = this.lru.take(this.evictCount) lruCache = new LRUCache( this.limit, this.evictCount, - cache, + this.cache.evictMany(itemsToRemove).miss(item, entry), this.lru.skip(this.evictCount).add(item) ) } else { From e90a721cfc135d0e41be4b7e199ff7076e6076af Mon Sep 17 00:00:00 2001 From: jordangarcia Date: Fri, 30 Sep 2016 17:45:02 -0700 Subject: [PATCH 4/6] Use mutable versions of ObserverState --- src/reactor.js | 9 ++-- src/reactor/fns.js | 93 +++++++++++++++++++----------------------- src/reactor/records.js | 8 ++-- 3 files changed, 51 insertions(+), 59 deletions(-) diff --git a/src/reactor.js b/src/reactor.js index e88182f..0f5f676 100644 --- a/src/reactor.js +++ b/src/reactor.js @@ -45,6 +45,8 @@ class Reactor { this.reactorState = initialReactorState this.observerState = new ObserverState() + this.observerState = this.observerState.asMutable() + this.ReactMixin = createReactMixin(this) // keep track of the depth of batch nesting @@ -108,10 +110,9 @@ class Reactor { handler = getter getter = [] } - let { observerState, entry } = fns.addObserver(this.reactorState, this.observerState, getter, handler) - this.observerState = observerState + let entry = fns.addObserver(this.reactorState, this.observerState, getter, handler) return () => { - this.observerState = fns.removeObserverByEntry(this.reactorState, this.observerState, entry) + fns.removeObserverByEntry(this.reactorState, this.observerState, entry) } } @@ -123,7 +124,7 @@ class Reactor { throw new Error('Must call unobserve with a Getter') } - this.observerState = fns.removeObserver(this.reactorState, this.observerState, getter, handler) + fns.removeObserver(this.reactorState, this.observerState, getter, handler) } /** diff --git a/src/reactor/fns.js b/src/reactor/fns.js index 27afc83..a27271f 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -188,27 +188,22 @@ export function addObserver(reactorState, observerState, getter, handler) { handler: handler, }) - let updatedObserverState = observerState.withMutations(map => { - keypathDeps.forEach(keypath => { - map.updateIn(['keypathToEntries', keypath], entries => { - return (entries) - ? entries.add(entry) - : Immutable.Set().add(entry) - }) - }) + const keypathToEntries = observerState.get('keypathToEntries') + + keypathDeps.forEach(keypath => { + if (!keypathToEntries.has(keypath)) { + keypathToEntries.set(keypath, Immutable.Set().asMutable().add(entry)) + } else { + keypathToEntries.get(keypath).add(entry) + } }) const getterKey = createGetterKey(getter); + observerState.get('trackedKeypaths').union(keypathDeps) + observerState.get('observersMap').setIn([getterKey, handler], entry) + observerState.get('observers').add(entry) - const finalObserverState = updatedObserverState - .update('trackedKeypaths', keypaths => keypaths.union(keypathDeps)) - .setIn(['observersMap', getterKey, handler], entry) - .update('observers', observers => observers.add(entry)) - - return { - observerState: finalObserverState, - entry: entry, - } + return entry } /** @@ -254,9 +249,7 @@ export function removeObserver(reactorState, observerState, getter, handler) { entriesToRemove = observerState.getIn(['observersMap', getterKey], Immutable.Map({})).toList() } - return observerState.withMutations(map => { - entriesToRemove.forEach(entry => removeObserverByEntry(reactorState, map, entry, keypathDeps)) - }) + entriesToRemove.forEach(entry => removeObserverByEntry(reactorState, map, entry, keypathDeps)) } /** @@ -266,42 +259,40 @@ export function removeObserver(reactorState, observerState, getter, handler) { * @return {ObserverState} */ export function removeObserverByEntry(reactorState, observerState, entry, keypathDeps = null) { - return observerState.withMutations(map => { - const getter = entry.get('getter') - if (!keypathDeps) { - const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') - keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) - } + const getter = entry.get('getter') + if (!keypathDeps) { + const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') + keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) + } - map.update('observers', observers => observers.remove(entry)) + observerState.get('observers').remove(entry) - // update the keypathToEntries - keypathDeps.forEach(keypath => { - const kp = ['keypathToEntries', keypath] - map.updateIn(kp, entries => { - // check for observers being present because reactor.reset() can be called before an unwatch fn - return (entries) - ? entries.remove(entry) - : entries - }) - // protect against unwatch after reset - if (map.hasIn(kp) && - map.getIn(kp).size === 0) { - map.removeIn(kp) - map.update('trackedKeypaths', keypaths => keypaths.remove(keypath)) - } - }) + // update the keypathToEntries + keypathDeps.forEach(keypath => { + const kp = ['keypathToEntries', keypath] + const entries = observerState.getIn(kp) - // remove entry from observersMap - const getterKey = createGetterKey(getter) - const handler = entry.get('handler') - map.removeIn(['observersMap', getterKey, handler]) - // protect against unwatch after reset - if (map.hasIn(['observersMap', getterKey]) && - map.getIn(['observersMap', getterKey]).size === 0) { - map.removeIn(['observersMap', getterKey]) + if (entries) { + // check for observers being present because reactor.reset() can be called before an unwatch fn + entries.remove(entry) + if (entries.size === 0) { + observerState.removeIn(kp) + observerState.get('trackedKeypaths').remove(keypath) + } } }) + + // remove entry from observersobserverState + const getterKey = createGetterKey(getter) + const handler = entry.get('handler') + + const observersMap = observerState.get('observersMap') + observersMap.removeIn([getterKey, handler]) + // protect against unwatch after reset + if (observersMap.has(getterKey) && + observersMap.get(getterKey).size === 0) { + observersMap.remove(getterKey) + } } /** diff --git a/src/reactor/records.js b/src/reactor/records.js index 20c62c1..f06a3fe 100644 --- a/src/reactor/records.js +++ b/src/reactor/records.js @@ -58,7 +58,7 @@ export const ObserverState = Record({ : Set } */ - keypathToEntries: Map({}), + keypathToEntries: Map({}).asMutable(), /* { @@ -67,11 +67,11 @@ export const ObserverState = Record({ } } */ - observersMap: Map({}), + observersMap: Map({}).asMutable(), - trackedKeypaths: Set(), + trackedKeypaths: Set().asMutable(), // keep a flat set of observers to know when one is removed during a handler - observers: Set(), + observers: Set().asMutable(), }) From 2d6865f6322478e22462d09d1db54e2ab3f12a7f Mon Sep 17 00:00:00 2001 From: jordangarcia Date: Sat, 1 Oct 2016 19:18:46 -0700 Subject: [PATCH 5/6] Refactor ObseverState to be mutable --- src/reactor.js | 86 +++++-------- src/reactor/fns.js | 226 +++++----------------------------- src/reactor/observer-state.js | 189 ++++++++++++++++++++++++++++ src/reactor/records.js | 23 ---- tests/observer-state-tests.js | 172 ++++++++++++++++++++++++++ tests/react-mixin-tests.js | 2 +- tests/reactor-fns-tests.js | 174 -------------------------- 7 files changed, 428 insertions(+), 444 deletions(-) create mode 100644 src/reactor/observer-state.js create mode 100644 tests/observer-state-tests.js diff --git a/src/reactor.js b/src/reactor.js index 0f5f676..3050322 100644 --- a/src/reactor.js +++ b/src/reactor.js @@ -1,4 +1,4 @@ -import Immutable from 'immutable' +import { Map, is, Set } from 'immutable' import createReactMixin from './create-react-mixin' import * as fns from './reactor/fns' import { DefaultCache } from './reactor/cache' @@ -7,9 +7,9 @@ import { isKeyPath } from './key-path' import { isGetter, getCanonicalKeypathDeps } from './getter' import { toJS } from './immutable-helpers' import { extend, toFactory } from './utils' +import ObserverState from './reactor/observer-state' import { ReactorState, - ObserverState, DEBUG_OPTIONS, PROD_OPTIONS, } from './reactor/records' @@ -45,8 +45,6 @@ class Reactor { this.reactorState = initialReactorState this.observerState = new ObserverState() - this.observerState = this.observerState.asMutable() - this.ReactMixin = createReactMixin(this) // keep track of the depth of batch nesting @@ -62,21 +60,22 @@ class Reactor { * @return {*} */ evaluate(keyPathOrGetter) { - // look through the keypathStates and see if any of the getters dependencies are dirty, if so resolve - // against the previous reactor state - let updatedReactorState = this.reactorState - if (!isKeyPath(keyPathOrGetter)) { - const maxCacheDepth = fns.getOption(updatedReactorState, 'maxCacheDepth') - let res = fns.resolveDirtyKeypathStates( - this.prevReactorState, - this.reactorState, - getCanonicalKeypathDeps(keyPathOrGetter, maxCacheDepth) - ) - updatedReactorState = res.reactorState - } + let result + + this.reactorState = this.reactorState.withMutations(reactorState => { + if (!isKeyPath(keyPathOrGetter)) { + // look through the keypathStates and see if any of the getters dependencies are dirty, if so resolve + // against the previous reactor state + const maxCacheDepth = fns.getOption(reactorState, 'maxCacheDepth') + fns.resolveDirtyKeypathStates( + this.prevReactorState, + reactorState, + getCanonicalKeypathDeps(keyPathOrGetter, maxCacheDepth) + ) + } + result = fns.evaluate(reactorState, keyPathOrGetter) + }) - let { result, reactorState } = fns.evaluate(updatedReactorState, keyPathOrGetter) - this.reactorState = reactorState return result } @@ -110,9 +109,9 @@ class Reactor { handler = getter getter = [] } - let entry = fns.addObserver(this.reactorState, this.observerState, getter, handler) + const entry = this.observerState.addObserver(this.reactorState, getter, handler) return () => { - fns.removeObserverByEntry(this.reactorState, this.observerState, entry) + this.observerState.removeObserverByEntry(this.reactorState, entry) } } @@ -124,7 +123,7 @@ class Reactor { throw new Error('Must call unobserve with a Getter') } - fns.removeObserver(this.reactorState, this.observerState, getter, handler) + this.observerState.removeObserver(this.reactorState, getter, handler) } /** @@ -227,15 +226,6 @@ class Reactor { this.observerState = new ObserverState() } - /** - * Denotes a new state, via a store registration, dispatch or some other method - * Resolves any outstanding keypath states and sets a new reactorState - * @private - */ - __nextState(newState) { - // TODO(jordan): determine if this is actually needed - } - /** * Notifies all change observers with the current state * @private @@ -246,29 +236,24 @@ class Reactor { return } + this.prevReactorState = this.prevReactorState.asMutable() + this.reactorState = this.reactorState.asMutable() + fns.getLoggerFunction(this.reactorState, 'notifyStart')(this.reactorState, this.observerState) - const keypathsToResolve = this.observerState.get('trackedKeypaths') - const { reactorState, changedKeypaths } = fns.resolveDirtyKeypathStates( + const keypathsToResolve = this.observerState.getTrackedKeypaths() + const changedKeypaths = fns.resolveDirtyKeypathStates( this.prevReactorState, this.reactorState, keypathsToResolve, true // increment all dirty states (this should leave no unknown state in the keypath tracker map): ) - this.reactorState = reactorState // get observers to notify based on the keypaths that changed - let observersToNotify = Immutable.Set().withMutations(set => { - changedKeypaths.forEach(keypath => { - const entries = this.observerState.getIn(['keypathToEntries', keypath]) - if (entries && entries.size > 0) { - set.union(entries) - } - }) - }) + const observersToNotify = this.observerState.getObserversToNotify(changedKeypaths) observersToNotify.forEach((observer) => { - if (!this.observerState.get('observers').has(observer)) { + if (!this.observerState.hasObserver(observer)) { // the observer was removed in a hander function return } @@ -279,23 +264,20 @@ class Reactor { fns.getLoggerFunction(this.reactorState, 'notifyEvaluateStart')(this.reactorState, getter) - const prevEvaluateResult = fns.evaluate(this.prevReactorState, getter) - const currEvaluateResult = fns.evaluate(this.reactorState, getter) + const prevValue = fns.evaluate(this.prevReactorState, getter) + const currValue = fns.evaluate(this.reactorState, getter) - this.prevReactorState = prevEvaluateResult.reactorState - this.reactorState = currEvaluateResult.reactorState - - const prevValue = prevEvaluateResult.result - const currValue = currEvaluateResult.result - - // TODO pull some comparator function out of the reactorState - if (!Immutable.is(prevValue, currValue)) { + // TODO(jordan) pull some comparator function out of the reactorState + if (!is(prevValue, currValue)) { handler.call(null, currValue) didCall = true } fns.getLoggerFunction(this.reactorState, 'notifyEvaluateEnd')(this.reactorState, getter, didCall, currValue) }) + this.prevReactorState = this.prevReactorState.asImmutable() + this.reactorState = this.reactorState.asImmutable() + fns.getLoggerFunction(this.reactorState, 'notifyEnd')(this.reactorState, this.observerState) } diff --git a/src/reactor/fns.js b/src/reactor/fns.js index a27271f..7b9139c 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -7,18 +7,6 @@ import { isEqual, isKeyPath } from '../key-path' import * as KeypathTracker from './keypath-tracker' import { each } from '../utils' -/** - * Immutable Types - */ -const EvaluateResult = Immutable.Record({ result: null, reactorState: null}) - -function evaluateResult(result, reactorState) { - return new EvaluateResult({ - result: result, - reactorState: reactorState, - }) -} - /** * @param {ReactorState} reactorState * @param {Object} stores @@ -132,78 +120,31 @@ export function dispatch(reactorState, actionType, payload) { * @return {ReactorState} */ export function loadState(reactorState, state) { - let dirtyStores = [] - const stateToLoad = toImmutable({}).withMutations(stateToLoad => { + reactorState = reactorState.asMutable() + let dirtyStores = Immutable.Set().asMutable() + + const stateToLoad = Immutable.Map({}).withMutations(stateToLoad => { each(state, (serializedStoreState, storeId) => { const store = reactorState.getIn(['stores', storeId]) if (store) { const storeState = store.deserialize(serializedStoreState) if (storeState !== undefined) { stateToLoad.set(storeId, storeState) - dirtyStores.push(storeId) + dirtyStores.add(storeId) } } }) }) - const dirtyStoresSet = Immutable.Set(dirtyStores) - return reactorState + reactorState .update('state', state => state.merge(stateToLoad)) .update('keypathStates', k => k.withMutations(keypathStates => { - dirtyStoresSet.forEach(storeId => { + dirtyStores.forEach(storeId => { KeypathTracker.changed(keypathStates, [storeId]) }) })) -} - -/** - * Adds a change observer whenever a certain part of the reactor state changes - * - * 1. observe(handlerFn) - 1 argument, called anytime reactor.state changes - * 2. observe(keyPath, handlerFn) same as above - * 3. observe(getter, handlerFn) called whenever any getter dependencies change with - * the value of the getter - * - * Adds a change handler whenever certain deps change - * If only one argument is passed invoked the handler whenever - * the reactor state changes - * - * @param {ReactorState} reactorState - * @param {ObserverState} observerState - * @param {KeyPath|Getter} getter - * @param {function} handler - * @return {ObserveResult} - */ -export function addObserver(reactorState, observerState, getter, handler) { - // use the passed in getter as the key so we can rely on a byreference call for unobserve - const rawGetter = getter - if (isKeyPath(getter)) { - getter = fromKeyPath(getter) - } - const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') - const keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) - const entry = Immutable.Map({ - getter: getter, - handler: handler, - }) - - const keypathToEntries = observerState.get('keypathToEntries') - - keypathDeps.forEach(keypath => { - if (!keypathToEntries.has(keypath)) { - keypathToEntries.set(keypath, Immutable.Set().asMutable().add(entry)) - } else { - keypathToEntries.get(keypath).add(entry) - } - }) - - const getterKey = createGetterKey(getter); - observerState.get('trackedKeypaths').union(keypathDeps) - observerState.get('observersMap').setIn([getterKey, handler], entry) - observerState.get('observers').add(entry) - - return entry + return reactorState.asImmutable() } /** @@ -219,81 +160,7 @@ export function getOption(reactorState, option) { return value } -/** - * Use cases - * removeObserver(observerState, []) - * removeObserver(observerState, [], handler) - * removeObserver(observerState, ['keyPath']) - * removeObserver(observerState, ['keyPath'], handler) - * removeObserver(observerState, getter) - * removeObserver(observerState, getter, handler) - * @param {ObserverState} observerState - * @param {KeyPath|Getter} getter - * @param {Function} handler - * @return {ObserverState} - */ -export function removeObserver(reactorState, observerState, getter, handler) { - if (isKeyPath(getter)) { - getter = fromKeyPath(getter) - } - let entriesToRemove; - const getterKey = createGetterKey(getter) - const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') - const keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) - if (handler) { - entriesToRemove = Immutable.List([ - observerState.getIn(['observersMap', getterKey, handler]), - ]) - } else { - entriesToRemove = observerState.getIn(['observersMap', getterKey], Immutable.Map({})).toList() - } - - entriesToRemove.forEach(entry => removeObserverByEntry(reactorState, map, entry, keypathDeps)) -} - -/** - * Removes an observer entry by id from the observerState - * @param {ObserverState} observerState - * @param {Immutable.Map} entry - * @return {ObserverState} - */ -export function removeObserverByEntry(reactorState, observerState, entry, keypathDeps = null) { - const getter = entry.get('getter') - if (!keypathDeps) { - const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') - keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) - } - - observerState.get('observers').remove(entry) - - // update the keypathToEntries - keypathDeps.forEach(keypath => { - const kp = ['keypathToEntries', keypath] - const entries = observerState.getIn(kp) - - if (entries) { - // check for observers being present because reactor.reset() can be called before an unwatch fn - entries.remove(entry) - if (entries.size === 0) { - observerState.removeIn(kp) - observerState.get('trackedKeypaths').remove(keypath) - } - } - }) - - // remove entry from observersobserverState - const getterKey = createGetterKey(getter) - const handler = entry.get('handler') - - const observersMap = observerState.get('observersMap') - observersMap.removeIn([getterKey, handler]) - // protect against unwatch after reset - if (observersMap.has(getterKey) && - observersMap.get(getterKey).size === 0) { - observersMap.remove(getterKey) - } -} /** * @param {ReactorState} reactorState @@ -342,7 +209,7 @@ export function resolveDirtyKeypathStates(prevReactorState, currReactorState, ke let changedKeypaths = []; - const newReactorState = currReactorState.update('keypathStates', k => k.withMutations(keypathStates => { + currReactorState.update('keypathStates', k => k.withMutations(keypathStates => { keypaths.forEach(keypath => { if (KeypathTracker.isClean(keypathStates, keypath)) { return @@ -362,26 +229,21 @@ export function resolveDirtyKeypathStates(prevReactorState, currReactorState, ke } })) - return { - changedKeypaths, - reactorState: newReactorState, - } + return changedKeypaths } /** + * This function must be called with mutable reactorState for performance reasons * @param {ReactorState} reactorState * @param {KeyPath|Gettter} keyPathOrGetter - * @return {EvaluateResult} + * @return {*} */ export function evaluate(reactorState, keyPathOrGetter) { const state = reactorState.get('state') if (isKeyPath(keyPathOrGetter)) { // if its a keyPath simply return - return evaluateResult( - state.getIn(keyPathOrGetter), - reactorState - ) + return state.getIn(keyPathOrGetter); } else if (!isGetter(keyPathOrGetter)) { throw new Error('evaluate must be passed a keyPath or Getter') } @@ -391,19 +253,17 @@ export function evaluate(reactorState, keyPathOrGetter) { let cacheEntry = cache.lookup(keyPathOrGetter) const isCacheMiss = !cacheEntry || isDirtyCacheEntry(reactorState, cacheEntry) if (isCacheMiss) { - const cacheResult = createCacheEntry(reactorState, keyPathOrGetter) - cacheEntry = cacheResult.entry - reactorState = cacheResult.reactorState + cacheEntry = createCacheEntry(reactorState, keyPathOrGetter) } - return evaluateResult( - cacheEntry.get('value'), - reactorState.update('cache', cache => { - return isCacheMiss - ? cache.miss(keyPathOrGetter, cacheEntry) - : cache.hit(keyPathOrGetter) - }) - ) + // TODO(jordan): respect the Getter's `shouldCache` setting + reactorState.update('cache', cache => { + return isCacheMiss + ? cache.miss(keyPathOrGetter, cacheEntry) + : cache.hit(keyPathOrGetter) + }) + + return cacheEntry.get('value') } /** @@ -452,15 +312,6 @@ function isDirtyCacheEntry(reactorState, cacheEntry) { }) } -/** - * Creates an immutable key for a getter - * @param {Getter} getter - * @return {Immutable.List} - */ -function createGetterKey(getter) { - return toImmutable(getter) -} - /** * Evaluates getter for given reactorState and returns CacheEntry * @param {ReactorState} reactorState @@ -469,20 +320,10 @@ function createGetterKey(getter) { */ function createCacheEntry(reactorState, getter) { // evaluate dependencies - const initial = { - reactorState: reactorState, - args: [], - } - // reduce here and capture updates to the ReactorState - const argResults = getDeps(getter).reduce((memo, dep) => { - const evaluateResult = evaluate(memo.reactorState, dep) - return { - reactorState: evaluateResult.get('reactorState'), - args: memo.args.concat(evaluateResult.get('result')), - } - }, initial) - const args = argResults.args - const newReactorState = argResults.reactorState + const args = getDeps(getter).reduce((memo, dep) => { + memo.push(evaluate(reactorState, dep)) + return memo + }, []) const value = getComputeFn(getter).apply(null, args) @@ -490,7 +331,7 @@ function createCacheEntry(reactorState, getter) { const keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) const keypathStates = reactorState.get('keypathStates') - const cacheStates = toImmutable({}).withMutations(map => { + const cacheStates = Immutable.Map({}).withMutations(map => { keypathDeps.forEach(keypath => { const keypathState = KeypathTracker.get(keypathStates, keypath) // The -1 case happens when evaluating soemthing against a previous reactorState @@ -500,14 +341,11 @@ function createCacheEntry(reactorState, getter) { }) }) - return { - reactorState: newReactorState, - entry: CacheEntry({ - value: value, - states: cacheStates, - dispatchId: reactorState.get('dispatchId'), - }) - } + return CacheEntry({ + value, + states: cacheStates, + dispatchId: reactorState.get('dispatchId'), + }) } /** diff --git a/src/reactor/observer-state.js b/src/reactor/observer-state.js new file mode 100644 index 0000000..1e482ee --- /dev/null +++ b/src/reactor/observer-state.js @@ -0,0 +1,189 @@ +import { Map, List, Set } from 'immutable' +import { getOption } from './fns' +import { fromKeyPath, getDeps, isGetter, getCanonicalKeypathDeps } from '../getter' +import { toImmutable } from '../immutable-helpers' +import { isKeyPath } from '../key-path' + +export default class ObserverState { + constructor() { + /* + { + : Set + } + */ + this.keypathToEntries = Map({}).asMutable() + + /* + { + : { + : + } + } + */ + this.observersMap = Map({}).asMutable() + + this.trackedKeypaths = Set().asMutable() + + // keep a flat set of observers to know when one is removed during a handler + this.observers = Set().asMutable() + } + + /** + * Adds a change observer whenever a certain part of the reactor state changes + * + * 1. observe(handlerFn) - 1 argument, called anytime reactor.state changes + * 2. observe(keyPath, handlerFn) same as above + * 3. observe(getter, handlerFn) called whenever any getter dependencies change with + * the value of the getter + * + * Adds a change handler whenever certain deps change + * If only one argument is passed invoked the handler whenever + * the reactor state changes + * + * @param {ReactorState} reactorState + * @param {KeyPath|Getter} getter + * @param {function} handler + * @return {ObserveResult} + */ + addObserver(reactorState, getter, handler) { + // use the passed in getter as the key so we can rely on a byreference call for unobserve + const rawGetter = getter + if (isKeyPath(getter)) { + // TODO(jordan): add a `dontCache` flag here so we dont waste caching overhead on simple keypath lookups + getter = fromKeyPath(getter) + } + + const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') + const keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) + const entry = Map({ + getter: getter, + handler: handler, + }) + + keypathDeps.forEach(keypath => { + if (!this.keypathToEntries.has(keypath)) { + this.keypathToEntries.set(keypath, Set().asMutable().add(entry)) + } else { + this.keypathToEntries.get(keypath).add(entry) + } + }) + + const getterKey = createGetterKey(getter); + + // union doesn't work with asMutable + this.trackedKeypaths = this.trackedKeypaths.union(keypathDeps) + this.observersMap.setIn([getterKey, handler], entry) + this.observers.add(entry) + + return entry + } + + /** + * Use cases + * removeObserver(observerState, []) + * removeObserver(observerState, [], handler) + * removeObserver(observerState, ['keyPath']) + * removeObserver(observerState, ['keyPath'], handler) + * removeObserver(observerState, getter) + * removeObserver(observerState, getter, handler) + * @param {ReactorState} reactorState + * @param {KeyPath|Getter} getter + * @param {Function} handler + * @return {ObserverState} + */ + removeObserver(reactorState, getter, handler) { + if (isKeyPath(getter)) { + getter = fromKeyPath(getter) + } + let entriesToRemove; + const getterKey = createGetterKey(getter) + const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') + const keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) + + if (handler) { + entriesToRemove = List([ + this.observersMap.getIn([getterKey, handler]), + ]) + } else { + entriesToRemove = this.observersMap.get(getterKey, Map({})).toList() + } + + entriesToRemove.forEach(entry => { + this.removeObserverByEntry(reactorState, entry, keypathDeps) + }) + } + + /** + * Removes an observer entry + * @param {ReactorState} reactorState + * @param {Immutable.Map} entry + * @param {Immutable.List|null} keypathDeps + * @return {ObserverState} + */ + removeObserverByEntry(reactorState, entry, keypathDeps = null) { + const getter = entry.get('getter') + if (!keypathDeps) { + const maxCacheDepth = getOption(reactorState, 'maxCacheDepth') + keypathDeps = getCanonicalKeypathDeps(getter, maxCacheDepth) + } + + this.observers.remove(entry) + + // update the keypathToEntries + keypathDeps.forEach(keypath => { + const entries = this.keypathToEntries.get(keypath) + + if (entries) { + // check for observers being present because reactor.reset() can be called before an unwatch fn + entries.remove(entry) + if (entries.size === 0) { + this.keypathToEntries.remove(keypath) + this.trackedKeypaths.remove(keypath) + } + } + }) + + // remove entry from observersobserverState + const getterKey = createGetterKey(getter) + const handler = entry.get('handler') + + this.observersMap.removeIn([getterKey, handler]) + // protect against unwatch after reset + if (this.observersMap.has(getterKey) && + this.observersMap.get(getterKey).size === 0) { + this.observersMap.remove(getterKey) + } + } + + getTrackedKeypaths() { + return this.trackedKeypaths.asImmutable() + } + + /** + * @param {Immutable.List} changedKeypaths + * @return {Entries[]} + */ + getObserversToNotify(changedKeypaths) { + return Set().withMutations(set => { + changedKeypaths.forEach(keypath => { + const entries = this.keypathToEntries.get(keypath) + if (entries && entries.size > 0) { + set.union(entries) + } + }) + }) + } + + hasObserver(observer) { + return this.observers.has(observer) + } +} + +/** + * Creates an immutable key for a getter + * @param {Getter} getter + * @return {Immutable.List} + */ +function createGetterKey(getter) { + return toImmutable(getter) +} diff --git a/src/reactor/records.js b/src/reactor/records.js index f06a3fe..29d3368 100644 --- a/src/reactor/records.js +++ b/src/reactor/records.js @@ -52,26 +52,3 @@ export const ReactorState = Record({ options: PROD_OPTIONS, }) -export const ObserverState = Record({ - /* - { - : Set - } - */ - keypathToEntries: Map({}).asMutable(), - - /* - { - : { - : - } - } - */ - observersMap: Map({}).asMutable(), - - trackedKeypaths: Set().asMutable(), - - // keep a flat set of observers to know when one is removed during a handler - observers: Set().asMutable(), -}) - diff --git a/tests/observer-state-tests.js b/tests/observer-state-tests.js new file mode 100644 index 0000000..420e19b --- /dev/null +++ b/tests/observer-state-tests.js @@ -0,0 +1,172 @@ +/*eslint-disable one-var, comma-dangle*/ +import { Map, Set, OrderedSet, List, is } from 'immutable' +import { Store } from '../src/main' +import * as fns from '../src/reactor/fns' +import * as KeypathTracker from '../src/reactor/keypath-tracker' +import { ReactorState } from '../src/reactor/records' +import ObserverState from '../src/reactor/observer-state' +import { toImmutable } from '../src/immutable-helpers' + +describe('ObserverState', () => { + beforeEach(() => { + jasmine.addCustomEqualityTester(is) + }) + + describe('#addObserver', () => { + let observerState, entry, handler, getter + + describe('when observing the identity getter', () => { + beforeEach(() => { + getter = [[], x => x] + handler = function() {} + const reactorState = new ReactorState() + + observerState = new ObserverState() + entry = observerState.addObserver(reactorState, getter, handler) + }) + + it('should properly update the observer state', () => { + expect(observerState.trackedKeypaths).toEqual(Set.of(List([]))) + + expect(observerState.observersMap).toEqual(Map().setIn( + [toImmutable(getter), handler], + Map({ getter, handler }) + )) + + expect(observerState.keypathToEntries).toEqual(Map([ + [toImmutable([]), Set.of(entry)] + ])) + + expect() + expect(observerState.observers).toEqual(Set.of(entry)) + }) + + it('should return a valid entry', () => { + expect(entry).toEqual(Map({ + getter: getter, + handler: handler, + })) + }) + }) + + describe('when observing a store backed getter', () => { + beforeEach(() => { + getter = [ + ['store1', 'prop1', 'prop2', 'prop3'], + ['store2'], + (a, b) => a + b + ] + handler = function() {} + + const reactorState = new ReactorState() + + observerState = new ObserverState() + entry = observerState.addObserver(reactorState, getter, handler) + }) + it('should properly update the observer state', () => { + expect(observerState.trackedKeypaths).toEqual(Set.of( + List(['store1', 'prop1', 'prop2']), + List(['store2']) + )) + + expect(observerState.observersMap).toEqual(Map().setIn( + [toImmutable(getter), handler], + Map({ getter, handler }) + )) + + expect(observerState.keypathToEntries).toEqual(Map([ + [toImmutable(['store1', 'prop1', 'prop2']), Set.of(entry)], + [toImmutable(['store2']), Set.of(entry)], + ])) + + expect(observerState.observers).toEqual(Set.of(entry)) + }) + it('should return a valid entry', () => { + const expected = Map({ + getter: getter, + handler: handler, + }) + expect(is(expected, entry)).toBe(true) + }) + }) + }) + + describe('#removeObserver', () => { + let reactorState, observerState, getter1, getter2, handler1, handler2, handler3 + + beforeEach(() => { + handler1 = () => 1 + handler2 = () => 2 + handler3 = () => 3 + + getter1 = [ + ['store1', 'prop1', 'prop2', 'prop3'], + ['store2'], + (a, b) => a + b + ] + getter2 = [[], x => x] + + reactorState = new ReactorState() + observerState = new ObserverState() + observerState.addObserver(reactorState, getter1, handler1) + observerState.addObserver(reactorState, getter1, handler2) + observerState.addObserver(reactorState, getter2, handler3) + }) + + describe('when removing by getter', () => { + it('should return a new ObserverState with all entries containing the getter removed', () => { + observerState.removeObserver(reactorState, getter1) + + expect(observerState.observersMap).toEqual(Map().setIn( + [toImmutable(getter2), handler3], + Map({ getter: getter2, handler: handler3 }) + )) + + const entry = Map({ + getter: getter2, + handler: handler3, + }) + expect(observerState.keypathToEntries).toEqual(Map().set(toImmutable([]), Set.of(entry))) + + expect(observerState.trackedKeypaths).toEqual(Set.of(toImmutable([]))) + + expect(observerState.observers).toEqual(Set.of(entry)) + }) + }) + + describe('when removing by getter / handler', () => { + it('should return a new ObserverState with all entries containing the getter removed', () => { + observerState.removeObserver(reactorState, getter1, handler1) + + const entry1 = Map({ getter: getter2, handler: handler3 }) + const entry2 = Map({ getter: getter1, handler: handler2 }) + expect(observerState.observersMap).toEqual(Map() + .setIn( + [toImmutable(getter2), handler3], + entry1 + ) + .setIn( + [toImmutable(getter1), handler2], + entry2 + )) + + + + const expectedKeypathToEntries = Map() + .set(toImmutable(['store1', 'prop1', 'prop2']), Set.of(Map({ getter: getter1, handler: handler2 }))) + .set(toImmutable(['store2']), Set.of(Map({ getter: getter1, handler: handler2 }))) + .set(toImmutable([]), Set.of(Map({ getter: getter2, handler: handler3 }))) + expect(observerState.keypathToEntries).toEqual(expectedKeypathToEntries) + + expect(observerState.trackedKeypaths).toEqual(Set.of( + toImmutable([]), + toImmutable(['store1', 'prop1', 'prop2']), + toImmutable(['store2']) + )) + + expect(observerState.observers).toEqual(Set.of(entry1, entry2)) + }) + }) + }) +}) +/*eslint-enable one-var, comma-dangle*/ diff --git a/tests/react-mixin-tests.js b/tests/react-mixin-tests.js index 6be2d47..8435158 100644 --- a/tests/react-mixin-tests.js +++ b/tests/react-mixin-tests.js @@ -189,7 +189,7 @@ describe('reactor.ReactMixin', () => { it('should unobserve all getters', () => { React.unmountComponentAtNode(mountNode) - expect(reactor.observerState.get('observersMap').size).toBe(0) + expect(reactor.observerState.observers.size).toBe(0) }) }) }) diff --git a/tests/reactor-fns-tests.js b/tests/reactor-fns-tests.js index 1c2227d..733c5c6 100644 --- a/tests/reactor-fns-tests.js +++ b/tests/reactor-fns-tests.js @@ -438,180 +438,6 @@ describe('reactor fns', () => { }) }) - describe('#addObserver', () => { - let initialObserverState, nextObserverState, entry, handler, getter, nextReactorState - - describe('when observing the identity getter', () => { - beforeEach(() => { - getter = [[], x => x] - handler = function() {} - const reactorState = new ReactorState() - - initialObserverState = new ObserverState() - const result = fns.addObserver(reactorState, initialObserverState, getter, handler) - nextObserverState = result.observerState - entry = result.entry - }) - it('should properly update the observer state', () => { - const expectedTrackedKeypaths = Set.of(List([])) - expect(is(expectedTrackedKeypaths, nextObserverState.get('trackedKeypaths'))).toBe(true) - - const expectedObserversMap = Map().setIn( - [toImmutable(getter), handler], - Map({ getter, handler }) - ) - expect(is(expectedObserversMap, nextObserverState.get('observersMap'))).toBe(true) - - const expectedKeypathToEntries = Map([ - [toImmutable([]), Set.of(entry)] - ]) - - expect(is(expectedKeypathToEntries, nextObserverState.get('keypathToEntries'))).toBe(true) - - const expectedObservers = Set.of(entry) - expect(is(expectedObservers, nextObserverState.get('observers'))).toBe(true) - }) - it('should return a valid entry', () => { - const expected = Map({ - getter: getter, - handler: handler, - }) - expect(is(expected, entry)).toBe(true) - }) - }) - - describe('when observing a store backed getter', () => { - beforeEach(() => { - getter = [ - ['store1', 'prop1', 'prop2', 'prop3'], - ['store2'], - (a, b) => a + b - ] - handler = function() {} - - const reactorState = new ReactorState() - initialObserverState = new ObserverState() - const result = fns.addObserver(reactorState, initialObserverState, getter, handler) - nextObserverState = result.observerState - entry = result.entry - }) - it('should properly update the observer state', () => { - const expectedTrackedKeypaths = Set.of( - List(['store1', 'prop1', 'prop2']), - List(['store2']) - ) - expect(is(expectedTrackedKeypaths, nextObserverState.get('trackedKeypaths'))).toBe(true) - - const expectedObserversMap = Map().setIn( - [toImmutable(getter), handler], - Map({ getter, handler }) - ) - expect(is(expectedObserversMap, nextObserverState.get('observersMap'))).toBe(true) - - const expectedKeypathToEntries = Map([ - [toImmutable(['store1', 'prop1', 'prop2']), Set.of(entry)], - [toImmutable(['store2']), Set.of(entry)], - ]) - expect(is(expectedKeypathToEntries, nextObserverState.get('keypathToEntries'))).toBe(true) - - const expectedObservers = Set.of(entry) - expect(is(expectedObservers, nextObserverState.get('observers'))).toBe(true) - }) - it('should return a valid entry', () => { - const expected = Map({ - getter: getter, - handler: handler, - }) - expect(is(expected, entry)).toBe(true) - }) - }) - }) - - describe('#removeObserver', () => { - let reactorState, initialObserverState, nextObserverState, getter1, getter2, handler1, handler2, handler3 - - beforeEach(() => { - handler1 = () => 1 - handler2 = () => 2 - handler3 = () => 3 - - getter1 = [ - ['store1', 'prop1', 'prop2', 'prop3'], - ['store2'], - (a, b) => a + b - ] - getter2 = [[], x => x] - - reactorState = new ReactorState() - const initialObserverState1 = new ObserverState() - const result1 = fns.addObserver(reactorState, initialObserverState1, getter1, handler1) - const initialObserverState2 = result1.observerState - const result2 = fns.addObserver(reactorState,initialObserverState2, getter1, handler2) - const initialObserverState3 = result2.observerState - const result3 = fns.addObserver(reactorState,initialObserverState3, getter2, handler3) - initialObserverState = result3.observerState - }) - - describe('when removing by getter', () => { - it('should return a new ObserverState with all entries containing the getter removed', () => { - const result = fns.removeObserver(reactorState, initialObserverState, getter1) - - const expectedObserversMap = Map().setIn( - [toImmutable(getter2), handler3], - Map({ getter: getter2, handler: handler3 }) - ) - expect(is(expectedObserversMap, result.get('observersMap'))).toBe(true) - - const entry = Map({ - getter: getter2, - handler: handler3, - }) - const expectedKeypathToEntries = Map().set(toImmutable([]), Set.of(entry)) - expect(is(expectedKeypathToEntries, result.get('keypathToEntries'))).toBe(true) - - const expectedTrackedKeypaths = Set.of(toImmutable([])) - expect(is(expectedTrackedKeypaths, result.get('trackedKeypaths'))).toBe(true) - - const expectedObservers = Set.of(entry) - expect(is(expectedObservers, result.get('observers'))).toBe(true) - }) - }) - - describe('when removing by getter / handler', () => { - it('should return a new ObserverState with all entries containing the getter removed', () => { - const result = fns.removeObserver(reactorState, initialObserverState, getter1, handler1) - - const entry1 = Map({ getter: getter2, handler: handler3 }) - const entry2 = Map({ getter: getter1, handler: handler2 }) - const expectedObserversMap = Map() - .setIn( - [toImmutable(getter2), handler3], - entry1 - ) - .setIn( - [toImmutable(getter1), handler2], - entry2 - ) - expect(is(expectedObserversMap, result.get('observersMap'))).toBe(true) - - const expectedKeypathToEntries = Map() - .set(toImmutable(['store1', 'prop1', 'prop2']), Set.of(Map({ getter: getter1, handler: handler2 }))) - .set(toImmutable(['store2']), Set.of(Map({ getter: getter1, handler: handler2 }))) - .set(toImmutable([]), Set.of(Map({ getter: getter2, handler: handler3 }))) - expect(is(expectedKeypathToEntries, result.get('keypathToEntries'))).toBe(true) - - const expectedTrackedKeypaths = Set.of( - toImmutable([]), - toImmutable(['store1', 'prop1', 'prop2']), - toImmutable(['store2']) - ) - expect(is(expectedTrackedKeypaths, result.get('trackedKeypaths'))).toBe(true) - - const expectedObservers = Set.of(entry1, entry2) - expect(is(expectedObservers, result.get('observers'))).toBe(true) - }) - }) - }) describe('#getDebugOption', () => { it('should parse the option value in a reactorState', () => { const reactorState = new ReactorState({ From cc2ccb791240802fd4391ca07c4e931b6a1deb0e Mon Sep 17 00:00:00 2001 From: jordangarcia Date: Sat, 1 Oct 2016 22:46:50 -0700 Subject: [PATCH 6/6] Fix reset --- src/reactor/cache.js | 13 +++++++++++++ src/reactor/fns.js | 37 ++++++++++++++++------------------- tests/reactor-fns-tests.js | 40 +++++++++++++++++++++----------------- 3 files changed, 52 insertions(+), 38 deletions(-) diff --git a/src/reactor/cache.js b/src/reactor/cache.js index 2be3f96..85c88cb 100644 --- a/src/reactor/cache.js +++ b/src/reactor/cache.js @@ -108,6 +108,10 @@ export class BasicCache { return new BasicCache(newCache) } + + empty() { + return new BasicCache() + } } const DEFAULT_LRU_LIMIT = 1000 @@ -224,6 +228,15 @@ export class LRUCache { this.lru.remove(item) ) } + + empty() { + return new LRUCache( + this.limit, + this.evictCount, + this.cache.empty(), + OrderedSet() + ) + } } /** diff --git a/src/reactor/fns.js b/src/reactor/fns.js index 7b9139c..8ae3e02 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -160,35 +160,32 @@ export function getOption(reactorState, option) { return value } - - /** * @param {ReactorState} reactorState * @return {ReactorState} */ export function reset(reactorState) { - const prevState = reactorState.get('state') + const storeMap = reactorState.get('stores') return reactorState.withMutations(reactorState => { - const storeMap = reactorState.get('stores') - const storeIds = storeMap.keySeq().toJS() - storeMap.forEach((store, id) => { - const storeState = prevState.get(id) - const resetStoreState = store.handleReset(storeState) - if (resetStoreState === undefined && getOption(reactorState, 'throwOnUndefinedStoreReturnValue')) { - throw new Error('Store handleReset() must return a value, did you forget a return statement') - } - if (getOption(reactorState, 'throwOnNonImmutableStore') && !isImmutableValue(resetStoreState)) { - throw new Error('Store reset state must be an immutable value, did you forget to call toImmutable') - } - reactorState.setIn(['state', id], resetStoreState) - }) - - reactorState.update('keypathStates', k => k.withMutations(keypathStates => { - storeIds.forEach(id => { - KeypathTracker.changed(keypathStates, [id]) + // update state + reactorState.update('state', s => s.withMutations(state => { + storeMap.forEach((store, id) => { + const storeState = state.get(id) + const resetStoreState = store.handleReset(storeState) + if (resetStoreState === undefined && getOption(reactorState, 'throwOnUndefinedStoreReturnValue')) { + throw new Error('Store handleReset() must return a value, did you forget a return statement') + } + if (getOption(reactorState, 'throwOnNonImmutableStore') && !isImmutableValue(resetStoreState)) { + throw new Error('Store reset state must be an immutable value, did you forget to call toImmutable') + } + state.set(id, resetStoreState) }) })) + + reactorState.set('keypathStates', new KeypathTracker.RootNode()) + reactorState.set('dispatchId', 1) + reactorState.update('cache', cache => cache.empty()) }) } diff --git a/tests/reactor-fns-tests.js b/tests/reactor-fns-tests.js index 733c5c6..e5aea5e 100644 --- a/tests/reactor-fns-tests.js +++ b/tests/reactor-fns-tests.js @@ -5,10 +5,15 @@ import * as fns from '../src/reactor/fns' import * as KeypathTracker from '../src/reactor/keypath-tracker' import { ReactorState, ObserverState } from '../src/reactor/records' import { toImmutable } from '../src/immutable-helpers' +import { DefaultCache } from '../src/reactor/cache' const status = KeypathTracker.status describe('reactor fns', () => { + beforeEach(() => { + jasmine.addCustomEqualityTester(is) + }) + describe('#registerStores', () => { let reactorState let store1 @@ -374,7 +379,13 @@ describe('reactor fns', () => { let initialReactorState, nextReactorState, store1, store2 beforeEach(() => { - const reactorState = new ReactorState() + const cache = DefaultCache() + cache.miss('key', 'value') + + const reactorState = new ReactorState({ + cache: DefaultCache(), + }) + store1 = new Store({ getInitialState() { return toImmutable({ @@ -415,25 +426,18 @@ describe('reactor fns', () => { expect(is(expected, result)).toBe(true) }) + it('should empty the cache', () => { + const cache = nextReactorState.get('cache') + expect(cache.asMap()).toEqual(Map({})) + }) + + it('reset the dispatchId', () => { + expect(nextReactorState.get('dispatchId')).toBe(1) + }) + it('should update keypathStates', () => { const result = nextReactorState.get('keypathStates') - const expected = new KeypathTracker.RootNode({ - changedPaths: Set.of(List(['store1']), List(['store2'])), - state: 6, - status: status.DIRTY, - children: toImmutable({ - store1: { - state: 3, - status: status.DIRTY, - children: {}, - }, - store2: { - state: 2, - status: status.DIRTY, - children: {}, - }, - }), - }) + const expected = new KeypathTracker.RootNode() expect(is(result, expected)).toBe(true) }) })