elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.81k stars 8.2k forks source link

Discuss: Using redux containers prolifically vs only at a high level. #14547

Closed stacey-gammon closed 6 years ago

stacey-gammon commented 6 years ago

Goal

Hear opinions from people experienced in react and redux on whether they prefer to:

  1. Limit redux containers, using them only on top level components and passing props down to nested, non-redux linked inner components. or
  2. Limit the passing down of props and embrace the use of redux containers at all levels of the UI hierarchy.

By "redux containers", I'm referring to components that are wrapped in a connected component, which I historically suffix with "container"

Examples

Limiting redux container scope to top level component and pass props down

// DashboardPanelContainer.js
const mapDispatchToProps = (dispatch, { panelId }) => ({
  onEdit: () => dispatch(onEdit(panelId)),
});

export const DashboardPanelContainer = connect(
  mapDispatchToProps
)(DashboardPanel);

// DashboardPanel.js
export function DashboardPanel({ onEdit }) {
<div>
  <PanelHeader onEdit={onEdit} />
  <PanelContents />
</div>
}

// PanelHeader.js
export function PanelHeader({ onEdit }) {
<div>
  <PanelTitle />
  <PanelMenu onEdit={onEdit} />
</div>
}

// PanelMenu.j
export function PanelMenu({ onEdit }) {
<div>
 <button onClick={onEdit}>Edit</button>
</div>
}

Use redux containers in nested components to avoid passing props down

// DashboardPanel.js
export function DashboardPanel() {
<div>
  <PanelHeader />
  <PanelContents />
</div>
}

// PanelHeader.js
export function PanelHeader() {
<div>
  <PanelTitle />
  <PanelMenuContainer />
</div>
}

// PanelMenuContainer.js
const mapDispatchToProps = (dispatch, { panelId }) => ({
  onEdit: () => dispatch(onEdit(panelId)),
});

export const PanelMenuContainer = connect(
  mapDispatchToProps
)(PanelMenu);

// PanelMenu.js
export function PanelMenu({ onEdit }) {
<div>
 <button onClick={onEdit}>Edit</button>
</div>
}

Pros and Cons

Limiting containers to top level components

Pros:

Cons:

Embracing redux across all levels

Pros:

Cons:

kobelb commented 6 years ago

My reply to the original e-mail is as follows:

Coming from Flux before Redux, I've generally preferred to have one top-level "smart component" that is hooked into the store and passes all props through the component tree.

The only drawback that I've found to this approach is that you sometimes have to modify a large number of files to pass new data all the way through the component tree. This pain is generally remediated by using PropTypes which will help you wire the whole thing up, and make sure that you don't forget any. I'm interested to see what TypeScript offers us additionally.

I've grown quite fond of the legibility of this approach, and have found myself struggling when multiple components throughout are all connected to the Redux store and grab the data that they care about

sorenlouv commented 6 years ago

This question was also very relevant for us, when we started out on the APM plugin (which also uses Redux).

I ended up using connected components "when it made sense". Which turned out to be a lot. I start out by creating "dumb" presentational components, that are not directly connected to Redux, but when I start seeing props being passed through several layers of components (that don't need the props) only to reach a leaf component, I know I need to add a connected component somewhere.

An important aspect that I noticed is that performance (can) increase significantly when connecting components. Couple of reasons for this: 1) connect() creates a Pure HOC, that implements shouldComponentUpdate with a shallow check on props (compared to never doing the check, and always re-rendering). 2) By adding connected components down the tree allows leaf components to re-render, without having all their parents re-render as well (this is required if only the very top level component is connected).

I still strive to get as many "dumb" presentational components as possible, but when needed I connect them.

kimjoar commented 6 years ago

++ to what @sqren wrote above, that has been the exact same approach I've had. I've usually started with having a connected component per route, then adding more if/when needed (based on the "does this feel bad" heuristic).

However, if building components that are fully reusable outside of redux is a goal, we might need to sit down and work on this. E.g. how does ajax requests work in that world? Do they have to be moved "inside the components" to work? What about other actions? It feels like just moving connects higher in the tree will not be enough, as we need to handle both state and actions too.

w33ble commented 6 years ago

I've been partial to wrapping components and injecting data from Redux at every level I need it. You get to avoid needlessly passing props through many layers of child components, and it's generally been a little more clear component by component where your data is coming from.

In fact, we've been wrapping ALL of our components, even when they don't need access to the redux store. We treat containers as the place to put all of our state, local or Redux, and write all of our presentational components as SFC's. Overall I like the pattern, state can always be found in the container, components always have a container layer (even if they don't have state), and everything looks the same. If we need to add local state, or Redux, after the fact, we have a clear place for it.

More to your question, Redux containers at nested points is my preference. And the readability pro you have for the top level integration I don't think actually happens in practice, or at least I haven't seen that happen.

weltenwort commented 6 years ago

As long as we always also export the unconnected components (e.g. PanelMenu and PanelMenuContainer in your examples), redux-independent re-use is possible. It requires some additional assembly to implement the passing-down of props in a non-redux context, but there would still be little duplication of presentational components.

A pattern that might be worth considering in this context might be the "function-as-child" pattern. It differs from higher-order components in that it performs the composition at render-time instead of compile-time, which grants the consumer more control (see also the decorator disussions in the python community). A useful discussion of that pattern can be found here: https://medium.com/merrickchristensen/function-as-child-components-5f3920a9ace9. I'm not saying that it's necessarily a good idea in our situation, but something to consider.

As another data point, the redux FAQ indicate that using multiple containers throughout the tree is the officially recommended practice: https://github.com/reactjs/redux/blob/master/docs/faq/ReactRedux.md#should-i-only-connect-my-top-component-or-can-i-connect-multiple-components-in-my-tree

chrisronline commented 6 years ago

++ to @weltenwort

Redux recommends to separate out the redux part of the code versus the consuming UI layer. As long as that separation is maintained, re-use is easier than it would be without it. Redux provides the glue that might need to be replicated if re-use is occurring in a separate environment (without Redux) but if all that needs to be refactored/replicated is the glue and the rest of the parts work seamlessly, that's a pretty big win IMO.

bmcconaghy commented 6 years ago

I like the pattern of connecting whatever you need data for. I've done it the other way too (top component only connected) and it winds up turning into a mess of passed props.

w33ble commented 6 years ago

It occurs to me that I've given a talk on state earlier this year. Here's a couple images from my slides that seem relevant here...

When we talk about using Redux, what we're really talking about is shared state. Passing shared state down from parent components is how everyone was doing state back when React first became a thing. It works, but you end up passing props through components that never use them, and that can get out of control pretty easily. It also causes components to not really be reusable and makes it hard to track down where that state really comes from.

screenshot 2017-10-25 08 11 47

The whole reason to use any kind of global state management, at least as I understand it, is so you can tie shared state in only where you need it, instead of "polluting" the whole tree so that leaf nodes can access that state. The use of "containers" is simply a way to decouple the view layer from the use of shared state.

screenshot 2017-10-25 08 12 00

mattapperson commented 6 years ago

I have existed on both sides of this argument in the past, siding with thought patterns like @sqren and then again at other times on the anti-redux warpath. However at this point my stance is in line with @weltenwort (not to mention that this is the stance the redux maintainers currently take). So let me thus break down my main thoughts:

The core concept behind redux is to separate UI and logic When the majority of your components are connected, this might make it easier to build things out, but harder to on-board new devs and overall harder to maintain as there is no clear code path to what is being used where you have to just "know"/remember what components are using what data.

Redux is great, but there are at times performance costs 90% of the time, this does not matter, but for the 10% of time where this ends up playing a roll, having an arch that is so dependent on redux can make working around this a pain. In my opinion redux is a great tool, but we should allow the use of other options (again @weltenwort I think hit the nail on the head with "export both")

I am new here, and so maybe these things matter less here then at past companies I have worked at, but these are none-the-less my thoughts that I thought were worth sharing :)

markerikson commented 6 years ago

Hi, I'm a Redux maintainer. This issue hasn't been active in a while, but I just saw someone link to it and figured I'd comment.

It's up to you how many components you connect, but generally speaking, connecting more components is better for performance. Each connected component will be extracting smaller pieces of the state, meaning that more of the time that component's data is the same and it won't need to re-render.

As mentioned, the Redux FAQ entry on connecting multiple components suggests this is a good idea. You might also want to look at my post Practical Redux, Part 6: Connected Lists, Forms, and Performance, and the Redux Performance section of my React/Redux links list.

weltenwort commented 6 years ago

Hi @markerikson, thanks for chiming in with these links. I agree that it's a good idea to point out that the runtime complexity of mapStateToProps should be kept low to get those performance benefits. It's also a good place to apply memoization, e.g. with reselect.

And let me take this opportunity to say how much I appreciate the react and redux ecosystem link lists you maintain. They have been a huge asset while learning and arguing. Thank you!

stacey-gammon commented 6 years ago

Synced with @weltenwort and @spalger and we touched on this topic. A summary of the relevant discussion points is:

kobelb commented 6 years ago

Closing this as we aren't actively discussing any longer, thanks everyone for your input.