Settings UI - Search: don't connect sub components to global state tree

[eliorivero]

The sub components that compose Writing tab got connected in #5997
This is unnecessary, adds a slightly overhead to performance and it's harder to maintain. We should keep only the parent components connected and pass the props to the children as we have been doing so far.

[zinigor]

Thanks for creating an issue for that!
Have you conducted any testing that would show that performance increase? I have looked into this a bit and some studies show that it's better to connect lower level components to the state than to have higher lever components thread properties down. For example this article states that:

Components consuming exclusive state did not need a container component at the top threading props to it; they can subscribe to the state and become a container component. This dramatically reduces the amount of work React has to do against Redux actions.

Plus the selectors that we use are only recomputed when state changes for them. If we keep the main component listening to all changes, it will have to redraw every time, won't it?

Let's discuss this more, @oskosk @dereksmart @tyxla @dmsnell perhaps any of you have an idea what direction it's better to take here?

[eliorivero]

If we keep the main component listening to all changes, it will have to redraw every time, won't it?

This is actually a good example. The one change that you introduced, search, is the one that will necessarily redraw the components. Of course they will have to be rendered during search. The difference is that if that what was searched and which modules matches is resolved in the parent component, it can quickly even dismiss rendering completely a component. With your approach, all components must start their life, connect and only then, they will find out if they're meant to be rendered or not.

All other previous changes didn't need any redraw since the form settings were handling their own connections to global state tree. These middle components were doing nothing more than presentational stuff. They were only rendered once when the tab was displayed. All other was managed either in SettingsGroup or in the form components.

It's also how Calypso is done. For example:
Parent - connected
https://github.com/Automattic/wp-calypso/blob/b73f49af4f72f859f004d16d6274f76c6d1168da/client/my-sites/site-settings/main.jsx
General tab - not connected
https://github.com/Automattic/wp-calypso/blob/b73f49af4f72f859f004d16d6274f76c6d1168da/client/my-sites/site-settings/form-general.jsx
And the form components handle their connections.

It's also harder to maintain given our approach of splitting things into smaller files. Actually, you already did this for Discussion, Security and Traffic tabs. And it shows that it's more maintainable to do it in that way than how it's done in Writing tab.

In addition, and this should go to a separate issue, we need to do some re-thinking of the structure of this search thing. Right now the search renders the tabs (like writing/index.jsx) which in turn may or may not render the settings card (like /writing/composing.jsx). I don't think any of these should be aware that they're being searched. We should be able to render the setting cards, not the tab where they are and they shouldn't be aware of search at any time.
This can be also seen in the number of conflicts when merging that Writing subcomponents caused due to its coupling. If they weren't aware of search they wouldn't have any code related to search and thus wouldn't have caused any conflict.

[dmsnell]

Since I'm not familiar with the code changes I'll offer some general advice over this topic which has been significant in every project of size I've been a part of.

To start off I'll introduce what I consider to be the ideal arrangement of data passing:

• Each React component lives in its own module (file)
• Each React component is a named export wherein all properties are passed in via props
• The default export of these modules is a wrapped version of the base component with state data injected via connect() or similar methods.
• Props not extracted from state are passed no more than a single layer from the parent

// ImageViewer.js
export const ImageViewer = props => …

const mapStateToProps = ( state, ownProps ) => ( {
	imageUrl: getImageUrl( state, ownProps.imageName ),
} );

const mapDispatchToProps = ( {
	selectImage,
} );

export default connect( mapStateToProps, mapDispatchToProps )( ImageSearch );

// thing.js
<ImageViewer imageName={ searchText } />

In this scenario we end up with a completely testable component via the unwrapped named export plus we have default "practical case" props injected from state for real-world use. connect() automatically makes a component pure unless this behavior is specifically overwritten. this ends up being very fast because Redux state changes result in data that is different via shallow comparison.

My number one rule for passing props down is never pass down props though a component - only pass from a single parent to a single child and also never pass the thing itself but rather pass a reference to the thing (okay, that was two rules…).

If we pass through a component, meaning that we see something like this…

<SomeChild { ...this.props } />

// or

<SomeChild { ...omit( this.props, [ 'cat', 'dog', 'fish' ] ) } />

…then we have completely obscured the data being passed and the reason it's there. It becomes a maintenance nightmare and this only grows as we get multiple levels deep.

With the theme of keeping the UI state in the most minimal form needs to represent the application visually I prefer to pass only references or primitive data types where possible. I would not want to pass a site object down because that ends up strongly coupling the structure of site to the visual component. Instead, by passing down the siteId or domain or some other reference we allow for the components to describe which object they need and for the state subsystem to provide it, allowing us to easily swap out the provider mechanisms.

Would be happy to discuss more about this and why I have settled on the approach given above.

[eliorivero]

In addition to what I posted above, there's also the inconsistency that only the setting components in Writing tab were connected, for example:
https://github.com/Automattic/jetpack/blob/feature/settings-overhaul/_inc/client/writing/composing.jsx
However, those in the Discussion, Verification and Security tabs were not, for example:
https://github.com/Automattic/jetpack/blob/feature/settings-overhaul/_inc/client/traffic/verification-services.jsx
and instead the connection was placed in the parent
https://github.com/Automattic/jetpack/blob/feature/settings-overhaul/_inc/client/traffic/index.jsx

[zinigor]

OK, thanks both of you for a detailed explanation!
@eliorivero you said:

We should keep only the parent components connected and pass the props to the children as we have been doing so far.

That would have us pass the search term property and the selector method down several components, and that counters what @dmsnell described as a best practice. Would you be happy if we had a minimal number of components connected, but would still connect the lowermost lever because we need to have the selector available in them?
Like here
https://github.com/Automattic/jetpack/blob/feature/settings-overhaul/_inc/client/traffic/verification-services.jsx
It would be great if we could determine exactly what portion of the form to show.

So in two words: we can cut off rendering of tabs entirely at SearchableSettings level if they are not found, and connect only lowermost components.

[eliorivero]

That would have us pass the search term property and the selector method down several components,

No, it's only one layer, from parent to child, from writing/index.jsx to writing/composing.jsx for example. They're already passing props. Other components inside writing/composing.jsx are connected on their own, for example, SettingsGroup.

would be great if we could determine exactly what portion of the form to show.

Perhaps instead of connecting stuff like writing/composing.jsx the connection should be resolved in SettingsCard, which you are already properly connecting in the banners PR. The things that search finds and displays are setting cards, independently of their tabs, so maybe these cards should be the one to be aware of search. That way we wouldn't be tied to a model of displaying tabs and choose what to display like what it's currently done in _inc/client/settings/index.jsx.

[zinigor]

Let's leave this open for now because there's a PR attached to this issue. I'll take a look at it and we'll see if it needs to go in, or closed for good.