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

Why only mergeProps called when component isn't pure? #118

Closed
vslinko opened this issue Sep 25, 2015 · 6 comments · Fixed by #122
Closed

Why only mergeProps called when component isn't pure? #118

vslinko opened this issue Sep 25, 2015 · 6 comments · Fixed by #122

Comments

@vslinko
Copy link

vslinko commented Sep 25, 2015

First of all, I'm using Relay. It means that redux own only part of my state and I have a problem with it.

I wrapped my App component using connect because I want to get locale info from store.
If I keep pure option as true then connect prevents re-rendering, because nothing changed in redux.
But, If I set pure option into false, connect doesn't recall mapStateToProps and mapDispatchToProps, so stateProps and dispatchProps are cached forever.

Is it a bug?

https://github.com/rackt/react-redux/blob/master/src/components/createConnect.js#L93

@gaearon
Copy link
Contributor

gaearon commented Sep 25, 2015

Might be. Can you submit a failing test?

@esamattis
Copy link
Contributor

Seems like a bug to me. I think this.updateStateProps(nextProps) should be called inside the if (!pure) { ... } statement before this.updateState(nextProps).

I'll fix this in couple of days (if you don't beat me to it with a PR). Meanwhile can you try whether it fixes the issue for you?

@vslinko
Copy link
Author

vslinko commented Sep 25, 2015

I think it also should include this.updateDispatchProps(nextProps)

@vslinko
Copy link
Author

vslinko commented Sep 25, 2015

Meanwhile can you try whether it fixes the issue for you?

I found a workaround, so I can't test it right now, but I sure it will work.

esamattis added a commit to esamattis/react-redux that referenced this issue Sep 28, 2015
esamattis added a commit to esamattis/react-redux that referenced this issue Sep 28, 2015
@esamattis
Copy link
Contributor

@vslinko can you try whether #122 fixes this for you.

esamattis added a commit to esamattis/react-redux that referenced this issue Sep 28, 2015
esamattis added a commit to esamattis/react-redux that referenced this issue Sep 28, 2015
esamattis added a commit to esamattis/react-redux that referenced this issue Sep 28, 2015
esamattis added a commit to esamattis/react-redux that referenced this issue Sep 28, 2015
@vslinko
Copy link
Author

vslinko commented Sep 28, 2015

@epeli it works, thank you

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 a pull request may close this issue.

3 participants