Five Pitfalls of React Component Design
Designing React components and architecting component hierarchies can be difficult, for a few reasons. A common (and probably fair) criticism of React is that it provides a lot of different ways to do a lot of different things, and it can be hard to tease out the best way to approach any given problem.
Some questions you might ask are things like:
- Should I split my data loading logic into a separate component and pass down the data to a purely presentational component, or just use a hook and keep everything together in one component?
- If I have a lot of JSX, is it better to split it into a bunch of small components so it’s easier to parse in pieces, or should I keep everything in a bigger JSX tree so I can scan through to find what I’m looking for?
- Should I define things like single-use event handlers and helpers separately, and import them, or is it better to keep things close to my component by defining a bunch of inline functions?
Power and flexibility can be an advantage, but they can also provide more ways for things to go wrong, or to become (over)complicated. I genuinely don’t believe there’s a single right way to approach component design / architecture, so I’m not going to try to provide some sort of ironclad set of rules here that guarantee maintainable components. Instead, I’m going to ask and answer a couple of questions:
- What are the common reasons to go back to look at a component after initially building it?
- Given those reasons, how can I avoid making life more difficult for the person who looks at this component next?
Let’s start with the first question — what are the common reasons to go back to look at a component after initially building it?
So… I build my component, I write my unit tests, it does everything I want it to do and I’m happy, so I put it down and move on to the next thing.
An indeterminate amount of time passes, and then someone opens up that component file again. Why did they open it?
These reasons jump to mind:
- They want to find out what it does
- They want to know why it is doing something they don’t expect, or not doing something they do expect
- They need to make it do something new or something different
Apart from just clicking around randomly, I can’t think of anything significant, outside these three reasons. I’m sure there are probably some edge cases, but I’m willing to bet that 90%+ of the time, it’ll be for one of those three reasons. (One potential edge case is: they are specifically opening the component because they know it needs refactoring, and have come to do that).
If we take that as read, we move on to the next question: what can we do to make the next developer’s experience smoother? (Keep in mind that you might be the next developer, it might be six months down the line and you’ll have forgotten your own reasons for your approach).
With the common reasons to revisit a component fresh in our minds, let’s look at five common pitfalls. We’ll explore why they happen, why they cause problems, and how we can avoid them.
Pitfall 1: unclear or vague names for components
Naming things is hard. I’ve definitely felt like people must be giving me the side-eye in the past when two other engineers and I spent half an hour in a heated debate about what to name a particular component or function. It doesn’t seem THAT important. At least, not until you’re looking at a big component tree wondering exactly what DataLoader loads. Data, sure, but what data, and why? Or maybe you're looking at ConfigWrapper wondering what it's wrapping... or maybe being wrapped with? What content is rendered by the ContentComponent?
You should be able to get a fairly concrete idea of what a component is doing from reading the name. When I’m inspecting a tree of React components, I want to be able to zero in on whichever component is relevant to the functionality I currently care about. Vague naming can be a real obstacle to that.
Let’s look at an example. We have a sidebar that looks like this:
Rendered by this tree of components:
<DataSourceConnectorServiceLoader />
<DataSourceConnectorConnector />
<DataSourceConnectorComponent />
<DataSourcePopoverComponent />
<SpreadsheetsConnectorConnector />
<SpreadsheetsConnectorMounter />
<SpreadsheetsConnectorComponent />Can you tell me what the ConnectorMounter does, and what the ConnectorConnector does? If you were looking at a prop called services in the SpreadsheetsConnectorComponent, where is that likely to be being passed down from? (If you said the ConnectorServiceLoader, you're unfortunately incorrect).
The point here is that I can’t really discern the individual responsibilities of these components by looking at their names. The only information I can glean from this tree is:
- It’s something to do with spreadsheets, and connecting spreadsheets
- There’s something at the top level that loads a service
- There’s some sort of “popover”
Putting aside arguments on whether this could be refactored to use fewer components, or architected differently: What would a better version of this tree look like?
Let’s do some investigation. I’ll show my working here at a high level, as I don’t think it’s going to be super useful to dive into each component in detail, but it might be helpful to see the broad strokes.
Starting at the top level and looking into ConnectorServiceLoader a little closer, I can see it's actually not loading the “service”, it's just loading the name of the service, which is required the next level down. So, simple change, let's call it <DataSourceConnectorServiceNameLoader />.
Down one step, the DataSourceConnectorConnector, all this does is grab some things from Redux state and set up a couple of action callbacks. So, another simple change: DataSourceConnectorRedux.
Next, DataSourceConnectorComponent, Component is redundant here (aren’t these all “components”?), so let's remove that. Its primary responsibility is to choose the appropriate component to render based on the particular service we're dealing with, so in this case, Spreadsheets. I'm thinking DataSourceConnectorSpecifier. It specifies which DataSourceConnector we want to render.
The DataSourcePopoverComponent is fine, once we remove Component. It’s rendering a sidebar that “pops” over the main content of the page, so the name feels clear enough.
Like the higher level one, SpreadsheetsConnectorConnector can be SpreadsheetsConnectorRedux as it deals with similar things.
Then, we have SpreadsheetsConnectorMounter. Its responsibility is to load the details of all the relevant services (Google Sheets, Dropbox, OneDrive etc.), and also all of the user's "service accounts" (individual connections to those services). So I'm going to go for SpreadsheetsConnectorServiceAccountFetcher.
Finally, the bottom layer, which I think can just simply be SpreadsheetsConnector.
The revised tree looks like this:
<DataSourceConnectorServiceNameLoader>
<DataSourceConnectorRedux>
<DataSourceConnectorSpecifier>
<DataSourcePopover>
<SpreadsheetsConnectorRedux
<SpreadsheetsConnectorServiceAccountFetcher>
<SpreadsheetsConnector>I’m not going to pretend this is an ideal component hierarchy, but I’d say it’s a definite improvement. If I wanted to answer questions like:
- What Spreadsheets-specific Redux actions can be dispatched from the
SpreadsheetsConnector? - Where is the
serviceAccountsprop passed from? - Why does this component render in a slide-out sidebar?
- Where is it determined that this component should be Spreadsheets-specific?
I have a good chance of being able to get on the right track with just a glance at the component tree. Previously, this is unlikely to have been the case.
Pitfall 2: lots of small, shallow components
Lots of engineers internalise the idea that it’s important to break things into smaller, contained pieces of functionality. Generally, this way of thinking leads to lots and lots of small functions, or, in the React world, lots of small components. However, breaking functionality unnecessarily into lots of smaller components can make it harder to find where particular things are actually being rendered, or where a behaviour is coming from.
The important thing is for components to be deep relative to the API that they expose. The ideal situation is a component that provides a simple interface, but a lot of power / capability under the hood. As John Ousterhout explains in A Philosophy of Software Design, each “interface” you add to a codebase increases the level of complexity, because any developer working there will need to know about that interface and how it works in order to use it. In order to justify this increased complexity, the new interface / component must provide some tangible and significant amount of benefit.
A particular case of “small component explosion” results from having a rigid separation between logic and presentation.
Here’s a real world example:
<GraphQLEditPageWithDashboardsQuery>
<GraphQLEditPageConnector>
<GraphQLEditPage>
<EditContainerDashboardPageConnector>
<EditContainerDashboardPageComponent
<QueriedContainerLayout>
<ContainerLayoutConnector>
<ContainerLayout>
<DashboardConnector>
<Dashboard>There are really just four components in here:
GraphQLEditPage, EditContainerDashboardPage, ContainerLayout and Dashboard. A tree with just those four components wouldn't be too hard to parse, but if you try to "nicely" separate all the logic into separate concerns, it's easy to end up with this tree of ten components instead. Also, as we discussed in the previous section, the general / vague naming means it can be quite hard to zoom in on the specific piece of functionality you might need to track down, because there aren't really any good threads to follow based on the component names.
Taking a step back: I’d personally always opt for bigger components that group related functionality together. These chains often rely on props, and there’s really not an easy way to follow a prop “up” the component chain other than figuring out all the places that render the component, and what value they pass to it for that particular prop. When you’re dealing with chains of many components, this can quickly become a real nightmare for a developer’s cognitive load.
Circling back around to our original question we were trying to answer here: “What is this / what is its purpose?”, it’s a lot easier to answer this by scrolling through a single component, than by scrolling through five small related ones and trying to keep the connections between them in your mind as you do so.
Pitfall 3: too many props / complicated API
Diving into an example immediately here is probably going to best illustrate the point, so let’s do it.
Here’s a component called ManageScreens, which, you guessed it, handles the management of TV screens. Let's take a look at the props it can take:
const ManageScreens = ({
isModalOpen,
currentView,
isConnecting,
devices,
error,
onClose,
onConnect,
onShowAddView,
onShowManageView,
onUpgradeLinkClick,
dashboards,
sharingLoops,
isFetchingDevices,
onDeleteDevice,
onRefreshDevice,
onSetDeviceName,
onSetDeviceConnection,
onCreateLoop,
onToggleLite,
isLiteEnabled,
groupId,
}) => {That’s a whopping 21 props.
But so what? It does a lot, why is that a bad thing?
So, firstly, the more complicated an interface, the more complexity added to the system, as we discussed previously. It means a more complicated API for interacting with the component, which adds cognitive load for the developer trying to understand or modify it, because they need to learn what each prop does and how it may be relevant to what they want to achieve.
Secondly, if you do embark on the arduous challenge of learning what each of the 21 props in this component are used for, it might actually not be particularly straightforward. As we discussed previously, for each prop, you need to follow it “down” into the component to figure out what effect it has (which can be challenging if it’s not a well named prop), and then “up” the component chain in order to find out what determines what the value of the prop is, or, with callbacks, what action it performs / effect it has.
A lot of the complication in this case seems to be that we’re “injecting” all of the potential actions via callbacks, and then passing them around as props. If we moved the handling of these events to within the component itself (or a separate hook), we’d immediately drop 11 of those props. Then, potentially some things could be loaded via hooks rather than passed in as props (ie. dashboards, sharingLoops, devices).
Realistically, the only things that really strike me here as “prop-ish” are: currentView, isLiteEnabled and potentially groupId. Three props would definitely make a slightly less intimidating API compared to twenty-one!
Pitfall 4: unclear booleans defined separately to conditional checks
A common pattern is to wrap up some more complicated logic during the “setup” stage of your component, so that in the JSX, you can do something neater.
Take this example (bold for emphasis):
{showManageLink && (
<Column flexAuto align="flex-start" justify="center">
<Button
intent={Button.INTENT.LINK}
icon="cog"
href="/admin/loops">
<FormattedMessage id="loops.manageLoops" />
</Button>
</Column>
)}The issue here is that the conditional is not really communicating any useful information, and you’ve actually hidden information that might be useful.
What you’ve written in, translated into plain English is: “show the manage link, if you should show the manage link”. Not massively helpful.
So to find out what determines whether the showManageLink variable is true, I need to scroll up to the top of the component, and there I see it's based on the current URL: if it's not /admin/loops, it is set to true. Essentially: it’s visible unless we’re on a specific page, in which case it’s hidden.
I really didn’t save much clutter here with my boolean, in fact, I would have saved time if I could just immediately see that was the case:
{currentURLPath !== '/admin/loops' && ( <My JSX> )}`This isn’t a hard and fast rule, however. Sometimes it is worth wrapping up information in this way.
In another component, we have a list of users, and display a number of actions based on the type of user they are (ie. group admin, account owner, view only). And the way we determine the type of user is by checking a number of conditions:
const isGroupAdmin = user => {
if (isUndefined(user)) {
return false;
}
if (!isUndefined(user.group_memberships)) {
return !!find(user.group_memberships, ({ role }) => role === 'admin');
}
if (!isUndefined(user.is_admin)) {
return user.permission === 'power_user';
}
return user.permission === 'admin';
};And then when rendering a relevant action, we do:
{(isGroupAdmin(member) || isGroupViewer(member)) && (
<AccountOwnerOnly>
<DropItem onClick={this.toggleMakeOwnerModal} isSmall>
<FormattedMessage id="people.makeOwner" />
</DropItem>
</AccountOwnerOnly>
)}The relevant question is still answered here: what determines whether this gets rendered?
Answer: If the user ( / member) is a group admin (or a group viewer).
We aren’t saying “it gets rendered if it should be rendered”, which has zero information content. If we want to know what determines if a user is a group admin, we have to go somewhere else, but we know exactly where we need to go because we’re calling the function right there!
There’s another example where actually using the conditions directly isn’t that helpful, and it might be clearer to extract it out. If we had something like externalUsers && externalUsers.length > 0 && dashboard as a conditional for a render, it might be more sensible to put that in a variable like areDashboardDependenciesLoaded or something like that.
{areDashboardDependenciesLoaded && ( <Dashboard /> )}// vs.{externalUsers && externalUsers.length > 0 && dashboard && ( <Dashboard /> )}
A good heuristic to use here is: if I can read it out loud and it’s clear what’s happening and why, then it’s fine.
We’re trying to maximise for readability and information content. The ideal is: it’s extremely readable and it conveys a lot of information about why something is the way it is.
Pitfall 5: Actions encapsulated in vaguely named “handler” functions
This is a similar case to the previous one, but with actions instead of booleans. A common case is defining something like onClick or handleClick at the top of your component, and then passing it in somewhere down in the JSX.
Again, there are many cases where variations of this may make sense. Click actions can do a lot of things, and trying to write them directly into JSX can result in some serious clutter.
For part of our app that handles interacting with Spreadsheets, we have something like:
<Input name={this.props.path} onChange={this.handleChange} />But if we moved the onChange here inline, we'd end up with something like:
<Input name={this.props.path} onChange={({ value }) => {
const shouldUpdate = /^[a-zA-Z]/.test(value);
this.setState({ value });
if (shouldUpdate) {
const selectionValue = inputToSelection(value);
if (!isUndefined(selectionValue)) {
const [start, end] = selectionValue;
const selection = [{
firstCell: start,
start,
end: end || start,
}];
this.props.onUpdateSelections(selection);
}
} else {
const { path, activeFieldPath, onSetValue } = this.props;
onSetValue(path, value, activeFieldPath);
}
}} />Which is… a lot. I’ve actually simplified this somewhat too, if there were also onClick, onBlur, onFocus etc. all defined inline, I think we'd start to find this difficult to read and understand.
The problem, as before, is that handleChange gives us no information about what is happening. Essentially we're saying onChange={doTheChangeThing}.
If we’re exploring this component, it’d be nice to be able to look at this <Input /> and get some idea of what happens when the value of it changes.
In this case, I think we could restructure to make things a little clearer and encapsulate some of the more important parts, something like:
<Input
name={this.props.path}
onChange={({ value }) => {
const isAlphabetical = /^[a-zA-Z]/.test(value);
this.setState({ value });
isAlphabetical ? updateSelection(value) : updateConfig(value)
}} />Here, what’s happening is defined inline, but it’s not quite so overwhelming. We can get a lot of information by looking at the component. We’re checking if the value is alphabetical, and then either updating a selection or the config based on that. The details are encapsulated in other functions, but we get an idea of what those functions are for, with the ability to dig further into that if we want.
If we are going to wrap this up in a single function, again, we should at least try to provide some idea of what is actually happening. handleChange or handleClick gives us absolutely no information on what to expect when we go look at what those functions do.
In this post, I’ve described the five most common pitfalls I’ve encountered when designing and architecting React components and component hierarchies. I’m sure there are other problems that I haven’t covered here, but I hope this has provided a bit of a useful exploration of some issues you may have encountered yourself.
As ever, a lot of this advice boils down to a single principle: develop for the next person who is going to look at your code.
How to do that is the more complicated part, but I hope this has given you a few ideas.
— Liggi
