Skip to content
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

Move the side effects out of shouldComponentUpdate() #225

Merged
merged 3 commits into from
Dec 22, 2015
Merged

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Dec 22, 2015

This attempts to completely remove any internal side effects from shouldComponentUpdate() because is not in line with how React tells us to treat it and may not even be called in certain cases, e.g. during hot reloading.

I didn't touch any tests, and that they pass is a good sign because it means we didn't regress over #99 which is where those side effects were moved to shouldComponentUpdate(). Here, instead, I moved them to render(), and applied a different optimization (namely, returning a constant element) to achieve the same effect as shouldComponentUpdate() did while always calling mapStateToProps and mapDispatchToProps with up-to-date props.

I confirm this PR fixes #224. It should not regress on performance.

@epeli Would you like to review?

gaearon added a commit that referenced this pull request Dec 22, 2015
Move the side effects out of shouldComponentUpdate()
@gaearon gaearon merged commit db78da8 into master Dec 22, 2015
@gaearon gaearon deleted the rewrite branch December 22, 2015 12:42
@gaearon
Copy link
Contributor Author

gaearon commented Dec 22, 2015

Actually I'm much happier with this code now :-). I feel like we're not working around but with React now.

@esamattis
Copy link
Contributor

Yeah, calling map state functions in shouldComponentUpdate felt bit like a hack to me too but I just wanted to make a smallest possible change in order to fix the issue at hand and didn't think it would be an issue because the map state functions should be pure too.

But I recall that in somewhere in React docs they advise against keeping references to created React elements so I wonder whether it would be better to just cache the data?

@gaearon
Copy link
Contributor Author

gaearon commented Dec 23, 2015

No, caching React element is an advanced perf pattern. (People usually do this for wrong reasons so it's not recommended.) The thing is, it works like shouldComponentUpdate(). React will bail out of reconciliation if we return the same element. That's why my change is not a perf regression.

@esamattis
Copy link
Contributor

Nice. This should make perf even a bit better.

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