From ae66da73556d8d40dfb08312df344d8a89045386 Mon Sep 17 00:00:00 2001 From: loganlinn Date: Sat, 30 Jan 2016 16:47:24 -0800 Subject: [PATCH 1/8] FIx duplicate keys in docs/package.json --- docs/package.json | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/package.json b/docs/package.json index de8b5a8..8da5885 100644 --- a/docs/package.json +++ b/docs/package.json @@ -23,8 +23,6 @@ "nuclear-js": "^1.0.5", "webpack": "^1.9.11", "webpack-dev-server": "^1.9.0", - "grunt-concurrent": "^1.0.0", - "grunt-contrib-connect": "^0.10.1", "remarkable": "^1.6.0", "front-matter": "^1.0.0", "glob": "^5.0.10", From 90ea034cf7b44dc84aacbbb08bc274efa94c894e Mon Sep 17 00:00:00 2001 From: loganlinn Date: Sun, 31 Jan 2016 01:19:16 -0800 Subject: [PATCH 2/8] Add caching abstraction Introduce a functional caching abstraction. The purpose is to provide extensible API with abstractions that are composable. Includes implementations for basic cache strategy (current) and an LRU cache (which wraps BasicCache). A custom caching strategy can be injected via Reactor constructor. All existing caching behavior remains the same (for now). --- src/reactor.js | 2 + src/reactor/cache.js | 208 +++++++++++++++++++++++++++++++++++++++++ src/reactor/fns.js | 94 ++++++------------- src/reactor/records.js | 3 +- tests/cache-tests.js | 126 +++++++++++++++++++++++++ 5 files changed, 368 insertions(+), 65 deletions(-) create mode 100644 src/reactor/cache.js create mode 100644 tests/cache-tests.js diff --git a/src/reactor.js b/src/reactor.js index ce46ecb..9147810 100644 --- a/src/reactor.js +++ b/src/reactor.js @@ -1,6 +1,7 @@ import Immutable from 'immutable' import createReactMixin from './create-react-mixin' import * as fns from './reactor/fns' +import { DefaultCache } from './reactor/cache' import { isKeyPath } from './key-path' import { isGetter } from './getter' import { toJS } from './immutable-helpers' @@ -27,6 +28,7 @@ class Reactor { const baseOptions = debug ? DEBUG_OPTIONS : PROD_OPTIONS const initialReactorState = new ReactorState({ debug: debug, + cache: config.cache || DefaultCache(), // merge config options with the defaults options: baseOptions.merge(config.options || {}), }) diff --git a/src/reactor/cache.js b/src/reactor/cache.js new file mode 100644 index 0000000..a0df9ae --- /dev/null +++ b/src/reactor/cache.js @@ -0,0 +1,208 @@ +import { List, Map, OrderedMap, Record } from 'immutable' + +export const CacheEntry = Record({ + value: null, + storeStates: Map(), + dispatchId: null, +}) + +/******************************************************************************* + * interface PersistentCache { + * has(item) + * lookup(item, notFoundValue) + * hit(item) + * miss(item, entry) + * evict(item) + * asMap() + * } + *******************************************************************************/ + +/** + * Plain map-based cache + */ +export class BasicCache { + + /** + * @param {Immutable.Map} cache + */ + constructor(cache = Map()) { + this.cache = cache; + } + + /** + * Retrieve the associated value, if it exists in this cache, otherwise + * returns notFoundValue (or undefined if not provided) + * @param {Object} item + * @param {Object?} notFoundValue + * @return {CacheEntry?} + */ + lookup(item, notFoundValue) { + return this.cache.get(item, notFoundValue) + } + + /** + * Checks if this cache contains an associated value + * @param {Object} item + * @return {boolean} + */ + has(item) { + return this.cache.has(item) + } + + /** + * Return cached items as map + * @return {Immutable.Map} + */ + asMap() { + return this.cache + } + + /** + * Updates this cache when it is determined to contain the associated value + * @param {Object} item + * @return {BasicCache} + */ + hit(item) { + return this; + } + + /** + * Updates this cache when it is determined to **not** contain the associated value + * @param {Object} item + * @param {CacheEntry} entry + * @return {BasicCache} + */ + miss(item, entry) { + return new BasicCache( + this.cache.update(item, existingEntry => { + if (existingEntry && existingEntry.dispatchId > entry.dispatchId) { + throw new Error("Refusing to cache older value") + } + return entry + }) + ) + } + + /** + * Removes entry from cache + * @param {Object} item + * @return {BasicCache} + */ + evict(item) { + return new BasicCache(this.cache.remove(item)) + } +} + +/** + * Implements caching strategy that evicts least-recently-used items in cache + * when an item is being added to a cache that has reached a configured size + * limit. + */ +export class LRUCache { + + constructor(limit = 1000, cache = new BasicCache(), lru = OrderedMap(), tick = 0) { + this.limit = limit; + this.cache = cache; + this.lru = lru; + this.tick = tick; + } + + /** + * Retrieve the associated value, if it exists in this cache, otherwise + * returns notFoundValue (or undefined if not provided) + * @param {Object} item + * @param {Object?} notFoundValue + * @return {CacheEntry} + */ + lookup(item, notFoundValue) { + return this.cache.lookup(item, notFoundValue) + } + + /** + * Checks if this cache contains an associated value + * @param {Object} item + * @return {boolean} + */ + has(item) { + return this.cache.has(item) + } + + /** + * Return cached items as map + * @return {Immutable.Map} + */ + asMap() { + return this.cache.asMap() + } + + /** + * Updates this cache when it is determined to contain the associated value + * @param {Object} item + * @return {LRUCache} + */ + hit(item) { + const nextTick = this.tick + 1; + + const lru = this.cache.lookup(item) ? + this.lru.remove(item).set(item, nextTick) : + this.lru; + + return new LRUCache(this.limit, this.cache, lru, nextTick) + } + + /** + * Updates this cache when it is determined to **not** contain the associated value + * If cache has reached size limit, the LRU item is evicted. + * @param {Object} item + * @param {CacheEntry} entry + * @return {LRUCache} + */ + miss(item, entry) { + const nextTick = this.tick + 1; + + if (this.lru.size >= this.limit) { + // TODO add options to clear multiple items at once + const evictItem = this.has(item) ? item : this.lru.keySeq().first() + + return new LRUCache( + this.limit, + this.cache.evict(evictItem).miss(item, entry), + this.lru.remove(evictItem).set(item, nextTick), + nextTick + ) + } else { + return new LRUCache( + this.limit, + this.cache.miss(item, entry), + this.lru.set(item, nextTick), + nextTick + ) + } + } + + /** + * Removes entry from cache + * @param {Object} item + * @return {LRUCache} + */ + evict(item) { + if (!this.cache.has(item)) { + return this; + } + + return new LRUCache( + this.limit, + this.cache.evict(item), + this.lru.remove(item), + this.tick + 1 + ) + } +} + +/** + * Returns default cache strategy + * @return {BasicCache} + */ +export function DefaultCache() { + return new BasicCache() +} diff --git a/src/reactor/fns.js b/src/reactor/fns.js index 7f47527..c5e6945 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -1,5 +1,6 @@ import Immutable from 'immutable' import logging from '../logging' +import { CacheEntry } from './cache' import { isImmutableValue } from '../immutable-helpers' import { toImmutable } from '../immutable-helpers' import { fromKeyPath, getStoreDeps, getComputeFn, getDeps, isGetter } from '../getter' @@ -330,22 +331,21 @@ export function evaluate(reactorState, keyPathOrGetter) { } // Must be a Getter - // if the value is cached for this dispatch cycle, return the cached value - if (isCached(reactorState, keyPathOrGetter)) { - // Cache hit - return evaluateResult( - getCachedValue(reactorState, keyPathOrGetter), - reactorState - ) - } - // evaluate dependencies - const args = getDeps(keyPathOrGetter).map(dep => evaluate(reactorState, dep).result) - const evaluatedValue = getComputeFn(keyPathOrGetter).apply(null, args) + const cache = reactorState.get('cache') + var cacheEntry = cache.lookup(keyPathOrGetter) + const isCacheMiss = !cacheEntry + if (isCacheMiss || isDirtyCacheEntry(reactorState, cacheEntry)) { + cacheEntry = createCacheEntry(reactorState, keyPathOrGetter) + } return evaluateResult( - evaluatedValue, - cacheValue(reactorState, keyPathOrGetter, evaluatedValue) + cacheEntry.get('value'), + reactorState.update('cache', cache => { + return isCacheMiss ? + cache.miss(keyPathOrGetter, cacheEntry) : + cache.hit(keyPathOrGetter) + }) ) } @@ -375,57 +375,31 @@ export function resetDirtyStores(reactorState) { return reactorState.set('dirtyStores', Immutable.Set()) } -/** - * Currently cache keys are always getters by reference - * @param {Getter} getter - * @return {Getter} - */ -function getCacheKey(getter) { - return getter -} - /** * @param {ReactorState} reactorState - * @param {Getter|KeyPath} keyPathOrGetter - * @return {Immutable.Map} + * @param {CacheEntry} cacheEntry + * @return {boolean} */ -function getCacheEntry(reactorState, keyPathOrGetter) { - const key = getCacheKey(keyPathOrGetter) - return reactorState.getIn(['cache', key]) -} +function isDirtyCacheEntry(reactorState, cacheEntry) { + const storeStates = cacheEntry.get('storeStates') -/** - * @param {ReactorState} reactorState - * @param {Getter} getter - * @return {Boolean} - */ -function isCached(reactorState, keyPathOrGetter) { - const entry = getCacheEntry(reactorState, keyPathOrGetter) - if (!entry) { - return false - } - - const storeStates = entry.get('storeStates') - if (storeStates.size === 0) { - // if there are no store states for this entry then it was never cached before - return false - } - - return storeStates.every((stateId, storeId) => { - return reactorState.getIn(['storeStates', storeId]) === stateId + // 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 }) } /** - * Caches the value of a getter given state, getter, args, value + * Evaluates getter for given reactorState and returns CacheEntry * @param {ReactorState} reactorState * @param {Getter} getter - * @param {*} value - * @return {ReactorState} + * @return {CacheEntry} */ -function cacheValue(reactorState, getter, value) { - const cacheKey = getCacheKey(getter) - const dispatchId = reactorState.get('dispatchId') +function createCacheEntry(reactorState, getter) { + // evaluate dependencies + const args = getDeps(getter).map(dep => evaluate(reactorState, dep).result) + const value = getComputeFn(getter).apply(null, args) + const storeDeps = getStoreDeps(getter) const storeStates = toImmutable({}).withMutations(map => { storeDeps.forEach(storeId => { @@ -434,19 +408,11 @@ function cacheValue(reactorState, getter, value) { }) }) - return reactorState.setIn(['cache', cacheKey], Immutable.Map({ + return CacheEntry({ value: value, storeStates: storeStates, - dispatchId: dispatchId, - })) -} - -/** - * Pulls out the cached value for a getter - */ -function getCachedValue(reactorState, getter) { - const key = getCacheKey(getter) - return reactorState.getIn(['cache', key, 'value']) + dispatchId: reactorState.get('dispatchId'), + }) } /** diff --git a/src/reactor/records.js b/src/reactor/records.js index a59314f..af24c43 100644 --- a/src/reactor/records.js +++ b/src/reactor/records.js @@ -1,4 +1,5 @@ import { Map, Set, Record } from 'immutable' +import { DefaultCache } from './cache' export const PROD_OPTIONS = Map({ // logs information for each dispatch @@ -38,7 +39,7 @@ export const ReactorState = Record({ dispatchId: 0, state: Map(), stores: Map(), - cache: Map(), + cache: DefaultCache(), // maintains a mapping of storeId => state id (monotomically increasing integer whenever store state changes) storeStates: Map(), dirtyStores: Set(), diff --git a/tests/cache-tests.js b/tests/cache-tests.js new file mode 100644 index 0000000..920d251 --- /dev/null +++ b/tests/cache-tests.js @@ -0,0 +1,126 @@ +import Immutable, { Map } from 'immutable' +import { toImmutable } from '../src/immutable-helpers' +import { BasicCache, LRUCache } from '../src/reactor/cache' + + +describe('Cache', () => { + describe('LRUCache', () => { + var cache + + beforeEach(() => { + jasmine.addCustomEqualityTester(Immutable.is) + }) + + it('should evict least recently used', () => { + cache = new LRUCache(3) + + expect(cache.asMap().isEmpty()).toBe(true) + + var a = {foo: "bar"} + var b = {bar: "baz"} + var c = {baz: "foo"} + + cache = cache.miss('a', a) + + expect(cache.asMap()).toEqual(Map({a: a})) + expect(cache.has('a')).toBe(true) + expect(cache.lookup('a')).toBe(a) + + cache = cache.miss('b', b) + expect(cache.asMap()).toEqual(Map({a: a, b: b})) + expect(cache.has('b')).toBe(true) + expect(cache.lookup('b')).toBe(b) + + cache = cache.miss('c', c) + expect(cache.asMap()).toEqual(Map({a: a, b: b, c: c})) + expect(cache.has('c')).toBe(true) + expect(cache.lookup('c')).toBe(c) + + expect(cache.has('d')).toBe(false) + expect(cache.lookup('d')).not.toBeDefined() + + var notFound = {found: false} + expect(cache.lookup('d', notFound)).toBe(notFound) + + cache = cache.miss('d', 4) + + expect(cache.asMap()).toEqual(Map({b: b, c: c, d: 4}), 'a should have been evicted') + + cache = cache.hit('b') // Touch b so its not LRU + + expect(cache.asMap()).toEqual(Map({b: b, c: c, d: 4}), 'should not have have changed') + + cache = cache.miss('e', 5) + + expect(cache.asMap()).toEqual(Map({b: b, d: 4, e: 5}), 'should have have changed') + + cache = cache.hit('b') + .hit('e') + .hit('d') + .miss('a', 1) + + expect(cache.asMap()).toEqual(Map({a: 1, d: 4, e: 5}), 'should have have changed') + }) + + it('should maintain LRU after manual evict', () => { + cache = new LRUCache(3) + .miss('a', 'A') + .miss('b', 'B') + .miss('c', 'C') + + expect(cache.asMap()).toEqual(Map({a: 'A', b: 'B', c: 'C'})) + + cache = cache.evict('a') + expect(cache.asMap()).toEqual(Map({b: 'B', c: 'C'})) + + cache = cache.miss('d', 'D') + expect(cache.asMap()).toEqual(Map({b: 'B', c: 'C', d: 'D'})) + }) + + it('should not evict if under limit', () => { + cache = new LRUCache(2) + .miss('a', 1) + .miss('b', 2) + .miss('b', 3) + + expect(cache.asMap()).toEqual(Map({a: 1, b: 3})) + cache = cache.miss('a', 4) + expect(cache.asMap()).toEqual(Map({a: 4, b: 3})) + + cache = new LRUCache(3) + .miss('a', 1) + .miss('b', 2) + .miss('b', 3) + .miss('c', 4) + .miss('d', 5) + .miss('e', 6) + + expect(cache.asMap()).toEqual(Map({e: 6, d: 5, c: 4})) + }) + + it('should not evict if hitting unknown items', () => { + cache = new LRUCache(2) + .hit('x') + .hit('y') + .hit('z') + .miss('a', 1) + .miss('b', 2) + .miss('c', 3) + .miss('d', 4) + .miss('e', 5) + + expect(cache.asMap()).toEqual(Map({d: 4, e: 5})) + + cache = new LRUCache(2) + .hit('x') + .hit('y') + .miss('a', 1) + .miss('b', 2) + .miss('y', 3) + .miss('x', 4) + .miss('e', 5) + + expect(cache.asMap()).toEqual(Map({x: 4, e: 5})) + }) + }) +}) From 137bfd6474c0709bf84cafd970aab20bdab77ffc Mon Sep 17 00:00:00 2001 From: loganlinn Date: Sun, 31 Jan 2016 13:46:28 -0800 Subject: [PATCH 3/8] Add comment about remove+set on LRU --- src/reactor/cache.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/reactor/cache.js b/src/reactor/cache.js index a0df9ae..c4c624c 100644 --- a/src/reactor/cache.js +++ b/src/reactor/cache.js @@ -143,6 +143,7 @@ export class LRUCache { hit(item) { const nextTick = this.tick + 1; + // if item exists, remove it first to reorder in lru OrderedMap const lru = this.cache.lookup(item) ? this.lru.remove(item).set(item, nextTick) : this.lru; From ba8b64b99976d2b06f30496ddc0dd9bf393dbbd9 Mon Sep 17 00:00:00 2001 From: loganlinn Date: Sun, 31 Jan 2016 14:01:09 -0800 Subject: [PATCH 4/8] Replace OrderedMap with OrderedSet We weren't using notion of monoatomically increasing tick in LRU cache because an OrderedMap does not re-order exising items that are set(), so we depend on remove()+set(). This means the value of OrderedMap isn't used and simplifies to OrderedSet. --- src/reactor/cache.js | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/src/reactor/cache.js b/src/reactor/cache.js index c4c624c..827f929 100644 --- a/src/reactor/cache.js +++ b/src/reactor/cache.js @@ -1,4 +1,4 @@ -import { List, Map, OrderedMap, Record } from 'immutable' +import { List, Map, OrderedSet, Record } from 'immutable' export const CacheEntry = Record({ value: null, @@ -100,11 +100,10 @@ export class BasicCache { */ export class LRUCache { - constructor(limit = 1000, cache = new BasicCache(), lru = OrderedMap(), tick = 0) { + constructor(limit = 1000, cache = new BasicCache(), lru = OrderedSet()) { this.limit = limit; this.cache = cache; this.lru = lru; - this.tick = tick; } /** @@ -141,14 +140,12 @@ export class LRUCache { * @return {LRUCache} */ hit(item) { - const nextTick = this.tick + 1; - - // if item exists, remove it first to reorder in lru OrderedMap + // if item exists, remove it first to reorder in lru OrderedSet const lru = this.cache.lookup(item) ? - this.lru.remove(item).set(item, nextTick) : + this.lru.remove(item).add(item) : this.lru; - return new LRUCache(this.limit, this.cache, lru, nextTick) + return new LRUCache(this.limit, this.cache, lru) } /** @@ -159,24 +156,20 @@ export class LRUCache { * @return {LRUCache} */ miss(item, entry) { - const nextTick = this.tick + 1; - if (this.lru.size >= this.limit) { // TODO add options to clear multiple items at once - const evictItem = this.has(item) ? item : this.lru.keySeq().first() + const evictItem = this.has(item) ? item : this.lru.first() return new LRUCache( this.limit, this.cache.evict(evictItem).miss(item, entry), - this.lru.remove(evictItem).set(item, nextTick), - nextTick + this.lru.remove(evictItem).add(item) ) } else { return new LRUCache( this.limit, this.cache.miss(item, entry), - this.lru.set(item, nextTick), - nextTick + this.lru.add(item) ) } } @@ -194,8 +187,7 @@ export class LRUCache { return new LRUCache( this.limit, this.cache.evict(item), - this.lru.remove(item), - this.tick + 1 + this.lru.remove(item) ) } } From bfcb9a10491df329147270cc82290117a60fecb7 Mon Sep 17 00:00:00 2001 From: loganlinn Date: Mon, 1 Feb 2016 17:14:29 -0800 Subject: [PATCH 5/8] Don't create new LRUCache when not needed If you call hit() on an item that's not actually in the LRUCache (edge case), it's a no-op, so we can just return this. --- src/reactor/cache.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/reactor/cache.js b/src/reactor/cache.js index 827f929..ca3d31a 100644 --- a/src/reactor/cache.js +++ b/src/reactor/cache.js @@ -140,12 +140,12 @@ export class LRUCache { * @return {LRUCache} */ hit(item) { - // if item exists, remove it first to reorder in lru OrderedSet - const lru = this.cache.lookup(item) ? - this.lru.remove(item).add(item) : - this.lru; + if (!this.cache.has(item)) { + return this; + } - return new LRUCache(this.limit, this.cache, lru) + // remove it first to reorder in lru OrderedSet + return new LRUCache(this.limit, this.cache, this.lru.remove(item).add(item)) } /** From 3a4eafbf7391b7b6dd34a757dc260f1b0464388f Mon Sep 17 00:00:00 2001 From: loganlinn Date: Mon, 1 Feb 2016 18:39:51 -0800 Subject: [PATCH 6/8] Add ability to LRUCache to evict many at once --- src/reactor/cache.js | 30 ++++++++++++++++++++++++------ tests/cache-tests.js | 17 +++++++++++++++-- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/reactor/cache.js b/src/reactor/cache.js index ca3d31a..81de0d7 100644 --- a/src/reactor/cache.js +++ b/src/reactor/cache.js @@ -93,6 +93,9 @@ export class BasicCache { } } +const DEFAULT_LRU_LIMIT = 1000 +const DEFAULT_LRU_EVICT_COUNT = 1 + /** * Implements caching strategy that evicts least-recently-used items in cache * when an item is being added to a cache that has reached a configured size @@ -100,8 +103,9 @@ export class BasicCache { */ export class LRUCache { - constructor(limit = 1000, cache = new BasicCache(), lru = OrderedSet()) { + constructor(limit = DEFAULT_LRU_LIMIT, evictCount = DEFAULT_LRU_EVICT_COUNT, cache = new BasicCache(), lru = OrderedSet()) { this.limit = limit; + this.evictCount = evictCount this.cache = cache; this.lru = lru; } @@ -145,7 +149,7 @@ export class LRUCache { } // remove it first to reorder in lru OrderedSet - return new LRUCache(this.limit, this.cache, this.lru.remove(item).add(item)) + return new LRUCache(this.limit, this.evictCount, this.cache, this.lru.remove(item).add(item)) } /** @@ -157,17 +161,30 @@ export class LRUCache { */ miss(item, entry) { if (this.lru.size >= this.limit) { - // TODO add options to clear multiple items at once - const evictItem = this.has(item) ? item : this.lru.first() + if (this.has(item)) { + return new LRUCache( + this.limit, + this.evictCount, + this.cache.miss(item, entry), + this.lru.remove(item).add(item) + ) + } + + const cache = (this.lru + .take(this.evictCount) + .reduce((c, evictItem) => c.evict(evictItem), this.cache) + .miss(item, entry)); return new LRUCache( this.limit, - this.cache.evict(evictItem).miss(item, entry), - this.lru.remove(evictItem).add(item) + this.evictCount, + cache, + this.lru.skip(this.evictCount).add(item) ) } else { return new LRUCache( this.limit, + this.evictCount, this.cache.miss(item, entry), this.lru.add(item) ) @@ -186,6 +203,7 @@ export class LRUCache { return new LRUCache( this.limit, + this.evictCount, this.cache.evict(item), this.lru.remove(item) ) diff --git a/tests/cache-tests.js b/tests/cache-tests.js index 920d251..005eb28 100644 --- a/tests/cache-tests.js +++ b/tests/cache-tests.js @@ -52,14 +52,14 @@ describe('Cache', () => { cache = cache.miss('e', 5) - expect(cache.asMap()).toEqual(Map({b: b, d: 4, e: 5}), 'should have have changed') + expect(cache.asMap()).toEqual(Map({b: b, d: 4, e: 5}), 'should have changed') cache = cache.hit('b') .hit('e') .hit('d') .miss('a', 1) - expect(cache.asMap()).toEqual(Map({a: 1, d: 4, e: 5}), 'should have have changed') + expect(cache.asMap()).toEqual(Map({a: 1, d: 4, e: 5}), 'should have changed') }) it('should maintain LRU after manual evict', () => { @@ -122,5 +122,18 @@ describe('Cache', () => { expect(cache.asMap()).toEqual(Map({x: 4, e: 5})) }) + + it('should be able to evict multiple LRU items at once', () => { + cache = new LRUCache(4, 2) + .miss('a', 1) + .miss('b', 2) + .miss('c', 3) + .miss('d', 4) + .miss('e', 5) + + expect(cache.asMap()).toEqual(Map({c: 3, d: 4, e: 5})) + expect(cache.miss('f', 6).asMap()).toEqual(Map({c: 3, d: 4, e: 5, f: 6})) + expect(cache.miss('f', 6).miss('g', 7).asMap()).toEqual(Map({e: 5, f: 6, g: 7})) + }) }) }) From 2eefde899285b792419709d7a3d2afce315042e1 Mon Sep 17 00:00:00 2001 From: loganlinn Date: Tue, 2 Feb 2016 10:49:48 -0800 Subject: [PATCH 7/8] Add comment to attribute clojure.core.cache inspiration --- src/reactor/cache.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/reactor/cache.js b/src/reactor/cache.js index 81de0d7..55a3d81 100644 --- a/src/reactor/cache.js +++ b/src/reactor/cache.js @@ -15,6 +15,8 @@ export const CacheEntry = Record({ * evict(item) * asMap() * } + * + * Inspired by clojure.core.cache/CacheProtocol *******************************************************************************/ /** From 4fb6b8c37e7df228f4bf75f89473acc0fa481e95 Mon Sep 17 00:00:00 2001 From: loganlinn Date: Tue, 2 Feb 2016 13:49:13 -0800 Subject: [PATCH 8/8] Fix bug where cache didn't update dirty entries --- src/reactor/fns.js | 4 ++-- tests/reactor-tests.js | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/reactor/fns.js b/src/reactor/fns.js index c5e6945..3b38566 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -334,8 +334,8 @@ export function evaluate(reactorState, keyPathOrGetter) { const cache = reactorState.get('cache') var cacheEntry = cache.lookup(keyPathOrGetter) - const isCacheMiss = !cacheEntry - if (isCacheMiss || isDirtyCacheEntry(reactorState, cacheEntry)) { + const isCacheMiss = !cacheEntry || isDirtyCacheEntry(reactorState, cacheEntry) + if (isCacheMiss) { cacheEntry = createCacheEntry(reactorState, keyPathOrGetter) } diff --git a/tests/reactor-tests.js b/tests/reactor-tests.js index bf095db..a80eadf 100644 --- a/tests/reactor-tests.js +++ b/tests/reactor-tests.js @@ -595,6 +595,24 @@ describe('Reactor', () => { expect(taxPercentSpy.calls.count()).toEqual(2) expect(subtotalSpy.calls.count()).toEqual(1) }) + + it('should update cache with updated item after action', () => { + const lastItemGetter = [['items', 'all'], (items) => items.last()] + + // ensure its in cache + const lastItemBefore = reactor.evaluate(lastItemGetter) + const cacheEntryBefore = reactor.reactorState.cache.lookup(lastItemGetter) + expect(lastItemBefore === cacheEntryBefore.value).toBe(true) + + checkoutActions.addItem('potato', 0.80) + + const lastItemAfter = reactor.evaluate(lastItemGetter) + const cacheEntryAfter = reactor.reactorState.cache.lookup(lastItemGetter) + expect(lastItemAfter === cacheEntryAfter.value).toBe(true) + + // sanity check that lastItem actually changed for completeness + expect(lastItemAfter !== lastItemBefore).toBe(true) + }) }) describe('#observe', () => {