The Power of Simplifying Large Components in React
January 2nd, 2020
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:
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.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.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:
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:
'notebook'
can be clickable and 'static'
is not)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:
SidebarSection
reduced its responsibilities.SidebarSection
became smaller.key={key}
four different times and instead just pass it like <Component key={key}
Downsides:
SidebarSection
becomes smaller, but the file becomes larger.SidebarSection
) became three "entities" (now separated to SidebarSection
, onHoverAction
, getProps
)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.
And that concludes the end of this post! I hope you found this to be valuable and look out for more in the future!