Potential connect() optimization

[gaearon]

As described in reduxjs/redux#1437 (comment), we can try introducing a separate code path for the fast case where neither state props nor dispatch props depend on the own props. In this case the problem from #99 seems irrelevant and we can do the fast check before calling setState().

I am not convinced that this is the best solution however. It basically forces computations to happen in the handleChange() hook which React has no control over. If in the future React learns to prioritize updates, it will not be able to schedule this work for later, but if we keep things as they are today, it will be able to do this just by postponing render().

[matianfu]

Will another callback as connect()'s parameter do the job? But I haven't yet figure out why you choose a connect() function, rather than a class to be sub-classed and mixed in to construct the container class. In this case, adding feature will be more smoothly without changing any existing code.

[gaearon]

But I haven't yet figure out why you choose a connect() function, rather than a class to be sub-classed and mixed in to construct the container class. In this case, adding feature will be more smoothly without changing any existing code.

Inheritance is not really a thing in React ecosystem, and for good reasons. It makes it too easy to break consuming packages, introduces naming conflicts between methods, is not composable, and in general exposes too many implementation details.

[gaearon]

Will another callback as connect()'s parameter do the job?

Why would we need it? I meant changing the internal implementation without touching the API. But as I said, I have concerns even about that.

[matianfu]

Why would we need it? I meant changing the internal implementation without touching the API. But as I said, I have concerns even about that.

I understand that you'd like to keep connect() simple. But existing api really does a lot of thing already.

mapState is for calculating derived value from store state. mapDispatch is providing actions.

---

The most distinct nature of React, IMHO, is not the much talked asynchronous. It is essentially an Inversion of Control Pattern. All life cycle method and render() is called by React, not by app code. The only thing app code can do is to call the setState() method.

This is something quite similar to Mark & Sweep garbage collector. There are two stages, in the first stage all referenced objects are marked. In the second stage all unmarked objects are recycled.

From the viewpoint of app code, React render can also be considered as two stages. The first stage can also be called "mark", which means, mark the components which should be refreshed. The second stage is the React's reconciliation.

In current status of code. There is no selective mark at all. All container/stateful component are marked.

Why we need extra api? I think this is a new and independent logic, irrelevant to existing function definition. So it's better to have something independent to others. Do one thing, do it well.

---

Also, I have not figure out why mapStateToProps() need ownProps argument. Is it because that it's so popular to write JS in JSX props field? such as:

<MyComponent xyz={...javascript code} />

To my understanding, if redux store is a complete state model of the app, all props are just derived value calculated from the store, so why they are going to be passed to a stateful container component?

I also remembered you've said in your tutorial: UI will be mostly predictable if it could be described as a pure function on states. Now we have redux store and reducers. It is exactly the container's job to implement such 'pure function' translating states to UI. But in connect() implementation, I can't see anything suggesting pure. It seems to be unnecessarily complex.

I have read the code and watched the you vidoe at readthesource over ten times. But there are still a lot of question mark haunting in my head. I need more time to figure them out.

[markerikson]

Also, I have not figure out why mapStateToProps() need ownProps argument.

Because a given connected component may also have props given to it by its parent:

return <SomeConnectedComponent otherProp1={42} otherProp2="abcd" />

and then mapStateToProps may depend on some of those props to actually extract the right pieces from state. The most obvious case is a component that can have multiple instances, and each instance is passed a different ID value as a prop. A good example of this is can be seen in the "tree-view" sample: https://github.com/reactjs/redux/blob/master/examples/tree-view/containers/Node.js#L75 .

A Redux store is not necessarily the entire state of an application. It can be, if you've designed your app that way. It may contain UI state. It may only contain cached data. There may be local component state, or there may not be. It entirely depends on the application and the developers. But it certainly is not the case that the only props for a connected component would be coming from the store.

Beyond that, while "pure functional" components are a nice ideal, in practice many components are not. Redux and React-Redux do not try to force you to write components in an absolute purely functional way, although they do encourage that approach in general.

Overall, it seems like you're over-thinking the issue. connect() provides a way to pass a subset of the store state as props to a component. It has a number of optimizations to cut down on the number of times your "real" component has to re-render. There may be further improvements that can be made, but the techniques used in the code now are there for good reasons.

[matianfu]

It has a number of optimizations to cut down on the number of times your "real" component has to re-render.

This is why I dive into the code. I want to know what kind of optimization it provides and how the code could benefit from it.

Correct me if I am wrong. In current implementation:

handleChange() {
        if (!this.unsubscribe) {
          return
        }

        const prevStoreState = this.state.storeState
        const storeState = this.store.getState()

        if (!pure || prevStoreState !== storeState) {
          this.hasStoreStateChanged = true
          this.setState({ storeState })
        }
      }

handleChange() is subscribed to redux store after the component is mounted. As long as store state changes, it will trigger setState() to itself. Then:

      shouldComponentUpdate() {
        return !pure || this.haveOwnPropsChanged || this.hasStoreStateChanged
      }

Since this.hasStoreStateChanged has been set to true in previous function, this function will return true as long as store state changes. Then render() must be triggered.

That is to say: React life cycle methods that are intentionally designed for minimizing re-render() are totally bypassed in React-Redux binding. It is essentially doing forceUpdate() on ALL container components as long as store state changes!

I think this is why the react element tree caching is implemented. If not, the performance will be horrible. That can hardly be called an optimization. I consider it as a remedy to the design/implementation flaw in the first place. Sorry to say that but I do think so. Such caching is not necessary if setState() / shouldComponentUpdate() are implemented correctly. The connect() code should be much simpler.

[davibe]

I am facing performances issues. Many of my components (ListItemComponent) are updating even if their props are not changed.. They are being redrawn because they are connect()-ed. Connect's default implementation of shouldComponentUpdate triggers a redraw when the store changes even if the props of the component itself are all the same.

I don't get the reasons behind this choice but it's clearly a huge problem in my application and it kind of makes stateToProps+functional component useless. The only way i can fix this is to turn my component into a class-based component so i can write my own shouldComponentUpdate.

Can someone explain to me why we want to force component redraw when store changes ?

[markerikson]

@matianfu , @davibe : I think your concerns are based on a misunderstanding of how the Connect wrapper component actually works. Note that there is a distinction between "the Connect wrapper component re-renders" and "the wrapped original component re-renders".

The wrapper component does not actually call your mapStateToProps function as part of the shouldComponentUpdate logic. That call is deferred as long as possible, and actually only happens when the wrapper component re-renders. Per https://github.com/reactjs/react-redux/blob/master/src/components/connect.js#L248-L270 , during the wrapper rendering process, it re-runs your mapStateToProps, and checks to see if the merged props have actually changed since last time. If they have not (like, say, if an unrelated portion of the state store changed), then the Connect component directly returns the cached React element, and does not even tell your component to re-render.

So, what that means is that we DO want the Connect wrapper component to re-render if the store has changed, but that does NOT automatically mean that your own component will actually re-render as well. It just means that things MAY be different, and now the Connect component needs to do additional checking based on the store and mapStateToProps.

@davibe , you may need to do some additional looking at your components. Remember that Connect only does shallow equality checking by default - if you're returning new instances in your mapStateToProps (such as using someArray.map()), those will always be seen as different and force a re-render of your component. It might help to use a debug tool that indicates why a component re-rendered, such as https://github.com/redsunsoft/react-render-visualizer , https://github.com/spredfast/react-transform-render-visualizer , or the "WhyDidYouUpdateMixin" listed at http://benchling.engineering/deep-dive-react-perf-debugging/ .

[davibe]

@markerikson i already made sure that my stateToProps does not do funny things.

I discovered that my purely functional component only re-renders if i fire an action that changes the query params using redux-router replaceState. If i fire another action the store does change but my component does not actually rerender (which is good). There must be something that react-router does which is unrelated to redux. As you explained it the connect wrapping element should be able to prevent the normal 'react' way of updating the components, but it's not

[gaearon]

@davibe

Yes, I think this is related to the context change. In this case returning constant element is not enough to bail out of reconciliation. That’s a rare case though so I wouldn’t worry too much about it.

[gaearon]

This is now fixed in the common case in [email protected].
For other cases, #348 contains a valuable tip.