Having large components _isn't always a bad thing_, but it's a good practice to take advantage of opportunities where you can simplify components further _especially when it provides additional benefits_. When you have a large component, it _can_ become disadvantageous because the larger a component gets the more difficult it becomes to maintain and read over time. Let's look at this component below and see reasons why it'd be better off simplifying it. (This is code from a production app, so this is actually a _real world_ example) The component `SidebarSection` below takes some props where `props.ids` is an array of item ids as strings and `props.items` is an object that maps sidebar items using each item's `id` as the key. It uses these props to render sidebar items: ```jsx import React from 'react' import { useSelector, useDispatch } from 'react-redux' import List from '@material-ui/core/List' import Divider from '@material-ui/core/Divider' import ListSubheader from '@material-ui/core/ListSubheader' import { EDIT_NOTEBOOK, DELETE_NOTEBOOK } from 'features/toplevel' import { selectSelected } from 'features/sidebar' import SidebarContext from './SidebarContext' import SidebarItem from './SidebarItem' function SidebarSection({ id: sectionId, ids, items, depth, expanded }) { const ctx = React.useContext(SidebarContext) const selectedId = useSelector(selectSelected) if (!ctx) return null return ( 0} disablePadding> {ids.map((id: string, itemIndex: number) => { const key = `SidebarSection_${id}_item${itemIndex}` const item = items[id] switch (item.type) { case 'divider': return case 'label': return ( {item.label} ) case 'notebook': { // Called inside unserializeHoverControlsProps when iterating over each hover action const onHoverAction = (action: any) => { if (action.Icon) { const notebook = item.data if (notebook) { action.onClick = ctx.createHoverControlsActionOnClick({ context: action.name === 'edit' ? EDIT_NOTEBOOK : action.name === 'delete' ? DELETE_NOTEBOOK : '', data: action.name === 'edit' ? item : action.name === 'delete' ? { id: notebook.id, title: notebook.info.title, description: notebook.info.description, isEncrypt: notebook.isEncrypt, created_at: notebook.created_at, modified_at: notebook.modified_at, } : null, }) } } } return ( ) } case 'static': return ( ) default: return null } })} ) } ``` The component actually doesn't look _that_ bad, but if you think about it whenever we edit the component we'd have to understand _every_ line of code before we introduce changes because we don't know if changing something can break other parts of the component or not. An example is the `onHoverAction` function that is created in the switch case. it's unnecessarily bloating our component, and depending on the implementation of `SidebarItem` it has potential to cause an _infinite loop_ because the reference to it is being re-created every time the component re-renders. It's also making this whole component a little more sensitive to unit tests because we're delegating the `SidebarSection` component to be responsible for the implementation details of `onHoverAction`. In our unit tests, we have to be aware of the implementation details of `onHoverAction` when we're testing that the `SidebarSection` component behaves correctly which doesn't make much sense (this means watching out for things like syntax errors, since a typo inside the function can break the rendering of `SidebarSection` and we'd blame the component for doing a bad job) We can simplify this by simply extracting it outside so we no longer have to put the blame on the component: ```jsx function onHoverAction(item, createOnClick) { return (action) => { if (action.Icon) { const notebook = item.data if (notebook) { action.onClick = ctx.createHoverControlsActionOnClick({ context: action.name === 'edit' ? EDIT_NOTEBOOK : action.name === 'delete' ? DELETE_NOTEBOOK : '', data: action.name === 'edit' ? item : action.name === 'delete' ? { id: notebook.id, title: notebook.info.title, description: notebook.info.description, isEncrypt: notebook.isEncrypt, created_at: notebook.created_at, modified_at: notebook.modified_at, } : null, }) } } } } function SidebarSection({ id: sectionId, ids, items, depth, expanded }) { const ctx = React.useContext(SidebarContext) const selectedId = useSelector(selectSelected) if (!ctx) return null return ( 0} disablePadding> {ids.map((id: string, itemIndex: number) => { const key = `SidebarSection_${id}_item${itemIndex}` const item = items[id] switch (item.type) { case 'divider': return case 'label': return ( {item.label} ) case 'notebook': { return ( ) } case 'static': return ( ) default: return null } })} ) } ``` All we did was move the function to another place, and it already gives us huge benefits with hardly any extra effort: 1. The reference to the function will stay the same. 2. The `SidebarSection` can now live a peaceful life as it no longer needs to worry about implementing `onHoverAction` correctly. All it needs to do is pass the arguments that `onHoverAction` expects. 3. We can now unit test `onHoverAction` separately because it's available as an export. Want to see if this is working expectedly? Simply import it, pass in the three parameters and see what it returns. 4. `SidebarSection` becomes easier to read and maintain. That's actually not all we can do to simplify it. We have another opportunity to simplify the component even further. There's duplicated code in these two switch blocks: ```jsx case 'notebook': return ( ) case 'static': return ( ) ``` It actually may not become much of an issue to just leave it the way it is. However, I'm sure that any developer who reads this code will be obligated to read each prop line by line just to be 100% sure themselves that they aren't all that different. After all, ideally we'd want to believe that there are important reasons for similar looking code get separated, so why in the world were these separated? In our case there wasn't really much of a good reason, so it's a good idea to simplify this so that future developers won't get caught in this awkward scenario when they're trying to debug this component. We can simplify this by simply doing this: ```jsx case 'notebook': case 'static': return ( ) ``` Simplying this provided a couple of important benefits: 1. We eliminated duplicate code. 2. It is now easier to read since we only need to look at one "copy" of the code. 3. Self documenting code (it basically tells us that items with type _"notebook"_ and _"static"_ are almost exactly the same and there's not much need to worry about their differences besides that items with type `'notebook'` can be clickable and `'static'` is not) ## When simplifying backfires with overthinking Now there is something else that we can possibly "simplify". Even though our switch cases became a little shorter, it looks a little ugly to look at. This is how our `SidebarSection` component looks like now with the simplification changes applied: ```jsx function SidebarSection({ id: sectionId, ids, items, depth, expanded }) { const ctx = React.useContext(SidebarContext) const selectedId = useSelector(selectSelected) if (!ctx) return null return ( 0} disablePadding> {ids.map((id: string, itemIndex: number) => { const key = `SidebarSection_${id}_item${itemIndex}` const item = items[id] switch (item.type) { case 'divider': return case 'label': return ( {item.label} ) case 'notebook': case 'static': return ( ) default: return null } })} ) } ``` One issue we might come up with here is that we are giving the render block of each item a little too much responsibility, making it responsible for passing the right props to the right components. Thinking that way it might be better to rewrite it this way instead: ```jsx function getProps({ item, expanded, sectionId, selectedId, depth, ctx }) { switch (item.type) { case 'divider': return { style: { padding: 0, margin: 0 } } case 'label': return { style: { transform: expanded ? undefined : 'scale(0.55)', textOverflow: 'ellipsis', overflow: 'hidden', userSelect: 'none', }, disableGutters: !expanded, } case 'notebook': case 'static': return { sectionId, depth, item: ctx.unserializeItem( item, item.type === 'notebook' ? { onHoverAction: onHoverAction( item, ctx.createHoverControlsActionOnClick, ), } : undefined, ), isSelected: item.id === selectedId, ...ctx, } default: return undefined } } function SidebarSection({ id: sectionId, ids, items, depth, expanded }) { const ctx = React.useContext(SidebarContext) const selectedId = useSelector(selectSelected) if (!ctx) return null return ( 0} disablePadding> {ids.map((id: string, itemIndex: number) => { const key = `SidebarSection_${id}_item${itemIndex}` const item = items[id] let Component if (item.type === 'divider') { Component = Divider } else if (item.type === 'label') { Component = ListSubheader } else if (['notebook', 'static'].includes(item.type)) { Component = SidebarItem } else { return null } return ( ) })} ) } ``` Now we further simplified `SidebarSection` to only be responsibility for calling `getProps` to provide the associated props and assigning the correct `Component` based on `item.type`. We can now unit test `getProps` to make sure that they are returning the correct props according to `item.type`. Was this a good attempt to simplify our react code? Let's see the benefits gained vs the downsides introduced: Benefits: 1. `SidebarSection` reduced its responsibilities. 2. `SidebarSection` became smaller. 3. We can clearly see what props are being injected to which component. 4. We now don't have to pass in `key={key}` four different times and instead just pass it like `

Posts tagged “simplification”

The Power of Simplifying Large Components in React