-
Notifications
You must be signed in to change notification settings - Fork 2k
framework: add treeSelect (a new type of memoized selector) #20547
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
2fd6e77
Framework: introduce tree selector
samouri 78a00c4
address feedback
samouri 2039a43
add checks for production
samouri e4f6eb1
goodbye mixedmap
samouri 12ab35e
update docs
samouri 46720f5
address jarda's feedback
samouri 6fedd60
adjust comment
samouri 46ea23a
improve test name
samouri b058d3f
address @dmsnells points
samouri File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # `treeSelect` | ||
|
|
||
| This module exports a function which creates a cached state selector for use with the Redux global application state. It is a good idea to use this function over plain selectors whenever either the computation over state or React's rerenders are expensive. | ||
| It is called `treeSelect` because it internally uses a tree of dependencies to allow the gc to free memory without explicitly clearing the cache. | ||
|
|
||
| ## Usage | ||
|
|
||
| `treeSelect` accepts the following arguments: | ||
|
|
||
| * **getDependents**: A function which maps over `state` and returns all of the relevant parts of state the selector needs. You should be creating an object whose values are all the return of other selectors -- no computations allowed here. | ||
| * **selector**: A function which takes in the same args as `getDependents` with one catch. Instead of being passed state as its first arg, it is given the results of getDependents. This forces you to declare all of your state-dependencies. | ||
|
|
||
| For example, imagine that our state contains post objects, each of which are assigned to a particular site. Retrieving an array of posts for a specific site would require us to filter over all of the known posts. While this would normally trigger a re-render in React, we can use `treeSelect` to create a cached version of the selector which can return a referentially equal object: | ||
|
|
||
| ```js | ||
| const getDependents = state => ( { posts: state.posts } ); | ||
| const selector = ( [ posts ], siteId ) => filter( posts, { siteId } ); | ||
|
|
||
| const getSitePosts = treeSelect( selector, getDependents ); | ||
| ``` | ||
|
|
||
| In using the selector, we pass in the arguments for `selector`. In this case, we'll need to pass a state object and siteId. | ||
|
|
||
| ```js | ||
| const sitePosts = getSitePosts( state, siteId ); | ||
| ``` | ||
|
|
||
| This result would only be calculated once, so long as `state.posts` remains the same. | ||
|
|
||
| ## FAQ | ||
|
|
||
| ### What is a cached selector? | ||
|
|
||
| We strive to keep redundant data out of our Redux state tree, but this can come at the cost of performance if our selectors are time-consuming in how they evaluate and filter over the state. | ||
|
|
||
| A cached selector is to redux state what a [materialized view](https://en.wikipedia.org/wiki/Materialized_view) is to a database table. It is a selector that caches results as long as its dependent parts of state have not changed. | ||
|
|
||
| ### How does the cached selector know when to recalculate its result? | ||
|
|
||
| Because Redux discourages us from mutating objects within state directly, we only need to verify that a piece of state is no longer referentially equal to its previous state (as opposed to a deep equality check). | ||
|
|
||
| When creating a treeSelect selector, you give it a function `getDependents` that returns an array of dependents. As an example, lets say you have a function that depends on both a particular site and its comments like so: | ||
|
|
||
| ```js | ||
| const getDependents = ( state, siteId ) => [ state.comments[ siteId ], state.sites[ siteId ] ]; | ||
| const selector = ( [ comments, site ], siteId ) => | ||
| `Site ${ site.title } has ${ comments.length } comments`; | ||
|
|
||
| const cachedSelector = treeSelect( selector, getDependents ); | ||
| ``` | ||
|
|
||
| internally, the selector will store a dependency tree of dependents where the last node is a map keyed by a run of `...args.join()`. The tree would look like: | ||
|
|
||
| ``` | ||
| comments | ||
| + | ||
| | | ||
| +--------------------+ | ||
| | | | ||
| v v | ||
| site1 site2 | ||
| + + | ||
| +------ ... | ||
| v | ||
| "siteId1" | ||
| + | ||
| | | ||
| v | ||
| "Site...." | ||
| ``` | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /** @format */ | ||
|
|
||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { isObject, some, isFunction } from 'lodash'; | ||
|
|
||
| /** | ||
| * 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 | ||
| * @return {Function} Cached selector | ||
| */ | ||
| export default function treeSelect( getDependents, selector ) { | ||
| if ( process.env.NODE_ENV !== 'production' ) { | ||
| if ( ! isFunction( getDependents ) || ! isFunction( selector ) ) { | ||
| throw new TypeError( | ||
| 'treeSelect: invalid arguments passed, selector and getDependents must both be functions' | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const cache = new WeakMap(); | ||
|
|
||
| return function( state, ...args ) { | ||
| const dependents = getDependents( state, ...args ); | ||
|
|
||
| if ( process.env.NODE_ENV !== 'production' ) { | ||
| if ( some( args, isObject ) ) { | ||
| throw new Error( 'Do not pass objects as arguments to a treeSelector' ); | ||
| } | ||
| } | ||
|
|
||
| // create a dependency tree for caching selector results. | ||
| // this is beneficial over standard memoization techniques so that we can | ||
| // garbage collect any values that are based on outdated dependents | ||
| const leafCache = dependents.reduce( insertDependentKey, cache ); | ||
|
|
||
| const key = args.join(); | ||
| if ( leafCache.has( key ) ) { | ||
| return leafCache.get( key ); | ||
| } | ||
|
|
||
| const value = selector( dependents, ...args ); | ||
| leafCache.set( key, value ); | ||
| return value; | ||
| }; | ||
| } | ||
|
|
||
| /* | ||
| * First tries to get the value for the key. | ||
| * If the key is not present, then inserts a new map and returns it | ||
| * | ||
| * Note: Inserts WeakMaps except for the last map which will be a regular Map. | ||
| * 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 ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could shave off another lookup by only calling |
||
| } | ||
|
|
||
| const newMap = currentIndex === arr.length - 1 ? new Map() : new WeakMap(); | ||
| map.set( key, newMap ); | ||
| return newMap; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,201 @@ | ||
| /** @format */ | ||
| import { filter } from 'lodash'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import treeSelect from '../'; | ||
|
|
||
| describe( 'index', () => { | ||
| describe( '#treeSelect', () => { | ||
| const post1 = { id: 'id1', text: 'here is post 1', siteId: 'site1' }; | ||
| const post2 = { id: 'id2', text: 'here is post 2', siteId: 'site1' }; | ||
| const post3 = { id: 'id3', text: 'here is post 3', siteId: 'site2' }; | ||
|
|
||
| let getSitePosts; | ||
| let selector; | ||
| let getDependents; | ||
|
|
||
| beforeEach( () => { | ||
| selector = jest.fn( ( [ posts ], siteId ) => filter( posts, { siteId } ) ); | ||
| getDependents = jest.fn( state => [ state.posts ] ); | ||
| getSitePosts = treeSelect( getDependents, selector ); | ||
| } ); | ||
|
|
||
| test( 'should create a function which returns the expected value when called', () => { | ||
| const state = { | ||
| posts: { | ||
| [ post1.id ]: post1, | ||
| [ post2.id ]: post2, | ||
| [ post3.id ]: post3, | ||
| }, | ||
| }; | ||
|
|
||
| expect( getSitePosts( state, 'site1' ) ).toEqual( [ post1, post2 ] ); | ||
| } ); | ||
|
|
||
| test( 'should cache the result of a selector function', () => { | ||
| const reduxState = { | ||
| posts: { | ||
| [ post1.id ]: post1, | ||
| [ post2.id ]: post2, | ||
| [ post3.id ]: post3, | ||
| }, | ||
| }; | ||
|
|
||
| getSitePosts( reduxState, 2916284 ); | ||
| getSitePosts( reduxState, 2916284 ); | ||
|
|
||
| expect( selector.mock.calls.length ).toBe( 1 ); | ||
| } ); | ||
|
|
||
| test( 'should cache the result of a selector function that has multiple dependents', () => { | ||
| const site1 = { id: 'siteId1' }; | ||
| const site2 = { id: 'siteId2' }; | ||
| const reduxState = { | ||
| posts: { | ||
| [ post1.id ]: post1, | ||
| [ post2.id ]: post2, | ||
| [ post3.id ]: post3, | ||
| }, | ||
| sites: { | ||
| [ site1.id ]: site1, | ||
| [ site2.id ]: site2, | ||
| }, | ||
| }; | ||
|
|
||
| const takeOne = jest.fn( ( [ posts, sites ] ) => [ | ||
| Object.values( posts )[ 0 ], | ||
| Object.values( sites )[ 0 ], | ||
| ] ); | ||
| const getDeps = jest.fn( state => [ state.posts, state.sites ] ); | ||
| const arborealTakeOne = treeSelect( getDeps, takeOne ); | ||
|
|
||
| arborealTakeOne( reduxState, 42 ); | ||
| const results = arborealTakeOne( reduxState, 42 ); | ||
| expect( results[ 0 ] ).toBe( Object.values( reduxState.posts )[ 0 ] ); | ||
| expect( results[ 1 ] ).toBe( Object.values( reduxState.sites )[ 0 ] ); | ||
|
|
||
| expect( takeOne.mock.calls.length ).toBe( 1 ); | ||
| } ); | ||
|
|
||
| test( 'should throw an error if getDependents is missing', () => { | ||
| expect( () => treeSelect( undefined, selector ) ).toThrow(); | ||
| } ); | ||
|
|
||
| test( 'should throw an error if selector is missing', () => { | ||
| expect( () => treeSelect( getDependents ) ).toThrow(); | ||
| } ); | ||
|
|
||
| test( 'should not throw an error in production for missing args', () => { | ||
| const prevEnv = process.env.NODE_ENV; | ||
| process.env.NODE_ENV = 'production'; | ||
|
|
||
| expect( () => treeSelect() ).not.toThrow(); | ||
|
|
||
| process.env.NODE_ENV = prevEnv; | ||
| } ); | ||
|
|
||
| test( 'should throw an error in development when given object arguments', () => { | ||
| const state = {}; | ||
|
|
||
| expect( () => getSitePosts( state, {} ) ).toThrow(); | ||
| expect( () => getSitePosts( state, [] ) ).toThrow(); | ||
| expect( () => getSitePosts( state, 1, [] ) ).toThrow(); | ||
| } ); | ||
|
|
||
| test( 'should not throw an error in production even when given object arguments', () => { | ||
| const prevEnv = process.env.NODE_ENV; | ||
| process.env.NODE_ENV = 'production'; | ||
|
|
||
| const state = { posts: [] }; | ||
| expect( () => getSitePosts( state, {} ) ).not.toThrow(); | ||
| expect( () => getSitePosts( state, [] ) ).not.toThrow(); | ||
| expect( () => getSitePosts( state, 1, [] ) ).not.toThrow(); | ||
|
|
||
| process.env.NODE_ENV = prevEnv; | ||
| } ); | ||
|
|
||
| test( 'should not throw an error in development when given primitives', () => { | ||
| const state = { posts: [] }; | ||
|
|
||
| expect( () => getSitePosts( state, 1 ) ).not.toThrow(); | ||
| expect( () => getSitePosts( state, '' ) ).not.toThrow(); | ||
| expect( () => getSitePosts( state, 'foo' ) ).not.toThrow(); | ||
| expect( () => getSitePosts( state, true ) ).not.toThrow(); | ||
| expect( () => getSitePosts( state, null ) ).not.toThrow(); | ||
| expect( () => getSitePosts( state, undefined ) ).not.toThrow(); | ||
| } ); | ||
|
|
||
| test( 'should call selector when making non-cached calls', () => { | ||
| const state = { | ||
| posts: { | ||
| [ post1.id ]: post1, | ||
| [ post3.id ]: post3, | ||
| }, | ||
| }; | ||
|
|
||
| const sitePosts1 = getSitePosts( state, post1.siteId ); | ||
| const sitePosts3 = getSitePosts( state, post3.siteId ); | ||
|
|
||
| expect( sitePosts1 ).toEqual( [ post1 ] ); | ||
| expect( sitePosts3 ).toEqual( [ post3 ] ); | ||
| expect( selector.mock.calls.length ).toBe( 2 ); | ||
| } ); | ||
|
|
||
| test( 'should bust the cache when watched state changes', () => { | ||
| const prevState = { | ||
| posts: { | ||
| [ post1.id ]: post1, | ||
| }, | ||
| }; | ||
|
|
||
| getSitePosts( prevState, post1.siteId ); | ||
|
|
||
| const nextState = { | ||
| posts: { | ||
| [ post1.id ]: { ...post1, modified: true }, | ||
| }, | ||
| }; | ||
|
|
||
| expect( getSitePosts( nextState, post1.siteId ) ).toEqual( [ { ...post1, modified: true } ] ); | ||
| expect( selector.mock.calls.length ).toBe( 2 ); | ||
| } ); | ||
|
|
||
| test( 'should maintain the cache for unique dependents simultaneously', () => { | ||
| const getPostByIdWithDataSpy = jest.fn( ( [ post ] ) => { | ||
| return { | ||
| ...post, | ||
| withData: true, | ||
| }; | ||
| } ); | ||
|
|
||
| const getPostByIdWithData = treeSelect( | ||
| ( state, postId ) => [ state.posts[ postId ] ], | ||
| getPostByIdWithDataSpy | ||
| ); | ||
|
|
||
| const state = { | ||
| posts: { | ||
| [ post1.id ]: post1, | ||
| [ post2.id ]: post2, | ||
| }, | ||
| }; | ||
|
|
||
| getPostByIdWithData( state, post1.id ); // dependents is [ post1 ] | ||
| getPostByIdWithData( state, post2.id ); // dependents is [ post2 ] | ||
| getPostByIdWithData( state, post1.id ); // dependents is [ post1 ]. should use cache | ||
|
|
||
| expect( getPostByIdWithDataSpy.mock.calls.length ).toBe( 2 ); | ||
| } ); | ||
|
|
||
| test( 'should call dependant state getter with dependents and arguments', () => { | ||
| const memoizedSelector = treeSelect( getDependents, getDependents ); | ||
| const state = { posts: {} }; | ||
|
|
||
| memoizedSelector( state, 1, 2, 3 ); | ||
|
|
||
| expect( getDependents ).toHaveBeenCalledWith( [ state.posts ], 1, 2, 3 ); | ||
| } ); | ||
| } ); | ||
| } ); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while I think
dependentis more proper I find myself constantly making typos increateSelectorcode because it usesdependantwith anaThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️