NotificationsYou must be signed in to change notification settings
Fork666
Star19.1k

A new approach to sharing selectors: createIndexedSelector#213

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

1000hz wants to merge2 commits intoreduxjs:masterfrom1000hz:indexed-selectors

Closed

A new approach to sharing selectors: createIndexedSelector#213

1000hz wants to merge2 commits intoreduxjs:masterfrom1000hz:indexed-selectors

Conversation

Copy link

1000hz commentedJan 19, 2017•
edited
Loading

I think there’s a better approach to sharing selectors with props between components than the one outlined in the docs. That approach suggests each component should create its own private instance of the selector, but this leads to unnecessary recomputations. Two different selector instances given the same arguments will individually recompute and store the same result.

Instead of creating a private instance of the selector for each component, we could instead create a higher-order selector that keeps a cache of selectors and instantiates a new instance only when necessary. We could call this concept an Indexed Selector.

constindexResolver=(state,index)=>indexexportfunctioncreateIndexedSelector(selectorFactory,resolver=indexResolver){constcache={}return(...args)=>{constkey=resolver(...args)constselector=cache[key]||(cache[key]=selectorFactory())returnselector(...args)}}

Note that for convenience and generalizability, I’ve opted to pass the index to selectors instead of a props object by default, but you can use your own resolver if you'd rather pass in your whole props object to your selectors. Rewriting the docs example to use this pattern, we get:

import{createSelector,createIndexedSelector}from'reselect'constgetVisibilityFilter=(state,index)=>state.todoLists[index].visibilityFilterconstgetTodos=(state,index)=>state.todoLists[index].todosconstgetVisibleTodos=createIndexedSelector(()=>{returncreateSelector([getVisibilityFilter,getTodos],(visibilityFilter,todos)=>{switch(visibilityFilter){case'SHOW_COMPLETED':returntodos.filter(todo=>todo.completed)case'SHOW_ACTIVE':returntodos.filter(todo=>!todo.completed)default:returntodos}})})exportdefaultgetVisibleTodos

constmapStateToProps=(state,props)=>{return{todos:getVisibleTodos(state,props.listId)}}

This has the added bonus of simplifying our connector since we no longer need to usemakeMapStateToProps!

Let me know of anything I might be overlooking or further changes you’d like to discuss.

adding createIndexedSelector.

68ff26b

1000hz changed the title~~A new approach to sharing selectors~~A new approach to sharing selectors: createIndexedSelector

Jan 19, 2017

fixing lint errors.

73ffb42

Copy link

coveralls commentedJan 19, 2017•
edited
Loading

Coverage remained the same at 100.0% when pulling73ffb42 on 1000hz:indexed-selectors into1143dcb on reactjs:master.

3 similar comments

Copy link

coveralls commentedJan 19, 2017

Coverage remained the same at 100.0% when pulling73ffb42 on 1000hz:indexed-selectors into1143dcb on reactjs:master.

Copy link

coveralls commentedJan 19, 2017

Coverage remained the same at 100.0% when pulling73ffb42 on 1000hz:indexed-selectors into1143dcb on reactjs:master.

Copy link

coveralls commentedJan 19, 2017

Coverage remained the same at 100.0% when pulling73ffb42 on 1000hz:indexed-selectors into1143dcb on reactjs:master.

Copy link

codecov-io commentedJan 19, 2017•
edited
Loading

Current coverage is 100% (diff: 100%)

Merging#213 intomaster will not change coverage

@@           master   #213   diff @@====================================  Files           1      1            Lines          12     13     +1     Methods         0      0            Messages        0      0            Branches        0      0          ====================================+ Hits           12     13     +1  Misses          0      0            Partials        0      0

Powered byCodecov. Last update1143dcb...73ffb42

Copy link

walerun commentedFeb 15, 2017

It looks interesting. But what if components have unique key/id each time when we create them. If we will delete some component, it means cache will have an unused selector, wich will never invoke. It may cause cache growth. Though maybe it's not a big problem.
I have one more question. Maybe it is not related to current PR, but how can we combine components selectors to create a new selector?

Copy link

toomuchdesign commentedFeb 27, 2017

Hi all! I've just published a tiny plugin to memoize a collection of selectors based on a resolver function for a project of mines.

It works more or less as Lodash's .memoize() does, but with almost zero configuration. The name isre-reselect and it's just a few line of code without dependencies.

If externalizing to another micro-package is ok, It should fit this multi-component scenario, too.

Ping me for any info.
Thank's for the great work you're doing on Reselect. Cheers!

Copy link

drcmda commentedMar 6, 2017•
edited
Loading

@ellbee @1000hz @toomuchdesign

Could something like create[Indexed/Cached/Mapped]Selector be officially taken in?

Our team is kind of scared committing to reselect because in essence it would force us to use the factory pattern pretty much all the time.

We feel like the least use-case, one that rarely applies to a real world scenario, has been made default while in the majority of cases mapStateToProps gets padded with boilerplate which offloads the cognitive load to the component designer that now has to think through intricacies that shouldn't be of concern.

Copy link

toomuchdesign commentedMar 9, 2017

@drcmda

Your point is the reason why I published re-reselect. I just moved a memoized reselect factory into a separate package declaring reselect as peerDependency.

Copy link

Collaborator

ellbee commentedMar 9, 2017

I think what I'll probably do here is advertise re-reselect in the documentation, and if it turns out to be very popular then we will pull it into reselect itself.