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

Changes to context do not propagate #106

Open
arasmussen opened this issue Feb 8, 2017 · 14 comments
Open

Changes to context do not propagate #106

arasmussen opened this issue Feb 8, 2017 · 14 comments
Labels

Comments

@arasmussen
Copy link

I believe this is a problem with asyncConnect containers where updating context does not properly propagate updates to children. I had to create a wrapper around react-redux's connect to get context changes to propagate. Do I need to do something similar for asyncConnect?

@arasmussen
Copy link
Author

It looks like this library directly uses react-redux's connect, how would I go about injecting my own connect function instead? Or somehow passing the {pure: false} setting so it knows to update child components on context changes...

@arasmussen
Copy link
Author

Ended up making a similar wrapper for the asyncConnect decorator and context updates propagate once again:

import { asyncConnect } from 'redux-connect';

export default function myAsyncConnect(asyncItems, mapStateToProps, mapDispatchToProps) {
  return asyncConnect(asyncItems, mapStateToProps, mapDispatchToProps, null, {pure: false});
}

@arasmussen
Copy link
Author

Feel free to close, might be cool to add this to FAQ or somewhere indexable by Google for "context updates don't propagate asyncConnect / redux". Thanks for the awesome lib!

@AVVS
Copy link
Member

AVVS commented Feb 8, 2017

There is no need to wrap like this, asyncConnect accepts exactly the same args as connect, and its actually documented ;)

@AVVS
Copy link
Member

AVVS commented Feb 8, 2017

@AVVS AVVS added the question label Feb 8, 2017
@arasmussen
Copy link
Author

arasmussen commented Feb 8, 2017

Sure, but it's cleaner to just do

asyncConnect([
  Container.asyncConnect,
])

rather than

asyncConnect([
  Container.asyncConnect,
], null, null, null, {pure: false})

all over the codebase. Containers shouldn't have to remember to pass {pure: false} just to get context updates to propagate... Seems like a bug in react-redux's connect tbh.

@AVVS
Copy link
Member

AVVS commented Feb 8, 2017

Thats not a bug, its a performance feature. Pure means it does shallow props check and if no props changed then it wouldnt rerender. Shallow means it doesnt check nested properties of the object, but merely checks their references. Because context itself doesnt change, but instead it has a changed prop inside it - your component doesnt see it, hence it doesnt update without the pure: false setting. So if you set it to false you are really killing a lot of CPU of your users

@arasmussen
Copy link
Author

I'm not sure what you mean by context itself not changing though. My context changes from {obj: {foo: 'bar'}} to {obj: {foo: 'baz'}} and updates don't propagate to components who care about obj.foo and specify that in their contextTypes. How's that not a bug?

@AVVS
Copy link
Member

AVVS commented Feb 8, 2017

Read more here - https://github.com/reactjs/react-redux/blob/master/docs/api.md#connectmapstatetoprops-mapdispatchtoprops-mergeprops-options

Its a basic performance feature. Your context is not checked beyond the upmost references. It might be counter intuitive, but you have to understand that every time state changes - every connected component render will be called, and your state changes upon every action there is. So, effectively, when you set something to pure: false - this component will rerender every time your state changes. Literally on any action

@arasmussen
Copy link
Author

I am changing context via Object.assign({}, obj, {foo: 'baz'}) so it should be completely changing the reference. Maybe React is being "smart" and changing keys on the underlying object under the hood? I'll take a look at the link.

So, effectively, when you set something to pure: false - this component will rerender every time your state changes. Literally on any action

You mean if a sub-component changes state then the whole tree will re-render? What else can I do to get context updates to propagate without that perf hit?

@AVVS
Copy link
Member

AVVS commented Feb 8, 2017 via email

@arasmussen
Copy link
Author

Hmm. I use context for the current viewer and company (customer), pretty much everything else is passed via props. That being said, every single container I use that fetches data (14 of them) is set to {pure: false} to fix this context propagation issue throughout my app. I don't perceive any performance issues on desktop or mobile. Do you think this is a major issue? I don't really see another great workaround besides passing viewer/company down as props...

@AVVS
Copy link
Member

AVVS commented Feb 8, 2017

if they dont change often, that still should be fine, JS is obviously fast enough and would do hundreds of thousands of evaluations per second (modern CPUs 💃 ), so you wouldnt really notice it - all the underlaying components should still be pure and wont rerender

@arasmussen
Copy link
Author

arasmussen commented Feb 8, 2017

Yeah they pretty much only update when the user explicitly edits one of them and they get reloaded. Which is only on a few specific components (edit user, edit company, login, logout, etc). Cool man, thanks for your responsiveness and helpfulness!

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

No branches or pull requests

2 participants