acdlite / redux-router

Redux bindings for React Router – keep your router state inside your Redux store
MIT License
2.3k stars 216 forks source link

unexpected redraws #241

Closed davibe closed 8 years ago

davibe commented 8 years ago

I have a page with

When the user types his query i trigger a pushState({}, currentRoute, {query: e.target.value}) because i want the written text to appear on the url bar so that the use can copy-paste the page around. I love how it works but when the list of search results includes many items they are all redrawn right when pushState action is dispatched (even if their dispatchToProps does not return anything different at all).

The reason i think it is that the top-level Route component sees a change in its state/props (a change of state.route.location.query) and this results in a (react) redraw of all the child elements.

One solution would be to write my list items in Class style (non functional) and override the shouldComponentUpdate method. However this is not how redux is supposed to work. If the state is not mutated in dispatchToProps there should be no redraw at all.

What's the right solution for this ?

Scarysize commented 8 years ago

Hey, you are better off posting this to stackoverflow to reach people beyond the not-too-active maintainers of this repo ;-) I will give it look if I have have the time.

davibe commented 8 years ago

the fact that something as important as redux-router may be not-too-active sounds scary

mjrussell commented 8 years ago

The reason i think it is that the top-level Route component sees a change in its state/props (a change of state.route.location.query) and this results in a (react) redraw of all the child elements.

One solution would be to write my list items in Class style (non functional) and override the shouldComponentUpdate method. However this is not how redux is supposed to work. If the state is not mutated in dispatchToProps there should be no redraw at all.

You are hitting the problem right on the head, but this is exactly how redux and react are supposed to work together (and is not really an issue with Redux-Router). The connect method from redux will invoke the re-render of the route level component because of the state change (Im assuming you are getting location from either React Router prop passed down or the store directly), and it is your responsibility to determine how far that re-render travels down the component tree (using shouldComponentUpdate to stop it). You say the state is not mutated but state.route.locaton.query is being changed means that there is indeed a state change.

IMO its a good idea to use shouldComponentUpdate on leaf nodes such as the items in the list to avoid redraws since it can be a significant performance hit. You should also only render the visible rows of your search result and do some minimal buffering, something similar to this. That way re-renders only render the visible sections

davibe commented 8 years ago

I discovered that on any action i trigger (even if it does not have anything to do with the router) the router state mutates

screenshot 2016-03-05 10 11 37
mjrussell commented 8 years ago

Hmm thats interesting...I'd be surprised if the routes were actually changing. Seems more likely that the tool you are using doesn't support non-serializable state (which the components and routes are) and is giving false positives. But thats just a guess

Scarysize commented 8 years ago

@davibe Is this still an issue or did you resolve it some how? Let us know!

davibe commented 8 years ago

as @mjrussell said it was probably the tool. Anyway i ended up storing the state in a redux router and only changing the url when strictly needed to avoid performance hits.