Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
38 changes: 30 additions & 8 deletions client/lib/tree-select/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,20 @@
*/
import { isObject, some, isFunction } from 'lodash';

const defaultGetCacheKey = ( ...args ) => args.join();

/**
* Returns a selector that caches values.
*
* @param {Function} getDependents A Function describing the dependent(s) of the selector.
* Must return an array which gets passed as the first arg to the selector
* @param {Function} selector A standard selector for calculating cached result
* @param {Object} options Options bag with additional arguments
* @param {Function} options.getCacheKey
* Custom way to compute the cache key from the `args` list
* @return {Function} Cached selector
*/
export default function treeSelect( getDependents, selector ) {
export default function treeSelect( getDependents, selector, options = {} ) {
if ( process.env.NODE_ENV !== 'production' ) {
if ( ! isFunction( getDependents ) || ! isFunction( selector ) ) {
throw new TypeError(
Expand All @@ -22,13 +27,15 @@ export default function treeSelect( getDependents, selector ) {
}
}

const cache = new WeakMap();
let cache = new WeakMap();

const { getCacheKey = defaultGetCacheKey } = options;

return function( state, ...args ) {
const cachedSelector = function( state, ...args ) {
const dependents = getDependents( state, ...args );

if ( process.env.NODE_ENV !== 'production' ) {
if ( some( args, isObject ) ) {
if ( getCacheKey === defaultGetCacheKey && some( args, isObject ) ) {
throw new Error( 'Do not pass objects as arguments to a treeSelector' );
}
}
Expand All @@ -38,7 +45,7 @@ export default function treeSelect( getDependents, selector ) {
// garbage collect any values that are based on outdated dependents
const leafCache = dependents.reduce( insertDependentKey, cache );

const key = args.join();
const key = getCacheKey( ...args );
if ( leafCache.has( key ) ) {
return leafCache.get( key );
}
Expand All @@ -47,8 +54,20 @@ export default function treeSelect( getDependents, selector ) {
leafCache.set( key, value );
return value;
};

cachedSelector.clearCache = () => {
// WeakMap doesn't have `clear` method, so we need to recreate it
cache = new WeakMap();
};

return cachedSelector;
}

/*
* This object will be used as a WeakMap key if a dependency is a falsy value (null, undefined, ...)
*/
const STATIC_FALSY_KEY = {};
Copy link
Member

Choose a reason for hiding this comment

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

I actually liked the MixedMap @samouri originally had in here. It had clear semantics and didn't require these odd workarounds. Is there real opposition against using MixedMap?

Copy link
Member Author

Choose a reason for hiding this comment

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

The semantics now is that the dependents are of type Maybe<Object> -- that looks quite clean to me. If I encounter a place where I need the MixedMap, I'll start rooting for it.

Copy link
Contributor

@samouri samouri Jan 17, 2018

Choose a reason for hiding this comment

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

If we only have a need to support null/undefined/false, then i don't think we should incorporate MixedMap which allowed for unbounded primitives like number and string


/*
* First tries to get the value for the key.
* If the key is not present, then inserts a new map and returns it
Expand All @@ -57,11 +76,14 @@ export default function treeSelect( getDependents, selector ) {
* The last map is a regular one because the the key for the last map is the string results of args.join().
*/
function insertDependentKey( map, key, currentIndex, arr ) {
if ( map.has( key ) ) {
return map.get( key );
const weakMapKey = key || STATIC_FALSY_KEY;

const existingMap = map.get( weakMapKey );
if ( existingMap ) {
return existingMap;
}

const newMap = currentIndex === arr.length - 1 ? new Map() : new WeakMap();
map.set( key, newMap );
map.set( weakMapKey, newMap );
return newMap;
}
54 changes: 54 additions & 0 deletions client/lib/tree-select/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,5 +197,59 @@ describe( 'index', () => {

expect( getDependents ).toHaveBeenCalledWith( [ state.posts ], 1, 2, 3 );
} );

test( 'should bust the cache when clearCache() method is called', () => {
const reduxState = {
posts: {
[ post1.id ]: post1,
[ post2.id ]: post2,
[ post3.id ]: post3,
},
};

const firstResult = getSitePosts( reduxState, 'site1' );
const memoizedResult = getSitePosts( reduxState, 'site1' );

// Repeated call returns identical object
expect( memoizedResult ).toBe( firstResult );

getSitePosts.clearCache();
const afterClearResult = getSitePosts( reduxState, 2916284 );

// Is forced to compute a new result after clearCache()
expect( afterClearResult ).not.toBe( firstResult );
} );

test( 'should memoize a falsy value returned by getDependents', () => {
const memoizedSelector = treeSelect( () => [ null ], () => [] );
const state = {};

const firstResult = memoizedSelector( state );
const secondResult = memoizedSelector( state );
expect( firstResult ).toBe( secondResult );
} );

test( 'accepts a getCacheKey option that enables object arguments', () => {
const reduxState = {
posts: {
[ post1.id ]: post1,
[ post2.id ]: post2,
[ post3.id ]: post3,
},
};

const memoizedSelector = treeSelect(
state => [ state.posts ],
( [ posts ], query ) => filter( posts, { siteId: query.siteId } ),
{ getCacheKey: query => `key:${ query.siteId }` }
);

// The arguments are objects, they are not identical, but generated keys are.
// Therefore, the second call returns the memoized result.
const firstResult = memoizedSelector( reduxState, { siteId: 'site1', foo: 'bar' } );
const secondResult = memoizedSelector( reduxState, { siteId: 'site1', foo: 'baz' } );
expect( firstResult ).toEqual( [ post1, post2 ] );
expect( firstResult ).toBe( secondResult );
} );
} );
} );