React's bogus setState anti-pattern

Josep M Sobrepere — Sun, 21 Apr 2019 23:37:00 +0000

TL; DR

This is bad:

function useCounter() {
  const [count, setCount] = useState(0)

  const increment = useCallback(() => setCount(count + 1), [count])
  const decrement = useCallback(() => setCount(count - 1), [count])

  return { count, increment, decrement }
}

This is good:

function useCounter() {
  const [count, setCount] = useState(0)

  const increment = useCallback(() => setCount(x => x + 1), [])
  const decrement = useCallback(() => setCount(x => x - 1), [])

  return { count, increment, decrement }
}

Rule of thumb

When transforming the state, use the function overload. Otherwise, you may not be working with the latest state.

When replacing the state, use the value overload.

What's wrong with the first implementation?

Basically, things won't work correctly if either increment or decrement get invoked more than once during the same event handler.

To illustrate this issue, let's check how composable useCounter is:

function useNCounter(nTimes) {
  const {count, increment: inc, decrement: dec} = useCounter();

  const increment = useCallback(() => {
    for (let i = 0; i < nTimes; i++) {
      inc();
    }
  }, [nTimes])

  const decrement = useCallback(() => {
    for (let i = 0; i < nTimes; i++) {
      dec();
    }
  }, [nTimes])

  return { count, increment, decrement };
}

useNCouter is a hook that enhances useCounter by accepting a parameter that represents the number of times that the counter should increase/decrease.

In this codesanbox -which uses the first implementation of useCounter- we can see how useNCounter doesn't work correctly. On the other hand, in this other codesandbox -which uses the second implementation- useNCounter works just fine.

Why are those 2 implementations not equivalent?

React batches the updates that happen inside its event handlers in order to avoid pointless evaluations of the render function.

With the initial implementation, the increment/decrement functions always set the same value. It's not until that value gets updated that a new callback function is created. And that doesn't happen until the next update. That's why setState should be treated as an asynchronous function.

Is this problem specific to hooks?

Nope. The traditional this.setState is also asynchronous. So, just remember this simple rule: if your next state depends on the previous one, use the function overload.

Is there an ESLint rule for this?

Not that I know of.

Is this actually an issue?

In my experience, this anti-pattern is responsible for lots of unexpected bugs. The code I have used comes from the main example of the react-hooks-testing-library, and I have seen it in many other places, like in Rangle's blog-post, or in Dan Abramov's post.

No way! Dan Abramov wouldn't make a mistake like that! You have to be wrong!

Ok, you are right. Dan Abramov knows what he is doing. That code works just fine.

However, just to try to prove my point, a slightly better implementation could have been:

function Counter() {
  const [count, setCount] = useState(0);
  const incCount = useCallback(() => setCount(x => x + 1), []);

  useInterval(incCount, 1000);

  return <h1>{count}</h1>;
}

The point that I'm trying to make here is that if your next state depends on the previous one, it's always a good idea to use the function overload.

Finally, I don't want to unfairly criticize Dan's code from the post. I think the main reason why he didn't use the function overload is because he didn't want to confuse the reader with too many concepts at once. That is a great post, you should read it if you haven't yet.

DEV Community: Josep M Sobrepere