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 12 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
2 changes: 0 additions & 2 deletions docs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
16 changes: 8 additions & 8 deletions src/console-polyfill.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
try {
if (!(window.console && console.log)) {
if (!(window.console && console.log)) { // eslint-disable-line
console = {
log: function(){},
debug: function(){},
info: function(){},
warn: function(){},
error: function(){}
};
log: function() {},
debug: function() {},
info: function() {},
warn: function() {},
error: function() {},
}
}
} catch(e) {}
} catch(e) {} // eslint-disable-line
2 changes: 1 addition & 1 deletion src/create-react-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export default function(reactor) {
each(this.getDataBindings(), (getter, key) => {
const unwatchFn = reactor.observe(getter, (val) => {
this.setState({
[key]: val
[key]: val,
})
})

Expand Down
48 changes: 47 additions & 1 deletion src/getter.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,29 @@
import Immutable, { List } from 'immutable'
import { isFunction, isArray } from './utils'
import { isFunction, isArray, toFactory } from './utils'
import { isKeyPath } from './key-path'

const CACHE_OPTIONS = ['default', 'always', 'never']

/**
* Getter helper functions
* A getter is an array with the form:
* [<KeyPath>, ...<KeyPath>, <function>]
*/
const identity = (x) => x

class GetterClass {
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 this would be better as a pseudo-constructor. IE I dont like the idea of changing the Getter type because now we have to handle both a Getter instance and a plain array getter in every interface.

Instead Getter(['items']) returning simply ['items'] makes a lot of sense to me for backwards compat and simplicity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so basically what i had before?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but called Getter instead of addOptionsToGetter. The latter name implied that you could mutate an existing Getter (potentially globally) versus a function to create a Getter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok -- addOptionsToGetter was by ref. Which would modify it globally. We could return a cloned copy with options added instead. Thoughts?

constructor(getter, config = {}) {
if (!isKeyPath(getter) && !isGetter(getter)) {
throw new Error('Getter must be passed a keyPath or Getter')
}
this.getter = getter
this.cache = CACHE_OPTIONS.indexOf(config.cache) > -1 ? config.cache : 'default'
this.cacheKey = config.cacheKey !== undefined ? config.cacheKey : null
}
}

const Getter = toFactory(GetterClass)

/**
* Checks if something is a getter literal, ex: ['dep1', 'dep2', function(dep1, dep2) {...}]
* @param {*} toTest
Expand All @@ -18,12 +33,22 @@ function isGetter(toTest) {
return (isArray(toTest) && isFunction(toTest[toTest.length - 1]))
}

/**
* Checks if something is a getter object, ie created with the Getter function
* @param {*} toTest
* @return {boolean}
*/
function isGetterObject(toTest) {
return toTest instanceof GetterClass
}

/**
* Returns the compute function from a getter
* @param {Getter} getter
* @return {function}
*/
function getComputeFn(getter) {
getter = convertToGetterLiteral(getter)
return getter[getter.length - 1]
}

Expand All @@ -33,6 +58,7 @@ function getComputeFn(getter) {
* @return {function}
*/
function getDeps(getter) {
getter = convertToGetterLiteral(getter)
return getter.slice(0, getter.length - 1)
}

Expand All @@ -43,6 +69,7 @@ function getDeps(getter) {
* @return {Immutable.Set}
*/
function getFlattenedDeps(getter, existing) {
getter = convertToGetterLiteral(getter)
if (!existing) {
existing = Immutable.Set()
}
Expand Down Expand Up @@ -83,6 +110,7 @@ function fromKeyPath(keyPath) {
* @param {Getter}
*/
function getStoreDeps(getter) {
getter = convertToGetterLiteral(getter)
if (getter.hasOwnProperty('__storeDeps')) {
return getter.__storeDeps
}
Expand All @@ -102,11 +130,29 @@ function getStoreDeps(getter) {
return storeDeps
}

/**
* If the function is an instance of GetterClass, return the getter property
* @param {*} getter
* returns {*}
*/
function convertToGetterLiteral(getter) {
if (isGetterObject(getter)) {
return getter.getter
}
if (isKeyPath(getter) || isGetter(getter)) {
return getter
}
throw new Error('convertToGetterLiteral must be passed a keyPath or Getter')
}

export default {
isGetter,
getComputeFn,
getFlattenedDeps,
getStoreDeps,
getDeps,
fromKeyPath,
isGetterObject,
Getter,
convertToGetterLiteral,
}
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, Getter } from './getter'
import createReactMixin from './create-react-mixin'

export default {
Expand All @@ -17,4 +17,5 @@ export default {
toImmutable,
isImmutable,
createReactMixin,
Getter,
}
27 changes: 23 additions & 4 deletions src/reactor.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import Immutable from 'immutable'
import createReactMixin from './create-react-mixin'
import * as fns from './reactor/fns'
import { isKeyPath } from './key-path'
import { isGetter } from './getter'
import { isGetter, isGetterObject } from './getter'
import { toJS } from './immutable-helpers'
import { toFactory } from './utils'
import {
Expand All @@ -12,6 +12,8 @@ import {
PROD_OPTIONS,
} from './reactor/records'

export const DEFAULT_MAX_ITEMS_TO_CACHE = 1000

/**
* State is stored in NuclearJS Reactors. Reactors
* contain a 'state' object which is an Immutable.Map
Expand All @@ -24,11 +26,24 @@ import {
class Reactor {
constructor(config = {}) {
const debug = !!config.debug
const useCache = config.useCache === undefined ? true : !!config.useCache

// use default # of items in cache
let maxItemsToCache = DEFAULT_MAX_ITEMS_TO_CACHE

// if user has defined maxItemsToCache, use the number provide or set to null (ie no max)
if (config.maxItemsToCache !== undefined) {
maxItemsToCache = Number(config.maxItemsToCache)
maxItemsToCache = maxItemsToCache && maxItemsToCache > 0 ? maxItemsToCache : null
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: is there a difference between maxItemsToCache && maxItemsToCache > 0 and just maxItemsToCache > 0 here? looks like it could be simplified (given that it's a Number or NaN)

}

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 @@ -88,19 +103,23 @@ class Reactor {
let { observerState, entry } = fns.addObserver(this.observerState, getter, handler)
this.observerState = observerState
return () => {
this.observerState = fns.removeObserverByEntry(this.observerState, entry)
let [ observerState, reactorState ] = fns.removeObserverByEntry(this.observerState, this.reactorState, entry)
this.observerState = observerState
this.reactorState = reactorState
}
}

unobserve(getter, handler) {
if (arguments.length === 0) {
throw new Error('Must call unobserve with a Getter')
}
if (!isGetter(getter) && !isKeyPath(getter)) {
if (!isGetterObject(getter) && !isGetter(getter) && !isKeyPath(getter)) {
throw new Error('Must call unobserve with a Getter')
}

this.observerState = fns.removeObserver(this.observerState, getter, handler)
let [ observerState, reactorState ] = fns.removeObserver(this.observerState, this.reactorState, getter, handler)
this.observerState = observerState
this.reactorState = reactorState
}

/**
Expand Down
Loading