[BUG] Component connected to state with `mapStateToProps` #1816

Closed
vieks opened this Issue Jun 17, 2016 · 10 comments

Comments

3 participants
@vieks

vieks commented Jun 17, 2016

I have only one connected component at the top level for the whole application.

I made some tests, and the problem is the following:

I have 2 reducers (locale and products). Only one of them is connected to the app with mapStateToProps.

const mapStateToProps = state => ({
    locale: state.get('locale'),
});

But if I update the products reducer that is not connected to the app, The mapStateToProps is anyway called and so on it will trigger my application re-rendering !

Why this behavior since products is for sure updated on my store but is not connected to the app ???
Normally it should skip the re-rendering process since that state part is not mapped to any props.

Versions:
"react-redux": "^4.4.5",
"redux": "^3.5.2",

@vieks vieks closed this Jun 17, 2016

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 17, 2016

Contributor

That's exactly the point of what mapStateToProps does. Redux only has a single "change" event, and notifies all subscribers every time an action is dispatched. Since Redux itself doesn't track what actual changes might have been made to the state, it's up to each individual subscriber to determine if anything meaningful has changed.

The wrapper component generated by connect() does this by calling mapStateToProps for every action dispatched, and comparing the new mapState result to the previous mapState result. If they're shallow-equal, then any changes to the store state obviously aren't relevant to this component, and it doesn't force a re-render of the wrapped "real" component.

Contributor

markerikson commented Jun 17, 2016

That's exactly the point of what mapStateToProps does. Redux only has a single "change" event, and notifies all subscribers every time an action is dispatched. Since Redux itself doesn't track what actual changes might have been made to the state, it's up to each individual subscriber to determine if anything meaningful has changed.

The wrapper component generated by connect() does this by calling mapStateToProps for every action dispatched, and comparing the new mapState result to the previous mapState result. If they're shallow-equal, then any changes to the store state obviously aren't relevant to this component, and it doesn't force a re-render of the wrapped "real" component.

@vieks

This comment has been minimized.

Show comment
Hide comment
@vieks

vieks Jun 18, 2016

@markerikson ok thanks. So how can I prevent a re-render for a reducer ?
For example 'products' reducer receive dispatched actions, so the state is updated, but I don't want that my connect function trigger an UI update. In fact I just want update that reducer but never trigger UI refresh or receive props due to that update.

Any ideas ?

vieks commented Jun 18, 2016

@markerikson ok thanks. So how can I prevent a re-render for a reducer ?
For example 'products' reducer receive dispatched actions, so the state is updated, but I don't want that my connect function trigger an UI update. In fact I just want update that reducer but never trigger UI refresh or receive props due to that update.

Any ideas ?

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 18, 2016

Contributor

That's... not how Redux works.

Again, every time you call store.dispatch(), Redux will execute your top-level reducer function (which usually calls a bunch of nested functions to break down the work). When that reducer returns, Redux doesn't know (or care) what parts of the state tree might have changed. It just executes every subscriber callback to let them know that an action was dispatched, and something might have changed.

The React Redux connect() function creates a wrapper component class. Every single instance of a connected component will create its own subscription to the store. (A simplified version of `connect() can be seen at https://gist.github.com/gaearon/1d19088790e70ac32ea636c025ba424e, which shows the basic behavior and implementation.) So, if I have 2 connected component classes, and 5 instances of each component in my page, there will be 10 individual component subscribed to the store. Each one needs to know that the store has probably changed, and see if there's any new data that that specific component instance wants to use.

For each component instance, mapStateToProps will be run with the next state data. If the mapState function returns the same data as the last time it was called, then connect() won't make the real component re-render. If the data returned from mapState did change, then the component may need to update itself. connect() will pass the new props to the real component, which can then use React methods like shouldComponentUpdate to determine if it really really needs to re-render or not.

You seem to be overly concerned about avoiding renders. It is very unlikely that this is something you need to worry about right now. If you'd like to know more about React performance, I have links to many good articles about that topic at https://github.com/markerikson/react-redux-links/blob/master/react-performance.md.

Ultimately, in a React+Redux application, you want your components to re-render when the store updates, if the data for a component has changed. connect() does that for you. If that's not what you want, then you shouldn't be using connect() (and if it's not, then what are you actually trying to do in the first place?).

Contributor

markerikson commented Jun 18, 2016

That's... not how Redux works.

Again, every time you call store.dispatch(), Redux will execute your top-level reducer function (which usually calls a bunch of nested functions to break down the work). When that reducer returns, Redux doesn't know (or care) what parts of the state tree might have changed. It just executes every subscriber callback to let them know that an action was dispatched, and something might have changed.

The React Redux connect() function creates a wrapper component class. Every single instance of a connected component will create its own subscription to the store. (A simplified version of `connect() can be seen at https://gist.github.com/gaearon/1d19088790e70ac32ea636c025ba424e, which shows the basic behavior and implementation.) So, if I have 2 connected component classes, and 5 instances of each component in my page, there will be 10 individual component subscribed to the store. Each one needs to know that the store has probably changed, and see if there's any new data that that specific component instance wants to use.

For each component instance, mapStateToProps will be run with the next state data. If the mapState function returns the same data as the last time it was called, then connect() won't make the real component re-render. If the data returned from mapState did change, then the component may need to update itself. connect() will pass the new props to the real component, which can then use React methods like shouldComponentUpdate to determine if it really really needs to re-render or not.

You seem to be overly concerned about avoiding renders. It is very unlikely that this is something you need to worry about right now. If you'd like to know more about React performance, I have links to many good articles about that topic at https://github.com/markerikson/react-redux-links/blob/master/react-performance.md.

Ultimately, in a React+Redux application, you want your components to re-render when the store updates, if the data for a component has changed. connect() does that for you. If that's not what you want, then you shouldn't be using connect() (and if it's not, then what are you actually trying to do in the first place?).

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 18, 2016

Contributor

Actually, backing up to your original statement:

I have 2 reducers (locale and products). Only one of them is connected to the app with mapStateToProps

If the second reducer isn't being used by any of your components, then what are you doing with that data?

Contributor

markerikson commented Jun 18, 2016

Actually, backing up to your original statement:

I have 2 reducers (locale and products). Only one of them is connected to the app with mapStateToProps

If the second reducer isn't being used by any of your components, then what are you doing with that data?

@vieks

This comment has been minimized.

Show comment
Hide comment
@vieks

vieks Jun 18, 2016

At first thanks a lot for your answer and the time taken 👍

Let me explain what I am trying to do:
I use ImmutableJS for my store, and I want to put absolutely everything inside it, comprising the app state (for inter-component communications) and component local state (responding to events, small UI update, etc...).
The purpose is to obtain a Monolithic immutable state, where all the application data is stored as a only one source of truth. The main advantage is to implement an advanced time-travel, sync for each user interactivity. And also It's easy and more pleasant to reason about a single place where all the logic and data lives on.

So how I have started to implement it:
I have all my normal reducers than manage the app state (global state/inter-component com. state/whatever the name...). That state is the intended behavior for a Redux app. So when I dispatch an action, the store is updated, and connect thanks to mapStateToProps maps new data state to props at a single top level component wrapping all my app. Then, for performance and advantage of ImmutableJS, I implement shouldComponentUpdate.

The behavior is OK, all works fine. The problem is for my local state management that is also connected to my store.

For example, when a onClick event is triggered, I will dispatch an action, so the store is updated. But now, and that's the problem, even if the part of my state is not linked to props, the behavior of mapStateToProps will trigger a re-render of all my apps !!! And this is really bad, even if I implement shouldComponentUpdate for components, Its truly overkill for performance to pass through all the components hierarchy in order to respond to a cheap local state (per component state).

My goal, is obviously to trigger the re-render of my actual component only, with the benefit of the 'all data/state in a monolithic place'.
So you will tell me how I access my local state since this one is put in my store and at the same time I want to short-circuit the connect mapping props behavior ?
In fact, store is a singleton per application, so I keep its reference inside my app, or It's also possible to get an access to it with Redux context but that is not the solution I retained.

So per component I have that method:

  getState() {
    return store.getState().getIn(['local', 'myComponent']);
  }

And I trigger the update with: this.setState({}) at component level and use also `shouldCompoenentUpdate``. How can I compare the modifications from the new state with the old state ? I only implement on the following react lifecycle method:

  componentDidUpdate() {
    this.lastState = this.getState();
  }

So everything works fine, permitting me to have nice performance, and my local state is stored inside the Redux store so its synchronized with it, while I still trigger component re-rendering at component level. But this last point is the problem, since connect will also trigger to my local state from the top-down...

vieks commented Jun 18, 2016

At first thanks a lot for your answer and the time taken 👍

Let me explain what I am trying to do:
I use ImmutableJS for my store, and I want to put absolutely everything inside it, comprising the app state (for inter-component communications) and component local state (responding to events, small UI update, etc...).
The purpose is to obtain a Monolithic immutable state, where all the application data is stored as a only one source of truth. The main advantage is to implement an advanced time-travel, sync for each user interactivity. And also It's easy and more pleasant to reason about a single place where all the logic and data lives on.

So how I have started to implement it:
I have all my normal reducers than manage the app state (global state/inter-component com. state/whatever the name...). That state is the intended behavior for a Redux app. So when I dispatch an action, the store is updated, and connect thanks to mapStateToProps maps new data state to props at a single top level component wrapping all my app. Then, for performance and advantage of ImmutableJS, I implement shouldComponentUpdate.

The behavior is OK, all works fine. The problem is for my local state management that is also connected to my store.

For example, when a onClick event is triggered, I will dispatch an action, so the store is updated. But now, and that's the problem, even if the part of my state is not linked to props, the behavior of mapStateToProps will trigger a re-render of all my apps !!! And this is really bad, even if I implement shouldComponentUpdate for components, Its truly overkill for performance to pass through all the components hierarchy in order to respond to a cheap local state (per component state).

My goal, is obviously to trigger the re-render of my actual component only, with the benefit of the 'all data/state in a monolithic place'.
So you will tell me how I access my local state since this one is put in my store and at the same time I want to short-circuit the connect mapping props behavior ?
In fact, store is a singleton per application, so I keep its reference inside my app, or It's also possible to get an access to it with Redux context but that is not the solution I retained.

So per component I have that method:

  getState() {
    return store.getState().getIn(['local', 'myComponent']);
  }

And I trigger the update with: this.setState({}) at component level and use also `shouldCompoenentUpdate``. How can I compare the modifications from the new state with the old state ? I only implement on the following react lifecycle method:

  componentDidUpdate() {
    this.lastState = this.getState();
  }

So everything works fine, permitting me to have nice performance, and my local state is stored inside the Redux store so its synchronized with it, while I still trigger component re-rendering at component level. But this last point is the problem, since connect will also trigger to my local state from the top-down...

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 18, 2016

Contributor

Well, there's pros and cons to putting data into your Redux store. There's times you'd want to put data that's local to a specific component into your store, and there's times you might not want to. You're seeing some of the reasons why it can be something to avoid.

Several major questions:

  • Why have you decided to put every last bit of your state into Redux? Again, you can, and it very often is indeed a good thing, but like anything in programming, there are tradeoffs you have to consider. Is there a reason you're not using any local component state? I do see you're aiming for time traveling, but do you really need every bit of data in Redux?
  • You said you're using Immutable.js. Obviously you have to extract the raw JS data in order to actually render successfully. Where and how are you doing that? Are you doing, say, this.props.someImmutableObject.getIn("someField")? Are you calling toJS() somewhere? If so, where are you calling toJS()? That can drastically affect performance.
  • Are you seeing actual definite performance problems? If so, how are you measuring? Have you profiled your code? Are you seeing physically slow rendering? Or is this all still a theoretical concern?
  • You.... oh. You say that you've only connected a single top-level component? That will definitely cause re-renders down your tree. You should be connecting many times at lower levels of your component hierarchy for better performance. Why are you doing that?

Again, I want to emphasize: programming involves tradeoffs. It sounds like you've made certain architectural decisions already, and those decisions come with tradeoffs. It also sounds like you only have a partial understanding of how Redux and React Redux work. Understanding how those tools work is going to be a fundamental prerequisite for understanding the tradeoffs you've already chosen, even if you didn't deliberately think about choosing them.

I'll re-encourage you to read through the articles in the list I linked, especially since you're using Immutable.js - that brings in its own set of tradeoffs, and my list has several articles discussing those.

Contributor

markerikson commented Jun 18, 2016

Well, there's pros and cons to putting data into your Redux store. There's times you'd want to put data that's local to a specific component into your store, and there's times you might not want to. You're seeing some of the reasons why it can be something to avoid.

Several major questions:

  • Why have you decided to put every last bit of your state into Redux? Again, you can, and it very often is indeed a good thing, but like anything in programming, there are tradeoffs you have to consider. Is there a reason you're not using any local component state? I do see you're aiming for time traveling, but do you really need every bit of data in Redux?
  • You said you're using Immutable.js. Obviously you have to extract the raw JS data in order to actually render successfully. Where and how are you doing that? Are you doing, say, this.props.someImmutableObject.getIn("someField")? Are you calling toJS() somewhere? If so, where are you calling toJS()? That can drastically affect performance.
  • Are you seeing actual definite performance problems? If so, how are you measuring? Have you profiled your code? Are you seeing physically slow rendering? Or is this all still a theoretical concern?
  • You.... oh. You say that you've only connected a single top-level component? That will definitely cause re-renders down your tree. You should be connecting many times at lower levels of your component hierarchy for better performance. Why are you doing that?

Again, I want to emphasize: programming involves tradeoffs. It sounds like you've made certain architectural decisions already, and those decisions come with tradeoffs. It also sounds like you only have a partial understanding of how Redux and React Redux work. Understanding how those tools work is going to be a fundamental prerequisite for understanding the tradeoffs you've already chosen, even if you didn't deliberately think about choosing them.

I'll re-encourage you to read through the articles in the list I linked, especially since you're using Immutable.js - that brings in its own set of tradeoffs, and my list has several articles discussing those.

@vieks

This comment has been minimized.

Show comment
Hide comment
@vieks

vieks Jun 18, 2016

Ok, I will answering you later. Before as you advised me I will read some articles/thread about it. In fact, I just want to build the best solution for performance.
I read from http://somebody32.github.io/high-performance-redux/ :

Remember!
mapStateToProps is the new render. Called on every store update.

Based on the most recent tests, feeback, etc... Do you have a link explaining the best architecture to implement relating to the above citation ?

And I use toJS() in render method, is it really slow ?

vieks commented Jun 18, 2016

Ok, I will answering you later. Before as you advised me I will read some articles/thread about it. In fact, I just want to build the best solution for performance.
I read from http://somebody32.github.io/high-performance-redux/ :

Remember!
mapStateToProps is the new render. Called on every store update.

Based on the most recent tests, feeback, etc... Do you have a link explaining the best architecture to implement relating to the above citation ?

And I use toJS() in render method, is it really slow ?

@vieks

This comment has been minimized.

Show comment
Hide comment
@vieks

vieks Jun 18, 2016

You say that you've only connected a single top-level component? That will definitely cause re-renders down your tree.

I don't understand why react-redux doesn't allow us to attach a function to prevent re-rendering based on our additional logic.

vieks commented Jun 18, 2016

You say that you've only connected a single top-level component? That will definitely cause re-renders down your tree.

I don't understand why react-redux doesn't allow us to attach a function to prevent re-rendering based on our additional logic.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 18, 2016

Contributor

The general impression I've gotten is that toJS() is relatively slow in general, because Immutable.js has to traverse its own complex internal data structures to pull together the new plain JS object value. If you're going to call that, you'd want to leave the data in Immutable form as long as possible (especially so you can get the benefits of cheaper shouldComponentUpdate comparisons), and only call toJS() at the last minute. So yeah, probably in render(), in your leaf nodes.

Architecture-wise: the evidence is that using connect() on many components, with each one extracting the small bits of data they need from the Redux state, is more performant that only connecting at the top level and trying to pass down everything as props all the time from that one component. The "High Performance Redux" slideshow demonstrates that approach.

I don't understand why react-redux doesn't allow us to attach a function to prevent re-rendering based on our additional logic.

Again, that's half the purpose of mapStateToProps. If you have a connected component, and neither props from its parent nor extracted data from the store have changed, then the connected component clearly doesn't need to re-render. If you need to do an additional check beyond that, you can implement shouldComponentUpdate on your "real" component, which would then be able to look at the full incoming props (parent props + store data props) and make a further decision about whether or not it needs to re-render.

Contributor

markerikson commented Jun 18, 2016

The general impression I've gotten is that toJS() is relatively slow in general, because Immutable.js has to traverse its own complex internal data structures to pull together the new plain JS object value. If you're going to call that, you'd want to leave the data in Immutable form as long as possible (especially so you can get the benefits of cheaper shouldComponentUpdate comparisons), and only call toJS() at the last minute. So yeah, probably in render(), in your leaf nodes.

Architecture-wise: the evidence is that using connect() on many components, with each one extracting the small bits of data they need from the Redux state, is more performant that only connecting at the top level and trying to pass down everything as props all the time from that one component. The "High Performance Redux" slideshow demonstrates that approach.

I don't understand why react-redux doesn't allow us to attach a function to prevent re-rendering based on our additional logic.

Again, that's half the purpose of mapStateToProps. If you have a connected component, and neither props from its parent nor extracted data from the store have changed, then the connected component clearly doesn't need to re-render. If you need to do an additional check beyond that, you can implement shouldComponentUpdate on your "real" component, which would then be able to look at the full incoming props (parent props + store data props) and make a further decision about whether or not it needs to re-render.

@DavidHe1127

This comment has been minimized.

Show comment
Hide comment
@DavidHe1127

DavidHe1127 Sep 24, 2017

Thanks @markerikson for all clarifications

Thanks @markerikson for all clarifications

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment