ag

React component code smells

🌱 This post is just budding and will be updated.

A collection of things I consider code smells in react components. All opinionated of course, so would love to hear if you (dis)agree or have other suggestions.

Too many props

Passing too many props into a single component may be a sign that the component should be split up.

How many is too many you may ask? Well, too many is very arbitrary so “it depends”. You might find yourself in a situation where a component should have 20 props or more, and still be satisfied that it only does one thing. But when you do stumble upon a component that have many props or you get the urge to add just one more to the already long list of props. There’s a couple of things to consider:

Is this component doing multiple things?

Like functions, components should do one thing well so it’s always good to check if the it’s possible to split the component into multiple smaller components. For example if the component is doing too much or if it has incompatible props.

Could I use composition?

A pattern that is very good but often overlooked is to compose components together instead of handling all logic inside just one. Let’s say we have a component that handles a user application to some organisation:

<ApplicationForm
  user={userData}
  organisation={organisationData}
  categories={categoriesData}
  locations={locationsData}
  onSubmit={handleSubmit}
  onCancel={handleCancel}
  ...
/>

Looking at the props of this component we can see that all of them are related to what the component does, but there’s still room to improve this by moving some of the components responsibility to it’s children instead:

<ApplicationForm onSubmit={handleSubmit} onCancel={handleCancel}>
  <ApplicationUserForm user={userData} />
  <ApplicationOrganisationForm organisation={organisationData} />
  <ApplicationCategoryForm categories={categoriesData} />
  <ApplicationLocationsForm locations={locationsData} />
</ApplicationForm>

Now we’ve made sure that the ApplicationForm only handles it’s most narrow responsibility, submitting and cancelling the form. The child components can handle everything related to their part of the bigger picture. This is also a great opportunity to use React Context for the communication between the children and their parent.

Am I passing down many ‘configuration’-props?

In some cases it’s a good idea to group together props into an options object, for example to make it easier to swap this configuration. If we have a component that displays some sort of grid or table:

<Grid
  data={gridData}
  pagination={false}
  autoSize={true}
  enableSort={true}
  sortOrder="desc"
  disableSelection={true}
  infiniteScroll={true}
  ...
/>

All of these props except data could be considered configuration. If we wanted to make all of these dynamic we would either have to manage a lot of lonely variables or put them in an object and then pass them down like pagination={options.pagination}. While this works I think it’s a bit cleaner to change the Grid so that it accepts an options prop instead.

const options = {
  pagination: false,
  autoSize: true,
  enableSort: true,
  sortOrder: 'desc',
  disableSelection: true,
  infiniteScroll: true,
  ...
}

<Grid
  data={gridData}
  options={options}
/>

This also means that it’s easier to exclude configuration options we don’t want to use when swapping between different options, we can just exclude pagination from some options and include in other.

Incompatible props

Avoid passing props that are incompatible with each other.

For example we might start by creating a common <Input /> component that is intended to just handle text, but after a while we also add the possibility to use it for phone numbers as well. The implementation could look something like this:

function Input({ value, isPhoneNumberInput, autoCapitalize }) {
  if (autoCapitalize) capitalize(value)

  return <input value={value} type={isPhoneNumberInput ? 'tel' : 'text'} />
}

The problem with this is that the props isPhoneNumberInput and autoCapitalize don’t make sense together. We can’t really capitalize phone numbers.

One solution to this is to break the component up into multiple smaller components. If we still have some logic we want to share between them, we can move it to a custom hook:

function TextInput({ value, autoCapitalize }) {
  if (autoCapitalize) capitalize(value)
  useSharedInputLogic()

  return <input value={value} type="text" />
}

function PhoneNumberInput({ value }) {
  useSharedInputLogic()

  return <input value={value} type="tel" />
}

While this example is a bit contrived, finding props that are incompatible with each other is usually a good indication that you should check if the component needs to be broken apart.

Shared state

ie duplicating the component and it shares state it shouldn’t Navbar never duplicated? Think again, animations! 💫

Fixes:

  • Remove the shared state, make sure you can use multiple instances of the component

Copying props into state

Don’t stop the data flow: For example copying props into state, meaning that when props change the state isn’t updated. (Constructor: state.x = prop.x, useState(prop))

When intentional, use standard naming like initialColor or defaultColor.

Reading: Writing resilient components by Dan Abramov.

Large “data” props

Passing data instead of composing (See Composable components → tabs)

(Not) prop drilling

Prop drilling is usually just fine but sometimes there’s better alternatives.

  • Use composition (pass down component as prop)
  • Use context api (when state is shared “horizontally”)

Returning JSX from functions

Splitting up a components JSX into multiple functions makes it hard to reason about the code, and is a clear example of a time where we probably want to create a new component instead.

Components doesn’t have to be moved to separate file, if tightly coupled?

Multiple booleans for state

Avoid using multiple booleans to represent a components state.

When writing a component and subsequently extending the components functionality it’s easy to end up in a situation where you have multiple booleans to indicate which state the component is in. For a small component that does a web request when you click a button you might have something like this:

function Component() {
  const [isLoading, setIsLoading] = useState(false)
  const [isFinished, setIsFinished] = useState(false)
  const [hasError, setHasError] = useState(false)

  const fetchSomething = () => {
    setIsLoading(true)

    fetch(url)
      .then(() => {
        setIsLoading(false)
        setIsFinished(true)
      })
      .catch(() => {
        setHasError(true)
      })
  }

  if (isLoading) return <Loader />
  if (hasError) return <Error />
  if (isFinished) return <Success />

  return <button onClick={fetchSomething} />
}

So when a user clicks the button fetchSomething is called, which in turn sets isLoading to true and then does a web request with fetch. When the request returns we either set isLoading to false and isFinished to true if the request was successful or set hasError to true if there was an error.

While this technically works fine it’s hard to reason about what state the component is in and it’s more error prone than alternatives. Another thing to point out before moving on is that it’s also possible we could end up in an “impossible state”, for example if we accidentally set both isLoading and isFinished to true at the same time.

The solution is to manage the state with an enum instead. In other languages enums are a way to define a variable that is only allowed to point at a predefined set of constant values, and while enums doesn’t technically exist in Javascript we can use a string instead:

function Component() {
  const [state, setState] = useState('idle')

  const fetchSomething = () => {
    setState('loading')

    fetch(url)
      .then(() => {
        setState('finished')
      })
      .catch(() => {
        setState('error')
      })
  }

  if (state === 'loading') return <Loader />
  if (state === 'error') return <Error />
  if (state === 'finished') return <Success />

  return <button onClick={fetchSomething} />
}

By doing it this way we’ve removed the possibility for impossible states and made it much easier to reason about this component. Finally, if you’re using some sort of type system like TypeScript it’s even better since you can specify the possible states:

const [state, setState] = useState<'idle' | 'loading' | 'error' | 'finished'>('idle')

Too many useState in a component

Many many useState

Fixes:

  • useReducer

Large useEffect

Avoid large useEffects that do multiple things. They make your code error prone and harder to reason about.

A mistake that I made a lot when hooks was released was putting to many things into a single useEffect. For example:

function Post({ id, unlisted }) {
  ...

  useEffect(() => {
    fetch(`/posts/${id}`).then(/* do something */)

    setVisibility(unlisted)
  }, [id, unlisted])

  ...
}

While this effect isn’t that large it still do multiple things. When the unlisted prop changes we will fetch the post even if id hasn’t changed.

To catch errors like this I try to describe the effects I write by saying “when [dependencies] change do this” to myself. Applying that to the effect above we get “when id or unlisted changes, fetch the post and update visibility”. If this sentence contains the words ”or” or ”and” it usually points to a problem.

Breaking this effect up into two effects instead:

function Post({ id, unlisted }) {
  ...

  useEffect(() => { // when id changes fetch the post
    fetch(`/posts/${id}`).then(/* do something */)
  }, [id])

  useEffect(() => { // when unlisted changes update visibility
    setVisibility(unlisted)
  }, [unlisted])

  ...
}