Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants