Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Caching stuff #208

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/getter.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,41 @@ function getStoreDeps(getter) {
return storeDeps
}

/**
* Add override options to a getter
* @param {getter} getter
* @param {object} options
* @param {boolean} options.useCache
* @param {*} options.cacheKey
* @returns {getter}
*/
function addGetterOptions(getter, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This API change to introduce the concept of "getter options" is a little awkward imo.

Currently, it will replace any previously set options which could lead to unexpected behavior given its name.
An alternative would be to call this setGetterOptions to indicate it would replace, not patch, the options.
But I feel like the API is being conflated with implementation details of hidden __options object. The alternative that I would suggest is two separate methods to configure these options.

cc @jordangarcia since this is API design considerations

Copy link
Contributor

Choose a reason for hiding this comment

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

@jordangarcia Another thing to consider is that it's probably natural from an API standpoint that getters are configured where they are defined. It may be time to introduce a formal Getter() constructor... :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see a Getter constructor.

// same as `['items']`
var getter = Getter(['items']);
// or
var getter = Getter(['items'], {
  useCache: false
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jordangarcia Is there an advantage to using a constructor outside of style preference?

if (!isKeyPath(getter) && !isGetter(getter)) {
throw new Error('createGetter must be passed a keyPath or Getter')
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the error string was copied from createGetter() and should be updated

}

if (getter.hasOwnProperty('__options')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing these options on getter could lead to surprising behavior if getter a key path that should be carefully considered and definitely documented.

A newcomer to nuclear-js might think this would change the cache key (globally):

addGetterOptions(['items'], {cacheKey: "items_cache"});
reactor.evaluate(['items']);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a Getter constructor alleviates some of this confusion, as it's clear that you are creating an instance of something and the options you provide with it would be tied to that instance.

throw new Error('Cannot reassign options to getter')
}

getter.__options = {}

if (options.useCache !== undefined) {
getter.__options.useCache = !!options.useCache
}

if (options.cacheKey !== undefined) {
getter.__options.cacheKey = options.cacheKey
}
return getter
}

export default {
isGetter,
getComputeFn,
getFlattenedDeps,
getStoreDeps,
getDeps,
fromKeyPath,
addGetterOptions,
}
3 changes: 2 additions & 1 deletion src/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import Reactor from './reactor'
import Immutable from 'immutable'
import { toJS, toImmutable, isImmutable } from './immutable-helpers'
import { isKeyPath } from './key-path'
import { isGetter } from './getter'
import { isGetter, addGetterOptions } from './getter'
import createReactMixin from './create-react-mixin'

export default {
Expand All @@ -17,4 +17,5 @@ export default {
toImmutable,
isImmutable,
createReactMixin,
addGetterOptions,
}
20 changes: 20 additions & 0 deletions src/reactor.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,16 @@ import {
class Reactor {
constructor(config = {}) {
const debug = !!config.debug
const useCache = config.useCache === undefined ? true : !!config.useCache
const itemsToCache = Number(config.maxItemsToCache)
const maxItemsToCache = itemsToCache && itemsToCache > 1 ? itemsToCache : null
const baseOptions = debug ? DEBUG_OPTIONS : PROD_OPTIONS
const initialReactorState = new ReactorState({
debug: debug,
maxItemsToCache: maxItemsToCache,
// merge config options with the defaults
options: baseOptions.merge(config.options || {}),
useCache: useCache
})

this.prevReactorState = initialReactorState
Expand Down Expand Up @@ -285,6 +290,21 @@ class Reactor {
this.__isDispatching = false
}
}

/**
* Retrieve cache values
* @returns {Immutable.Map}
*/
getCacheValues() {
return this.reactorState.get('cache')
}

/**
* Clear all cached values
*/
clearCacheValues() {
this.reactorState = this.reactorState.set('cache', Immutable.Map())
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the use case of this api method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added as an afterthought, thought it might be useful in some cases of switching views in a spa. will remove

}
}

export default toFactory(Reactor)
55 changes: 48 additions & 7 deletions src/reactor/fns.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { each } from '../utils'
* Immutable Types
*/
const EvaluateResult = Immutable.Record({ result: null, reactorState: null})
export const CACHE_CLEAR_RATIO = .8;

function evaluateResult(result, reactorState) {
return new EvaluateResult({
Expand Down Expand Up @@ -335,14 +336,13 @@ export function evaluate(reactorState, keyPathOrGetter) {
// Cache hit
return evaluateResult(
getCachedValue(reactorState, keyPathOrGetter),
reactorState
updateCacheRecency(reactorState, keyPathOrGetter)
)
}

// evaluate dependencies
const args = getDeps(keyPathOrGetter).map(dep => evaluate(reactorState, dep).result)
const evaluatedValue = getComputeFn(keyPathOrGetter).apply(null, args)

return evaluateResult(
evaluatedValue,
cacheValue(reactorState, keyPathOrGetter, evaluatedValue)
Expand Down Expand Up @@ -381,6 +381,9 @@ export function resetDirtyStores(reactorState) {
* @return {Getter}
*/
function getCacheKey(getter) {
if (getter.__options && getter.__options.cacheKey !== undefined) {
return getter.__options.cacheKey
}
return getter
}

Expand Down Expand Up @@ -424,6 +427,17 @@ function isCached(reactorState, keyPathOrGetter) {
* @return {ReactorState}
*/
function cacheValue(reactorState, getter, value) {
const hasGetterUseCacheSpecified = getter.__options && getter.__options.useCache !== undefined

// Prefer getter settings over reactor settings
if (hasGetterUseCacheSpecified) {
if (!getter.__options.useCache) {
return reactorState
}
} else if (!reactorState.get('useCache')) {
return reactorState
}

const cacheKey = getCacheKey(getter)
const dispatchId = reactorState.get('dispatchId')
const storeDeps = getStoreDeps(getter)
Expand All @@ -434,11 +448,38 @@ function cacheValue(reactorState, getter, value) {
})
})

return reactorState.setIn(['cache', cacheKey], Immutable.Map({
value: value,
storeStates: storeStates,
dispatchId: dispatchId,
}))
const maxItemsToCache = reactorState.get('maxItemsToCache')
const itemsToCache = maxItemsToCache * CACHE_CLEAR_RATIO

return reactorState.withMutations(state => {
if (maxItemsToCache && maxItemsToCache <= state.get('cache').size) {
do {
let key = state.get('cacheRecency').first()
state.deleteIn(['cache', key])
state.deleteIn(['cacheRecency', key])
} while (itemsToCache < state.get('cache').size)
}

state.setIn(['cacheRecency', cacheKey], cacheKey)
state.setIn(['cache', cacheKey], Immutable.Map({
value: value,
storeStates: storeStates,
dispatchId: dispatchId,
}))
})
}

/**
* Readds the key for the item in cache to update recency
* @param {ReactorState} reactorState
* @param {cacheKey} cacheKey
* @return {ReactorState}
*/
function updateCacheRecency(reactorState, cacheKey) {
return reactorState.withMutations(state => {
state.deleteIn(['cacheRecency', cacheKey])
Copy link
Contributor

Choose a reason for hiding this comment

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

This deleteIn() isn't needed. setIn() is called for same key path directly after.

return reactorState.setIn(['cacheRecency', cacheKey], cacheKey)

edit: nevermind, I missed that it's an OrderedMap

state.setIn(['cacheRecency', cacheKey], cacheKey)
})
}

/**
Expand Down
5 changes: 4 additions & 1 deletion src/reactor/records.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Map, Set, Record } from 'immutable'
import { Map, Set, Record, OrderedMap } from 'immutable'

export const PROD_OPTIONS = Map({
// logs information for each dispatch
Expand Down Expand Up @@ -39,12 +39,15 @@ export const ReactorState = Record({
state: Map(),
stores: Map(),
cache: Map(),
cacheRecency: OrderedMap(),
// maintains a mapping of storeId => state id (monotomically increasing integer whenever store state changes)
storeStates: Map(),
dirtyStores: Set(),
debug: false,
maxItemsToCache: null,
// production defaults
options: PROD_OPTIONS,
useCache: true,
})

export const ObserverState = Record({
Expand Down
35 changes: 34 additions & 1 deletion tests/getter-tests.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isGetter, getFlattenedDeps, fromKeyPath } from '../src/getter'
import { isGetter, getFlattenedDeps, fromKeyPath, addGetterOptions } from '../src/getter'
import { Set, List, is } from 'immutable'

describe('Getter', () => {
Expand Down Expand Up @@ -32,6 +32,39 @@ describe('Getter', () => {
})
})

describe('#addGetterOptions', () => {
it('should throw an error if not passed a getter', () => {
expect(() => { addGetterOptions('test', {}) }).toThrow()
})

it('should throw an error if options are added more than once', () => {
let getter = ['test']
getter = addGetterOptions(getter, {})
expect(() => { addGetterOptions(getter, {}) }).toThrow()
})

it('should not add options if they are not explicitly supplied or are not valid options', () => {
let getter = ['test']
getter = addGetterOptions(getter, {})
expect(getter.__options).toEqual({})
getter = ['test']
getter = addGetterOptions(getter, { fakeOption: true })
expect(getter.__options).toEqual({})
})

it('should add the use cache option', () => {
let getter = ['test']
getter = addGetterOptions(getter, { useCache: false })
expect(getter.__options.useCache).toBe(false)
})

it('should add the cacheKey option', () => {
let getter = ['test']
getter = addGetterOptions(getter, { cacheKey: 100 })
expect(getter.__options.cacheKey).toBe(100)
})
})

describe('getFlattenedDeps', function() {
describe('when passed the identity getter', () => {
it('should return a set with only an empty list', () => {
Expand Down
Loading