Bug: react-hooks/set-state-in-effect overly strict?

[karlhorky]

eslint-plugin-react-hooks version: 6.1.1

Steps To Reproduce

1. Use common, valid patterns for setting state in an effect, from those in the wild and official docs (see patterns in section below)
2. Problem reported by eslint-plugin-react-hooks 💥

Link to code example: https://codesandbox.io/p/devbox/still-resonance-lzsl44?file=%2Fapp%2FComponent.tsx%3A10%2C49&workspaceId=ws_GfAuHrswXyA1DoeSwsjjjz

$ pnpm eslint . --max-warnings 0

/project/workspace/app/Component.tsx
  7:5  error  Error: Calling setState synchronously within an effect can trigger cascading renders

Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:
* Update external systems with the latest state from React.
* Subscribe for updates from some external system, calling setState in a callback function when external state changes.

Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect).

/project/workspace/app/Component.tsx:7:5
   5 |
   6 |   useEffect(() => {
>  7 |     setDidMount(true);
     |     ^^^^^^^^^^^ Avoid calling setState() directly within an effect
   8 |   }, []);
   9 |
  10 |   return <div>Mounted: {didMount ? "Yes" : "No"}</div>;  react-hooks/set-state-in-effect

✖ 1 problem (1 error, 0 warnings)

Widespread setState in useEffect Patterns

1. Displaying different content on the server and client aka didMount, isClient, isMounted, mounted React docs 1, React docs 2, Next.js docs, next-themes docs, MUI Joy UI docs
2. Setting a value while avoiding cascading renders from @controversial
   [Compiler Bug]: False positives “calling setState synchronously within an effect body” with stable primitive values #34045

I will add any more common patterns I find to the list as I encounter more examples.

Some examples of other, less widespread patterns:

1. Syncing URL.createObjectURL() with state Bug: react-hooks/set-state-in-effect overly strict? #34743 (comment)

The current behavior

react-hooks/set-state-in-effect raises a problem with the message Calling setState synchronously within an effect can trigger cascading renders

The expected behavior

react-hooks/set-state-in-effect is less strict, allowing for common usage patterns while catching possible "effect loops" or other unwanted behavior

Alternatives considered

1. Improved examples on the react-hooks/set-state-in-effect docs page, showing more patterns such as the remediation patterns mentioned below that can be used in lieu of setting state directly in an effect (possible downside: some patterns may not have usable replacements outside of setting state in effects)
2. Improved examples on the react-hooks/set-state-in-effect docs page, showing alternative patterns with setting state directly in an effect eg. using startTransition(), requestAnimationFrame() or setTimeout() (downside: seems wrong, like a code smell or "tricking the lint rule")

History

• The React Compiler team is aware of problems like this and is thinking about how to better handle false positives [Compiler Bug]: False positives “calling setState synchronously within an effect body” with stable primitive values #34045 (comment)

cc @jedwards1211 @josephsavona

[Rel1cx]

I believe for use cases similar to "didMount", there are better alternative patterns that avoid violating the set-state-in-effect rule, as shown below:

import { useEffect, useRef, useState, Fragment } from "react";

export function MyComponent1() {
  const ref = useRef<HTMLDivElement>(null);
  const [didMount, setDidMount] = useState(false);

  useEffect(() => {
    const divRef = ref.current;
    const didMnt = !!divRef;

    setDidMount(didMnt);
  }, []);

  return <div ref={ref}>...</div>;
}

export function MyComponent2() {
  const [didMount, setDidMount] = useState(false);

  return (
    <Fragment
      ref={fragmentInstance => setDidMount(!!fragmentInstance)}
    >
      ...
    </Fragment>
  );
}

export function MyComponent3() {
  const [isConnected, setIsConnected] = useState(false);

  return <div ref={div => setIsConnected(!!div?.isConnected)}>...</div>;
}

Link to React Compiler Playground: https://playground.react.dev/#N4Igzg...

[SukkaW]

Besides ref callback (which runs in layout effect phase), I myself also use this approach:

useSyncExternalStore(
  subscriber,
  () => valueFromClient,
  () => valueFromServer
);

Basically, React should hydrate using the server value (a suspense boundary would be triggered if not provided), and then schedule an immediate re-render with the client value. I myself use this approach when working with localStorage.

[karlhorky]

@Rel1cx @SukkaW Ah, thanks for these tips! 👀

I believe for use cases similar to "didMount", there are better alternative patterns that avoid violating the set-state-in-effect rule, as shown below:

Oh nice, confirmed! Those 3 "didMount" patterns all do not show problems with react-hooks/set-state-in-effect:

CodeSandbox: https://codesandbox.io/p/devbox/eslint-plugin-react-hooks-set-state-in-effect-overly-strict-forked-3yjcg9?file=%2Fapp%2FComponent.tsx%3A1%2C63&workspaceId=ws_GfAuHrswXyA1DoeSwsjjjz

I missed that "values from refs" was a general exception to this rule - even though I did see this code example on the react-hooks/set-state-in-effect docs:

// ✅ setState in an effect is fine if the value comes from a ref
function Tooltip() {
  const ref = useRef(null);
  const [tooltipHeight, setTooltipHeight] = useState(0);

  useLayoutEffect(() => {
    const { height } = ref.current.getBoundingClientRect();
    setTooltipHeight(height);
  }, []);
}

The code example above made me think that this only applied to useLayoutEffect().

Having another example in the docs using useEffect() would make this more clear that it also applies to useEffect() too (I guess there is no distinction between these hooks for this rule).

---

I'm not sure that the patterns mentioned above would completely deal with all common, valid useEffect use cases, but listing them out is a start 👍

Remediation patterns for react-hooks/set-state-in-effect errors

1. "set state in effect with value from ref" in MyComponent1
2. "ref callbacks" in MyComponent2 and MyComponent3
3. "converting derived state variables into normal variables"
4. "set state during rendering"
5. "save previous state and compare during render"
6. "key prop from parent to reset state"
7. "useSyncExternalStore for hydration mismatches" (also mentioned on TkDodo's blog)
8. "avoid hydration mismatches by server-rendering values in React Server Components" (also mentioned in Next.js client hints demo by @rphlmr)

[aweebit]

2. Setting a value while avoiding cascading renders from @controversial
   [Compiler Bug]: False positives “calling setState synchronously within an effect body” with stable primitive values #34045

The ESLint plugin is actually right about reporting an error in this example. For details, please see the comment I've just posted in that issue.

1. Displaying different content on the server and client aka didMount, isClient, isMounted, mounted React docs 1, React docs 2, Next.js docs, next-themes docs, MUI Joy UI docs

I feel like this is the only actually valid case where setting state synchronously within an effect shouldn't be considered an anti-pattern.

Most other problems revealed by this new rule could be very easy to solve if React had support for dependency arrays in the useState hook. A while back, I wrote an entire issue explaining why this extension to React's public API should be made. Now with the new rule, I hope we'll see a lot of people finally realize they've been misusing useEffect this entire time. Hopefully, that realization will bring them to the same idea I had, and so the issue finally gets the attention it deserves.

Here is the issue I'm talking about:

• Reconsider providing a cleaner hook-based solution to derived state #33041

Please have a look at it and give it some support!

[aweebit]

@SukkaW may I know the reason for your downvotes?

[SukkaW]

@SukkaW may I know the reason for your downvotes?

Because

if React had support for dependency arrays in the useState hook

is simply not true.

What you are looking for is a way to replicate UNSAFE_componentWillReceiveProps in functional components. Currently, this pattern (described in the React docs, and also mentioned in your proposal) does the trick:

const [count, setCount] = useState(initialCount);
const [prevInitialCount, setPrevInitialCount] = useState(initialCount);

if (initialCount !== prevInitialCount) {
  setPrevInitialCount(initialCount);
  setCount(initialCount);
}

But to be fair, this can be a userspace hook as well (React doesn't have to add it). @Jack-Works already made useComponentWillReceiveUpdate and contributed to my foxact hooks library a year ago: https://foxact.skk.moe/use-component-will-receive-update/

[aweebit]

What you are looking for is a way to replicate UNSAFE_componentWillReceiveProps in functional components.

You could also see it as looking for a way to replicate getDerivedStateFromProps that was meant to be a safe replacement for UNSAFE_componentWillReceiveProps, and I don't see any problem with that.

Currently, this pattern (described in the React docs, and also mentioned in your proposal) does the trick:

const [count, setCount] = useState(initialCount);
const [prevInitialCount, setPrevInitialCount] = useState(initialCount);

if (initialCount !== prevInitialCount) {
  setPrevInitialCount(initialCount);
  setCount(initialCount);
}

There is a number of problems with this pattern:

• As described in the docs, calling state setters synchronously during rendering results in unnecessary render cycles whose results are simply thrown away, so double work is done for no reason whenever one of the state's dependencies changes (because the setPrevDependency function is always called, it even happens when the state itself doesn't change as a result!)
• Unlike the solution I propose, it goes against React's declarative nature because you call state setters imperatively
• It's very cumbersome and confusing, especially since you have to introduce a new state variable that's not even used in the UI
• It requires a userspace hook to be somewhat bearable, and trust me, that alone already means that 90% of devs just won't use it and will resort to useEffect instead (and it doesn't matter at all how simple that hook is to implement)

[SukkaW]

• As described in the docs, calling state setters synchronously during rendering results in unnecessary render cycles whose results are simply thrown away, so double work is done for no reason whenever one of the state's dependencies changes (because the setPrevDependency function is always called, it even happens when the state itself doesn't change as a result!)

It is simply not true. The setPrev will only be called if changed is true:

https://github.com/SukkaW/foxact/blob/0ffd3afe281b177dff85286470d8e0166e039bc9/packages/foxact/src/use-component-will-receive-update/index.ts#L14

• Unlike the solution I propose, it goes against React's declarative nature because you call state setters imperatively
• It's very cumbersome and confusing, especially since you have to introduce a new state variable that's not even used in the UI
• It requires a userspace hook to be somewhat bearable, and trust me, that alone already means that 90% of devs just won't use it and will resort to useEffect instead (and it doesn't matter at all how simple that hook is to implement)

You will need to step back and look at the whole picture, and take the entire React ecology into consideration:

• What happens to backward compatibility? How existing React libraries adopt this new shape of useState w/o breaking compatibility with old React versions?
• What happens to global state management libraries like Zustand and Redux? How to reset their state based on props changes?
• What happens to other libraries that maintain their own states, like react-hook-form or react-router? react-hook-form only gives you a resetField API, so how do you call that resetField based on a prop change?

And if you carefully consider all these problems, you will realize why useComponentWillReceiveUpdate is the way to go.

[aweebit]

It is simply not true. The setPrev will only be called if changed is true:

https://github.com/SukkaW/foxact/blob/0ffd3afe281b177dff85286470d8e0166e039bc9/packages/foxact/src/use-component-will-receive-update/index.ts#L14

Yes, it will! And that's exactly what I said in this part:

double work is done for no reason whenever one of the state's dependencies changes

And with this next part:

(because the setPrevDependency function is always called, it even happens when the state itself doesn't change as a result!)

I meant cases like this:

const [dependency, setDependency] = useState(0);

const [derived, setDerived] = useState(Math.round(dependency));
useComponentWillReceiveUpdate(() => {
  setDerived(Math.round(dependency));
}, [dependency]);

useEffect(() => setTimeout(() => setDependency(0.25)), []);

The setDependency(0.25) call causes a throwaway render to happen despite the fact that the value of derived doesn't even change in the end because under the hood, setPrev has to be called anyway!

Obviously this code doesn't make much sense, but it's the simplest demonstration I could come up with for the issue. It also shows another problem with the useComponentWillReceiveUpdate approach, and that's the fact that the state derivation logic (calling Math.round(dependency) in this case, but it could also be something more involved) has to be duplicated. With useState extended by a dependency array parameter on the other hand, no duplication would be necessary:

const [derived, setDerived] = useState(Math.round(dependency), [dependency]);

I honestly fail to see why someone would prefer useComponentWillReceiveUpdate to this concise, duplication-free and declarative solution.

You will need to step back and look at the whole picture, and take the entire React ecology into consideration:

• What happens to backward compatibility? How existing React libraries adopt this new shape of useState w/o breaking compatibility with old React versions?

I never said React had to remove support for synchronously calling state setters during rendering though, so I really don't see what the problem here is.

I didn't suggest a breaking change, just an extension of the existing API. If no dependency array is supplied to useState, it should behave in the exact same way like it always did.

And library authors are obviously not supposed to use this new useState dependency array functionality if they want to support React versions that don't have it – they should just keep using the old prevState pattern in that case. As I said: React obviously shouldn't drop support for this pattern with the introduction of useState dependency arrays so that no existing code relying on it stops working.

• What happens to global state management libraries like Zustand and Redux? How to reset their state based on props changes?

I've never used those libraries and don't know what their solutions to resetting state when some other state changes are, but if they offer such solutions, they will of course continue working as is .

• What happens to other libraries that maintain their own states, like react-hook-form or react-router? react-hook-form only gives you a resetField API, so how do you call that resetField based on a prop change?

In the exact same way you always did it till now. Again, I don't suggest any breaking changes. so you must've misunderstood me.

---

Your original answer also included this part:

And even if I call setPrev unconditionally, as long as the new value is the same/referentially equal to the current state, React will skip out the update. You can try in this simple example:

function Component() {
  const [v, setV] = useState(0);
  setV(0);
  return <div>Look, ma! Never re-render!</div>
}

This code example wasn't even relevant to what I'd said about double work being done whenever dependencies change to begin with because it didn't include any additional state variable for storing state values from previous renders, and so, accordingly, there were no setPrevDependency calls either.

But the interesting thing here is that your statement wasn't even true. I don't exactly know why, but when a state setter is synchronously called during rendering, it always causes that render's results to be thrown away immediately and another render to take place instead, even if the call doesn't result in the state variable's value being changed. So actually, the code you provided results in an infinite render loop, and nothing gets displayed in the end. I guess you already figured it out since you ended up deleting that comment part, but I still wanted to comment on it in case someone here can explain to me why React doesn't just detect that an additional render wouldn't change anything since there was no actual state change despite a state setter having been called, and why it then just doesn't carry on with the original render's result instead of throwing it away and going for another render – I've actually been curious about this for a while now.

[aweebit]

If you're interested, you can also have a look at the long discussion I had yesterday about the useState dependency array approach and its advantages over useComponentWillReceiveUpdate (also known as useImmediateEffect) in this other issue:

• Some recommended alternatives to useEffect are less declarative and concise reactjs/react.dev#7012

[Jack-Works]

What you are looking for is a way to replicate UNSAFE_componentWillReceiveProps in functional components.

You could also see it as looking for a way to replicate getDerivedStateFromProps that was meant to be a safe replacement for UNSAFE_componentWillReceiveProps, and I don't see any problem with that.

Currently, this pattern (described in the React docs, and also mentioned in your proposal) does the trick:
const [count, setCount] = useState(initialCount);
const [prevInitialCount, setPrevInitialCount] = useState(initialCount);

if (initialCount !== prevInitialCount) {
setPrevInitialCount(initialCount);
setCount(initialCount);
}

There is a number of problems with this pattern:

* As described in the docs, calling state setters synchronously during rendering results in unnecessary render cycles whose results are simply thrown away, so double work is done for no reason whenever one of the state's dependencies changes _(because the `setPrevDependency` function is always called, it even happens when the state itself doesn't change as a result!)_

* Unlike the solution I propose, it goes against React's declarative nature because you call state setters imperatively

* It's very cumbersome and confusing, especially since you have to introduce a new state variable that's not even used in the UI

* It requires a userspace hook to be somewhat bearable, and trust me, that alone already means that 90% of devs just won't use it and will resort to `useEffect` instead _(and it doesn't matter at all how simple that hook is to implement)_

Hi, this pattern is recommented by the document. https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes useComponentWillReceiveUpdate (the correct way) is just make it as easy as useEffect (the wrong way)

[aweebit]

Hi, this pattern is recommented by the document. https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes useComponentWillReceiveUpdate (the correct way) is just make it as easy as useEffect (the wrong way)

Hi! :) I know the prevState pattern is the recommended one, and I'm also very well aware of why useEffect shouldn't be used for resetting state when some other state changes. In #33041, my entire reasoning is based on comparing my suggested solution to those two approaches, and the link you shared is actually also included in that issue's description.

What I suggest is that React introduces an alternative to the prevState / useComponentWillReceiveUpdate pattern that wouldn't have these disadvantages of the latter that I listed earlier:

• As described in the docs, calling state setters synchronously during rendering results in unnecessary render cycles whose results are simply thrown away, so double work is done for no reason whenever one of the state's dependencies changes (because the setPrevDependency function is always called, it even happens when the state itself doesn't change as a result!)
• Unlike the solution I propose, it goes against React's declarative nature because you call state setters imperatively
• It's very cumbersome and confusing, especially since you have to introduce a new state variable that's not even used in the UI
• It requires a userspace hook to be somewhat bearable, and trust me, that alone already means that 90% of devs just won't use it and will resort to useEffect instead (and it doesn't matter at all how simple that hook is to implement)

Also it wouldn't have the state derivation logic duplication disadvantage that I mentioned in my last comment.

For more detailed explanations and motivating examples, please check these issues:

• Reconsider providing a cleaner hook-based solution to derived state #33041
• Some recommended alternatives to useEffect are less declarative and concise reactjs/react.dev#7012