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

Prevent mapState from running twice when component is bound #196

Closed
wants to merge 2 commits into from

Conversation

schmeedy
Copy link

First, there's high probability this pull request is actually wrong and there's some fundamental thing in React / Redux that I violate by the proposed change given there are multiple unit tests which codify current approach. I still think there's some problem with it, though, so I appreciate if you look into this!

I'm trying to prevent the mapState function from being called twice on each initial render of a "connected component" - first in the constructor and then right after that in shouldComponentUpdate where stateProps are recomputed because of difference in store state (as initially stored store state is always null). This second call can be avoided by simply initializing this.state.storeState with current store state during construction.

Is this a viable solution? If not, is there some other way of preventing mapState to be called twice even though the state itself hasn't changed in between those two calls?

@gaearon
Copy link
Contributor

gaearon commented Nov 24, 2015

This looks good to me.
@epeli What do you think about this change? Any reason storeState was initialized to null in b627269?

@esamattis
Copy link
Contributor

Sorry, I missed this. I seem to get too much noise from this repo so I tend to ignore bunch of react-redux related emails... :(

I was actually confused by this also when I wrote b627269. It's weird indeed that during the first render the mapstate is called multiple times. I did plan to send PR like this earlier but never found the time to do it.

Any reason storeState was initialized to null in b627269?

Can't remember exactly but my thinking was something along the lines that the state is required only when the component is mounted. Not during initialization.

But this PR looks good anyway only if React connect should throw an error if the store is not in the props or context test would not fail :)

@schmeedy
Copy link
Author

The failing test was unrelated to this PR, but I've fixed it. Linter still fails (it's failing in master too) - not sure how to fix that.

@ellbee ellbee mentioned this pull request Nov 27, 2015
@gaearon
Copy link
Contributor

gaearon commented Dec 12, 2015

Can you please rebase on the latest master? Sorry for the trouble.

@gaearon
Copy link
Contributor

gaearon commented Dec 20, 2015

Fixed via 0c05c01 and a8c1abf.

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

Successfully merging this pull request may close these issues.

3 participants