Prevent ConnectedRouter from re-rendering on every redux store update

[brendonpagano]

As discussed in #205 the children tree of ConnectedRouter component was re-rendering on every update from the redux store.

I tried out @jakewies`s example repository and think that the problem is with the shallow compare of props coming from Redux.

This PR replaces the plain Component from ConnectedRouter for a PureComponent, which includes shallow compare of props, and prevents re-renders if they're equal.

[jakewies]

Nice eye @AnnaTsu . Judging from the code, there doesn't seem to be any downside to having ConnectedRouter extend PureComponent, but I don't have the full context of this project. Just want to bring it up so the reviewers of this PR can think of any edge cases, if any.

[firec0der]

@jakewies, everything is simple, any change in the redux tree triggers calls of all mapStateToProps of mounted components. Every connected component receives new props and calls render method. In my case, I have ConnectedRouter rendered almost on top of the whole application, and on every redux change, my application tries to rerender because ConnectedRouter doesn't check received props and calls render.

[madsmadsen]

Been running a similar version all day (#204 (comment)), does not seem to have any issues with having a shouldComponentUpdate method (Or PureComponent).

[brendonpagano]

It does exactly what @firec0der said. It isn't checking for prop changes, and this is why it keeps re-rendering.

[brendonpagano]

@madsmadsen So you aren't having extra renders while implementing shouldComponentUpdate or using PureComponent?

[madsmadsen]

@madsmadsen So you aren't having extra renders while implementing shouldComponentUpdate or using PureComponent?

Rerenders stopped from ConnectedRouter when shouldComponentUpdate or PureComponent was implemented.

[supasate]

Love this!

@AnnaTsu, would you mind add a unit test to prevent this bug in the future?

[brendonpagano]

I'll add it as soon as i get home, @supasate :)

[brendonpagano]

Hey!

I have written two tests. One to check if the component is rendered only once when mounted, and the other to check if it does not re-render when an action that has nothing to do with the history or router is fired.

However, I also tested modifying the ConnectedRouter from PureComponent into Component, and the results were the same! Going against what we tested in this POC where PureComponent rendered only once (correct behavior), instead of twice (incorrect behavior), like when Component was used.

I'm not very familiar with testing React components, so maybe this can be the problem. Can someone see if there's something wrong in the tests?

[madsmadsen]

@AnnaTsu It's a difficult one to test, you'll need to have the component rendered inside the Route to be connected to the store, probably subscribed to a property in the store. Update a different property in the store, and see if it triggers a rerender.

I do not currently have time to look more into it, but I might have time later this week.

[brendonpagano]

@madsmadsen I believe that's what I've done in this test. However, I couldn't reproduce the same behavior when testing in this POC.

ConnectedRouter in properly rendered independent of being a Component or PureComponent in the unit tests.

[jakewies]

@AnnaTsu Just so I understand what's going on here, store.dispatch() will not re-render RenderCounter, BUT history.push() will, correct? Therefore renderCount should be 2.

initial render + history.push() = 2 renders

[brendonpagano]

Exactly. Initial render times upon mount should be one.

Then, i fire an action that has no connection to what ConnectedRouter should listen to, and modify the history (something that the component should re-render).

So, the expected render times should be 2.

[jakewies]

However, I also tested modifying the ConnectedRouter from PureComponent into Component, and the results were the same! Going against what we tested in this POC where PureComponent rendered only once (correct behavior), instead of twice (incorrect behavior), like when Component was used.

@AnnaTsu could you explain how you went about testing this? Was it programmatically inside a test file or were you testing it in a consuming application, such as the POC?

Reason I'm asking is because I pulled down this branch and ran yarn link in order to test your implementation with my original POC locally and I'm getting an ugly error related to a missing "store". But I'm pretty certain this error has NOTHING to do with your PR and everything to do with yarn link.

EDIT: Nevermind this, I'm just testing programmatically now. I can see that changing the implementation of ConnectedRouter to:

class ConnectedRouter extends Component {}

is giving false positives because tests are passing. I'll continue to look into this.

[brendonpagano]

Yes, @jakewies. I tested the render times via the POC. By that, i saw that changing the ConnecterRouter into a PureComponent, it wasn't re-rendering on mount and on store changes.

However, the tests are passing even when ConnectedRouter is extending Component, which clearly doesn't happen in the application.

[jakewies]

Right, I'm seeing that now. The assumption is:

If ConnectedRouter extends Component, and an action is dispatched, then ConnectedRouter will re-render an extra time.

However, in your test the TEST_ACTION being dispatched doesn't cause a re-render:

store.dispatch({ type: 'testAction' })

This goes against our observations in the POC. I'm not sure why this is happening.

[jakewies]

Is there a possibility that PureComponent is not the solution? Yes, it seems to have solved the issues when observing it in the POC, but in an isolated unit test both Component and PureComponent act the same...

My hypothesis is that there is a distinct difference between the test environment and the POC, and that difference is what's causing the confusion and could lead to the actual solution.

I'm just spitballing here.

[SergeyPoluektov]

Re-renders come from dispatching a location change action for the initial location
This is a common flow:
initialize router state with location object from history ->
-> dispatch the location change action with the same location object from ConnectedRouter constructor ->
-> handle the location change in reducer by merging state and payload with location object.

As result state.location object didn't change reference as we use the same location object from history. So PureComponent is solution for plain JS structure, but it's not solution for immutable structure, because every Immutable.fromJS calls return new reference. So after handling initial location change action in reducer for immutable data state.location changed reference. Plus in selectors Immutable.toJS calls for location create new reference every time (use reselect to solve it).

I think the solution is add the meta info to onLocationChange action to describe is initial action or not. And don't handle initial LOCATION_CHANGE action in reducer.
And move initial location change action from constructor to componentDidMount, see React lifecycle docs, cause it's a reason of this infinite loop

[gitn00b1337]

Hi, what's the ETA on this being released? I upgraded react-redux-forms as they had a bug in the old version, and long story short it meant upgrading most packages incl. this. Because of all the dispatches from redux forms the page starts re-rending constantly, but to make it worse because the page reloads the dispatches happen again and .... infinite loop

I don't know if you have any work around for this, but I'd be happy to use a beta version if there's plans to release this change asap?

[supasate]

@jakewies Your error with yarn link may be caused by using different ReactReduxContext. The library itself uses ReactReduxContext from its local node_modules directory, while, the POC app uses ReactReduxContext from the POC app's node_modules directory.

Let's try passing the same ReactReduxContext imported directly from 'react-redux' (or a new context) to both Provider and ConnectedRouter.

[supasate]

@SergeyPoluektov the unit test in this PR also uses plain JS structure and it passes even without PureComponent.

[supasate]

@gitn00b1337 We should find the root cause first. So, we can fix at the right point. Otherwise, the implemented unit tests will be nothing and it may break again in the future.

I have no clue now about this behavior and will spend more time looking into it this weekend if it's still unresolved.

Eventually, if we still cannot understand the root cause (maybe within a week from now), we may need to release a hot fix first and make it right later. However, I'd like to spend a bit more time to find the root cause first before we go to this route.

[gitn00b1337]

Ok thanks for getting back to me, if you could post here when something is released I'd appreciate it greatly, thanks guys

[SergeyPoluektov]

@SergeyPoluektov the unit test in this PR also uses plain JS structure and it passes even without PureComponent.

First of all, this unit tests don't work correctly for a case when redux store updates by non-related action. Try this one render-test

After that, tell me please, what the reason behind passing action and location into ConnectedRouter?

[supasate]

what the reason behind passing action and location into ConnectedRouter?

Thanks for asking. It would be useless now. Seems like I just forgot to remove it in this commit when I migrated to a new react-router version.

[SergeyPoluektov]

@supasate so, removing this mapStateToProps in ConnectedRouter and inheritance it from React.PureComponent should help with unnecessary re-renders.

[jakewies]

@SergeyPoluektov I noticed some differences in your gist compared to the current test file, mainly your use of createBrowserHistory while the test file uses createMemoryHistory.

I'm curious to know your thought process behind this. Is this a meaningful change, or can the two be used interchangeably in tests?

[SergeyPoluektov]

@jakewies no, it's not meaningful change. Both of them can be used interchangeably in tests. Basically, differences in my gist compared to current files are about using non-mocked redux Provider and none-mocked store with router reducer. I did it to be close to real use cases.

[jakewies]

@SergeyPoluektov got it. Thanks for the explanation 👍

[jakewies]

I'm going to test the gist out in a few.

Based on @SergeyPoluektov insights it looks like the recommended changes are:

1. Remove mapStateToProps - ConnectedRouter doesn't use them
2. Move initial call to handleLocationChange from constructor to componentDidMount
3. Identify initial LOCATION_CHANGE action in reducer and don't do anything
4. Update tests to handle redux store updates by non-related action

Am I missing anything?

[SergeyPoluektov]

@jakewies one more improvement regarding immutable selectors:
5. Avoid converting toJS for router state fields equals by reference

[panvourtsis]

@supasate i am seeing something a bit weird in the reducer:21 when you try to merge the two objects together.
After testing it myself i saw that using Object.assign rather than merge solves the problem of re-rendering to components.
Something is messing up with the immutable you are using and how you extend it to the state (state.merge).
I could introduce a new PR but since you are using this to other parts of the component i think you could see if this is fix can be applied in a deeper level, maybe on your structure files.
This also solves #211

[headfire94]

Any updates?
Spent a few hours trying to understand why my app completely hangs out, seems like this pkg is unusable until you fix this

[supasate]

Hi all. Thanks for your insights and patience!

For this PR, let's keep it simple by focusing on fixing double rendering in general case first (exclude the initial LOCATION_CHANGE and the infinite loop problem). I'll fix other issues in separate PRs.

So, @AnnaTsu, the PureComponent works well and let's change this PR based on what @SergeyPoluektov suggested as follows:

1. Change the MockProvider to the real react-redux's Provider.
2. Remove action and location props.

I tested in #215 and it works well for plain, immutable, and seamless-immutable structures.

[brendonpagano]

Sure! Gonna do it in a moment 😄

I've had a long time without coming back to Github, it's good to come back and see the problem being solved by the community.

[brendonpagano]

Applied the changes you commented and now all tests are indeed passing 😄

Now we can change the ConnectedRouter back to a React.Component and see the tests failing!

Good job, everyone 🎉

[jakewies]

Can confirm that the "does not render again when non-related action is fired" test passes when ConnectedRouter extends PureComponent and fails (as expected) when extending React.Component. 👍🏻

[supasate]

LGTM!

[supasate]

Thanks everyone!!!

[Arrvi]

Can you release a minor version with this fix?

[supasate]

@Arrvi I'm fixing other issues related to rendering too. Will release it soon. Hang tight!

[supasate]

Released in v6.1.0 and Happy New Year 🎉!