The Power of Simplifying Large Components in React

Christopher T.

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:

  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:

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:

  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:

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:

  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 <Component key={key}

Downsides:

  1. SidebarSection becomes smaller, but the file becomes larger.
  2. One "entity" (everything was inside SidebarSection) became three "entities" (now separated to SidebarSection, onHoverAction, getProps)
  3. 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!


Tags


Subscribe to the Newsletter
Get continuous updates
© jsmanifest 2020