search

reduxjs / reselect

Selector library for Redux
MIT License
19.04k stars 670 forks source link

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

Closed compulim closed 8 years ago

compulim commented 8 years ago

Assumption:

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 commented 8 years ago

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 commented 8 years ago

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:

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

oyeanuj commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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

compulim commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Thanks @ellbee, I think it's the same issue. Let's centralize the discussion to https://github.com/rackt/react-redux/pull/183.

pdf commented 8 years ago

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

ellbee commented 8 years ago

Check this PR: https://github.com/rackt/react-redux/pull/279

gaearon commented 8 years ago

React Redux 4.3.0 allows per-instance memoization now. https://github.com/rackt/react-redux/pull/279