"There's more than one way to skin a cat." - this saying also applies to React projects. The same user interface can be built using different approaches to code. However, some of these approaches are worse than the others. Here are 8 bad practices I often encounter in React projects.
Warning: these code snippets are here only to demonstrate common React bad and good practices. They are as simple as possible. Even though they may appear like a correct solution to a problem, they often require some extra modifications to make them actually useful and correct.
1. Letting components grow out of control
This is by far the most common issue. It's often tempting to add "just one more thing" to a component. However, after we do that twenty times, code of the component becomes very long. It's more difficult to understand and debug, more time is needed to maintain it and it's easier to introduce bugs. The right approach is to create a number of small components and use composition to build complex components. This solution also helps to prevent code duplication - small components are easy to reuse.
2. Overusing useState
Let's take a look at the following code:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21export function Example() {
const [username, setUsername] = useState("");
const [message, setMessage] = useState("Hello");
const handleUsernameChange = (event: React.ChangeEvent<HTMLInputElement>) => {
const newUsername = event.target.value;
setUsername(newUsername);
setMessage(`Hello ${newUsername}`);
};
return (
<div>
<div>{message}</div>
<div>
<input
type="text"
value={username}
onChange={handleUsernameChange}
/>
</div>
</div>
);
}The code works as expected, but there is an issue: message should not be created with useState. It's just a string derived from username state property - it's not a state property on it's own. Here's how this example should look like:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20export function Example() {
const [username, setUsername] = useState("");
const message = `Hello ${username}`;
const handleUsernameChange = (event: React.ChangeEvent<HTMLInputElement>) => {
const newUsername = event.target.value;
setUsername(newUsername);
};
return (
<div>
<div>{message}</div>
<div>
<input
type="text"
value={username}
onChange={handleUsernameChange}
/>
</div>
</div>
);
}3. Overusing useCallback
Here is another code snippet:
1
2
3
4
5
6
7
8
9
10
11export function Example() {
const validateEmail = useCallback((text: string) => {
return text.includes("@");
}, []);
return (
<div>
<label>Email:</label>
<TextInput validateValue={validateEmail} />
</div>
);
}What's wrong with that code? Yes, email validation is oversimplified. But this isn't the issue we are looking for in this example. The problem is that useCallback is unnecessary. Intentions were good - we didn't want to pass a value that changes on every render to our child component. However, validateEmail doesn't depend on our components! That's why we should move it outside:
1
2
3
4
5
6
7
8
9
10
11
12
13// A .ts file:
export function validateEmail(text: string) {
return text.includes("@");
}
// A .tsx file:
export function Example() {
return (
<div>
<label>Email:</label>
<TextInput validateValue={validateEmail} />
</div>
);
}We simplified our component and gained a function that can be used in other parts of our app.
4. Overusing useEffect
... and another "overusing a React hook" problem. React's documentation explicitly states that "useEffect is a React Hook that lets you synchronize a component with an external system.". Despite that, I often see code like this:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27export function Example() {
const [searchText, setSearchText] = useState("");
const [searchResults, setSearchResults] = useState<SearchResults>([]);
const handleSearchTextChange = useCallback((newSearchText: string) => {
setSearchText(newSearchText);
}, []);
useEffect(() => {
if (searchText.trim().length > 0) {
api.search(searchText).then(results => {
setSearchResults(results);
});
}
}, [searchText]);
return (
<div>
<TextInput
value={searchText}
onChange={handleSearchTextChange}
/>
<div>
{searchResults.map(result => (
<div key={result.id}>{result.title}</div>
))}
</div>
</div>
);
}Searching should not be performed via useEffect due to searchText change. It should be initiated as a reaction to change event. This is much better:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25export function Example() {
const [searchText, setSearchText] = useState("");
const [searchResults, setSearchResults] = useState<SearchResults>([]);
const handleSearchTextChange = useCallback((newSearchText: string) => {
setSearchText(newSearchText);
if (newSearchText.trim().length > 0) {
api.search(newSearchText).then(results => {
setSearchResults(results);
});
}
}, []);
return (
<div>
<TextInput
value={searchText}
onChange={handleSearchTextChange}
/>
<div>
{searchResults.map(result => (
<div key={result.id}>{result.title}</div>
))}
</div>
</div>
);
}Of course the above code is not a production-ready solution. We don't have debouncing, handling of the order of api promise resolution, error handling...
5. Complex functions passed to useEffect
useEffect has a very useful feature - function returned by useEffect's callback will be called when cleanup is needed. We can use it to remove event handlers, stop intervals etc. Here is an example... with a bug:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25interface Item {
updateInterval: number;
}
interface ExampleProps {
items: Item[];
activeItemId: number;
}
export function Example(props: ExampleProps) {
useEffect(() => {
const handler1 = () => console.log("handler1()");
const handler2 = () => console.log("handler2()");
const intervalId1 = setInterval(handler1, 5000);
if (!props.items[props.activeItemId]) {
return;
}
const intervalId2 = setInterval(handler2, props.items[props.activeItemId].updateInterval);
return () => {
clearInterval(intervalId1);
clearInterval(intervalId2);
};
}, [props.items, props.activeItemId]);
return (
<div>Lorem ipsum</div>
);
}What's the problem? There's a simple return in line 14, but we have already started interval1, so we have to change lines 13-15 to:
1
2
3
4
5if (!props.items[props.activeItemId]) {
return () => {
clearInterval(intervalId1);
};
}What if we need a third interval? Or an event handler? Or something else? We have to thoroughly review the code to ensure everything is always cleaned up properly. This is clearly a bad approach. It's much better to keep functions passed to useEffect simple:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17useEffect(() => {
const handler = () => console.log("handler1()");
const intervalId = setInterval(handler, 5000);
return () => {
clearInterval(intervalId);
};
}, [props.items, props.activeItemId]);
useEffect(() => {
const handler = () => console.log("handler2()");
if (!props.items[props.activeItemId]) {
return;
}
const intervalId = setInterval(handler, props.items[props.activeItemId].updateInterval);
return () => {
clearInterval(intervalId);
};
}, [props.items, props.activeItemId]);6. Not using custom hooks
We can create custom hooks by creating functions with names starting with use. We can use other hooks inside these functions. This is a great way to reduce code duplication and simplify code of components by making it shorter. Reusability, simplification, readability, ... these are great reasons to use custom hooks. So why do so many devs avoid them?
7. Putting complicated code inside event handlers
Here's an example of a handler executed when user submits a login form:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37async () => {
const loginData = {
username: username.trim(),
password: password.trim(),
};
if (loginData.username.length === 0) {
toast.error("Username is required");
return;
}
if (loginData.password.length === 0) {
toast.error("Password is required");
return;
}
setIsProcessing(true);
try {
const result = await api.login(loginData);
if (result.success) {
const userPreferences = await api.getUserPreferences();
dispatch(setUserId(result.userId));
dispatch(setUserPreferences(userPreferences));
thirdPartyLib.refreshSomething();
sessionManager.onSessionCreated(result.session);
}
if (result.success) {
toast.success("Logged in");
}
else {
toast.error("Error");
}
}
catch {
toast.error("Error");
}
finally {
setIsProcessing(false);
}
};Handlers like this one make component code longer and less readable. They often contain code that should be made reusable (e.g. data validation). Furthermore, if requirements change (e.g. data validation params), all related callbacks have to be updated. Let's analyze the above code and figure out what can be done to make it better.
- Data validators should moved to a separate file. Or we could use a library that handles forms and validation.
loginDataobject is passed to an api method. It's small and simple. However, in many cases objects like this are large, contain properties unrelated to component's state and contain values that require some transformation. It's better to move that code into a function placed in a separate file.- Lines 14-36 contain a common scheme: indicate that work is being done, do the work (async), handle errors and indicate that the operation succeeded/failed. A lot of that code could be moved into a custom hook.
- In lines 16-23 we call an api method and do a lot of stuff if it succeeds. Again, this code should not be here. What we want here is just one function call.
8. Not removing props that are no longer used (TypeScript)
Components evolve over time. Sometimes devs remove a feature that is no longer needed - they remove JSX elements, maybe some hooks and logic... but they forget to remove the no longer used prop from the interface:
1
2
3
4
5
6
7
8
9
10
11
12
13export interface UserProps {
name: string;
avatar: string;
avatarSize?: "sm" | "md" | "lg";
}
export function User(props: UserProps) {
return (
<div>
<img src={props.avatar} alt="" />
<span>{props.name}</span>
</div>
);
}IDEs will hint that avatarSize prop is available. Devs may try to use avatarSize prop, but it won't work, because it has been removed. If the prop is not optional, people will be forced to provide a value. Values of unused props may change and cause React to perform some extra work that is not necessary.
Summary
It's easy to create a React app. It's not easy to create a good React app. When multiple solutions are available, people tend to pick the one that is the fastest to implement. This often leads to bad code, which in turn leads to a lot of time being wasted to maintain the code. That's why I believe it's important to choose solutions that save time in the long run.