The Power of Simplifying Large Components in React
January 2nd, 2020
Share This Post
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:
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 (
<List dense={depth > 0} disablePadding>
{ids.map((id: string, itemIndex: number) => {
const key = `SidebarSection_${id}_item${itemIndex}`
const item = items[id]
switch (item.type) {
case 'divider':
return <Divider key={key} style={{ padding: 0, margin: 0 }} />
case 'label':
return (
<ListSubheader
key={key}
style={{
transform: expanded ? undefined : 'scale(0.55)',
textOverflow: 'ellipsis',
overflow: 'hidden',
userSelect: 'none',
}}
disableGutters={!expanded}
>
{item.label}
</ListSubheader>
)
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 (
<SidebarItem
key={key}
sectionId={sectionId}
depth={depth}
item={ctx.unserializeItem(item, { onHoverAction })}
isSelected={item.id === selectedId}
{...ctx}
/>
)
}
case 'static':
return (
<SidebarItem
key={key}
sectionId={sectionId}
depth={depth}
item={ctx.unserializeItem(item)}
isSelected={item.id === selectedId}
{...ctx}
/>
)
default:
return null
}
})}
</List>
)
}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:
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 (
<List dense={depth > 0} disablePadding>
{ids.map((id: string, itemIndex: number) => {
const key = `SidebarSection_${id}_item${itemIndex}`
const item = items[id]
switch (item.type) {
case 'divider':
return <Divider key={key} style={{ padding: 0, margin: 0 }} />
case 'label':
return (
<ListSubheader
key={key}
style={{
transform: expanded ? undefined : 'scale(0.55)',
textOverflow: 'ellipsis',
overflow: 'hidden',
userSelect: 'none',
}}
disableGutters={!expanded}
>
{item.label}
</ListSubheader>
)
case 'notebook': {
return (
<SidebarItem
key={key}
sectionId={sectionId}
depth={depth}
item={ctx.unserializeItem(item, {
onHoverAction: onHoverAction(
item,
ctx.createHoverControlsActionOnClick,
),
})}
isSelected={item.id === selectedId}
{...ctx}
/>
)
}
case 'static':
return (
<SidebarItem
key={key}
sectionId={sectionId}
depth={depth}
item={ctx.unserializeItem(item)}
isSelected={item.id === selectedId}
{...ctx}
/>
)
default:
return null
}
})}
</List>
)
}All we did was move the function to another place, and it already gives us huge benefits with hardly any extra effort:
- The reference to the function will stay the same.
- The
SidebarSectioncan now live a peaceful life as it no longer needs to worry about implementingonHoverActioncorrectly. All it needs to do is pass the arguments thatonHoverActionexpects. - We can now unit test
onHoverActionseparately 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. SidebarSectionbecomes 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:
case 'notebook':
return (
<SidebarItem
key={key}
sectionId={sectionId}
depth={depth}
item={ctx.unserializeItem(item, {
onHoverAction: onHoverAction(
action,
item,
ctx.createHoverControlsActionOnClick,
),
})}
isSelected={item.id === selectedId}
{...ctx}
/>
)
case 'static':
return (
<SidebarItem
key={key}
sectionId={sectionId}
depth={depth}
item={ctx.unserializeItem(item)}
isSelected={item.id === selectedId}
{...ctx}
/>
)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:
case 'notebook':
case 'static':
return (
<SidebarItem
key={key}
sectionId={sectionId}
depth={depth}
item={ctx.unserializeItem(item, item.type === 'notebook' ? {
onHoverAction: onHoverAction(
action,
item,
ctx.createHoverControlsActionOnClick,
),
} : undefined)}
isSelected={item.id === selectedId}
{...ctx}
/>
)
Simplying this provided a couple of important benefits:
- We eliminated duplicate code.
- It is now easier to read since we only need to look at one "copy" of the code.
- 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:
function SidebarSection({ id: sectionId, ids, items, depth, expanded }) {
const ctx = React.useContext(SidebarContext)
const selectedId = useSelector(selectSelected)
if (!ctx) return null
return (
<List dense={depth > 0} disablePadding>
{ids.map((id: string, itemIndex: number) => {
const key = `SidebarSection_${id}_item${itemIndex}`
const item = items[id]
switch (item.type) {
case 'divider':
return <Divider key={key} style={{ padding: 0, margin: 0 }} />
case 'label':
return (
<ListSubheader
key={key}
style={{
transform: expanded ? undefined : 'scale(0.55)',
textOverflow: 'ellipsis',
overflow: 'hidden',
userSelect: 'none',
}}
disableGutters={!expanded}
>
{item.label}
</ListSubheader>
)
case 'notebook':
case 'static':
return (
<SidebarItem
key={key}
sectionId={sectionId}
depth={depth}
item={ctx.unserializeItem(
item,
item.type === 'notebook'
? {
onHoverAction: onHoverAction(
action,
item,
ctx.createHoverControlsActionOnClick,
),
}
: undefined,
)}
isSelected={item.id === selectedId}
{...ctx}
/>
)
default:
return null
}
})}
</List>
)
}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:
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 (
<List dense={depth > 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 (
<Component
key={key}
{..getProps(
item,
expanded,
sectionId,
selectedId,
depth,
ctx
})}
/>
)
})}
</List>
)
}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:
SidebarSectionreduced its responsibilities.SidebarSectionbecame smaller.- We can clearly see what props are being injected to which component.
- We now don't have to pass in
key={key}four different times and instead just pass it like<Component key={key}
Downsides:
SidebarSectionbecomes smaller, but the file becomes larger.- One "entity" (everything was inside
SidebarSection) became three "entities" (now separated toSidebarSection,onHoverAction,getProps) - Stressing out our mouse more by scrolling top to bottom to get through the whole thing
So was it worth it?
In my honest opinion, if it takes too long to do the last part then it probably isn't worth it. The moral of the story is, it's definitely worth it to simplify code where it doesn't take much effort but still provides more multiple benefits in the outcome.
So in the case of our article, I support the first two simplification attempts in this post while I'm a little undecided on the third one.
However, we've now seen the power of simplifying large components in react.
Conclusion
And that concludes the end of this post! I hope you found this to be valuable and look out for more in the future!