React's bogus setState anti-pattern

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.

Comments

[Anton Korzunov]

if your next state depends on the previous one, use the function overload.

In reality, this is a quite rare situation. Usually, if you want to set something - you want to set something.

Even more - sometimes, when you depend on the previous state, let say I am talking about toggle command, it might be a mistake to depend on the previous state.

Lets imagine - you have a button, a toggle button, and once you clicked it - you toggle something. So - should toggle show something, or should toggle hide something would be defined when you click. If you clicked twice, some people like to double click - it shall still do the job you expected it to do. And your expectations were based on a visible state. Tada :)

So - it's not an antipattern, - it's a feature.

[Josep M Sobrepere]

Hi Anton!

In reality, this is a quite rare situation. Usually, if you want to set something - you want to set something.

I think that really depends of the nature of the data that you are working with... In my company we work with real-time data, and I assure you that this is not a rare situation for us. On the contrary, many times we need to update parts of the state depending on the value of other parts. Those others could have not finished updating yet, so relying on the "current state" is a common pitfall. Using the function overload is always the way to go in those situations.

Lets imagine - you have a button, a toggle button, and once you clicked it - you toggle something. So - should toggle show something, or should toggle hide something would be defined when you click. If you clicked twice, some people like to double click - it shall still do the job you expected it to do. And your expectations were based on a visible state. Tada :)

Sorry but that is just not true. Try to double click as fast as you are able to in this sandbox, and then let me know how many times the double-click behaves as a click... I've tried it a thousand times with no luck. I have even tried throtling the CPU and nope, nothing.

The only way to accomplish that would be to add some debouncing into the click, which you shouldn't do because the double-click sensitivity is user-configurable and it varies a lot depending on browser, OS, etc... From an accessibility stand-point that should be something to be avoided at all costs. Basically, trying to make a click behave like a double-click or vice-versa should always be considered a huge no-no.

[Anton Korzunov]

The idea was about - sometimes you have to synchronize actions between how they are visible to a user, and how they are visible to a system. Let's say - it's about the source of truth. You might pick the best source.
Don't be stuck to your use cases, and argue using only your use cases. Don't deal in absolutes.

• Don't try to find examples when deriving a state from a prev state is essential - there are a lot. It is a very powerful, useful and good pattern. And I never said the opposite.
• Don't try to call explicit setState an anti-pattern. There are many use cases when it would be more right to use it.

To be more concrete - I've seen many examples when developers are using the following code:

this.setState( state => ({
  ...state,
  someValue: true,
});

They heard that this is how it supposed to be used. Usually, I am using two constructs to solve this misunderstanding:

• there must be a better way (❤️ Raymond Hettinger)
• there are at least 3 ways to do it ( me )

[Josep M Sobrepere]

To be more concrete - I've seen many examples when developers are using the following code:

In my post I clearly state that:

When replacing the state, use the value overload

I think that I'm a very clear about the fact that this is only an anti-pattern when your next state depends on the previous one.

Don't try to find examples when deriving a state from a prev state is essential. It is a very powerful, useful and good pattern. And I never said the opposite.

Then I guess that I misunderstood you when you said that:

So - it's not an antipattern, - it's a feature.

My response to you had to do with this statement:

Lets imagine - you have a button, a toggle button, and once you clicked it - you toggle something. So - should toggle show something, or should toggle hide something would be defined when you click. If you clicked twice, some people like to double click - it shall still do the job you expected it to do. And your expectations were based on a visible state. Tada :)

Which, as I already pointed out to you before, it is wrong in many different ways: first because it's not true that by not using the function overload you would get the behaviour that you are describing, and even that was true, that wouldn't be a feature, it would be a bug... :-)

Don't try to call explicit setState an anti-pattern. There are many use cases when it would be more right to use it.

But I don't. Really, I do not. Please read my post again. I only call it an anti-pattern

if your next state depends on the previous one

That sentence appears like 4 times in the post.

Do you know of a single case where advising developers to use the function overload for those cases when they want to "evolve" the previous state would be harmful?

[Anton Korzunov]

Do you know of a single case where advising developers to use the function overload for those cases when they want to "evolve" the previous state would be harmful?

You just pointed on when it would be not(harmful) - "when they want to "evolve" the previous state".

• When they would not want (CRDT, replication, synchronization, not evolve actions and so on) - it would be unnessesary. Just code not as clear, at it could be.
• When it would be harmful? Only in situations when you are evolving from a previous state, while you shall not. But that would be not a code-level bug, but a wrong design decision.

Bonus: delays in the execution("concurency" or network), or just lags (cheap phones), makes more clear which way you should use in every case. I mean - state management could be a tricky thing. If not today, then yesterday.