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.

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
);

[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!