Skip to content

Selector with props does not make sense when defaultMemoize has cache size = 1 #66

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

Closed
compulim opened this issue Dec 14, 2015 · 11 comments

Comments

@compulim
Copy link

Assumption:

  • A component that appears on a page multiple times
  • The component use ReactRedux.connect and reselect to select data based on state and component.props

Problem:
If the page have two or more component instances and their props are different, the memoize function (which has cache size = 1) will always cache miss.

This is because the memoize function is shared amongst all component instance, thus, component with different props will always invalidate each other.

Solution:
There are multiple solutions to this issue. I think memoize cache should bound to the component instance.

@oyeanuj
Copy link

oyeanuj commented Dec 14, 2015

I have the same use-case, and had the same questions! What is the best practice or recommended approach here?

@timdorr @ellbee The docs mention using memoize function from lodash for an unbounded cache. Is that the best way to go about this?

@ellbee
Copy link
Collaborator

ellbee commented Dec 14, 2015

The docs mention using memoize function from lodash for an unbounded cache. Is that the best way to go about this?

Maybe. A couple of things to consider:

  • How big is your cache going to get? Do you need to use something like an lru cache to stop it getting out of control if your state and props are changing rapidly?
  • How is your memoize function going to create the keys for your cache? If your parameters are objects then you are going to have to serialize or hash them somehow. If those objects are big it could get expensive.

There is some discussion about this in the react-redux repo, and some more feedback would be welcome.

@oyeanuj
Copy link

oyeanuj commented Dec 14, 2015

@ellbee Thanks for the quick response!

How big is your cache going to get? Do you need to use something like an lru cache to stop it getting out of control if your state and props are changing rapidly?

My case is having 10-30 components of a particular kind on a page. Feeds in a post, Tweets in a stream, pictures/videos/media in a gallery are all similar examples. Its not as much as the state and props changing rapidly but getting a better understanding of how best to be able to re-use selectors for the same kind of components. According to my understanding, thats where the cache issue comes in (assuming the state and props for a single component won't change that much).

What is the best practice when dealing with two or more of the same component on the page?

How is your memoize function going to create the keys for your cache? If your parameters are objects then you are going to have to serialize or hash them somehow. If those objects are big it could get expensive.

The parameters are objects (which I imagine is the worst case). The keys could be the id of the object passed through the props. Do you think being able to use the default memoization function (with configurable cache) in reselect would be the best way to go about this?

@compulim
Copy link
Author

How often do 2+ component instances benefit when their memoize cache are shared? I don't think alot.

IMHO, cache size = 1 is already good enough, as long as the cache (lastArgs in defaultMemoize) belongs to a single component instance, not shared across instances.

@oyeanuj
Copy link

oyeanuj commented Dec 15, 2015

@compulim That would work as well - cache is specific to component-instance and not across instances.

@compulim
Copy link
Author

Trying to look at the problem by binding component instance with memorize function but it's not trivial. @ellbee can you give us some hints?

@ellbee
Copy link
Collaborator

ellbee commented Dec 16, 2015

Cool, thanks for looking into it. Did you check out the issue I linked? It is trying to solve the same thing by modifying react-redux slightly, but there is also the outline of an idea for solving it with Reselect.

@compulim
Copy link
Author

Thanks @ellbee, I think it's the same issue. Let's centralize the discussion to reduxjs/react-redux#183.

@pdf
Copy link

pdf commented Feb 3, 2016

@ellbee what was the verdict on this? It looks like all the issues got closed, but no resolution was produced.

@ellbee
Copy link
Collaborator

ellbee commented Feb 3, 2016

Check this PR: reduxjs/react-redux#279

@gaearon
Copy link

gaearon commented Feb 4, 2016

React Redux 4.3.0 allows per-instance memoization now.
reduxjs/react-redux#279

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants