Bug: react-hooks/set-state-in-effect: false-positive with ternary

[silverwind]

React version: [email protected], [email protected]

Steps To Reproduce

function getWidth(el: HTMLElement) {
  return el.clientWidth;
}

function Component({value}: {value: string}) {
  const [width, setWidth] = useState(0);
  const ref = useRef<HTMLDivElement>(null);

  useEffect(() => {
    setWidth(ref.current ? getWidth(ref.current) : 0);
//  ^^^^^^^^^^^^ Avoid calling setState() directly within an effect
  }, [value]);

  return <div ref={ref}>{value}{width}</div>;
}

Changing the line to setWidth(getWidth(ref.current)); makes the error go away, so it seems the presence of the ternary operator is confusing the rule.

The current behavior

Error is raised.

The expected behavior

No error to be raised.

[matskan]

Hi! It's a error from eslint, not a bug in react. You can turn it off or change to warning in your eslint config if it annoys you. Take a look at that issue.

[silverwind]

It's a bug in https://github.com/facebook/react/tree/main/packages/eslint-plugin-react-hooks which is hosted in this repo.

[Kushagra-Agarwal22]

The warning is about doing a state update in an effect that depends on something that changes every render (like layout), which can cause loops or jank. A common fix is to only measure when the ref is set and avoid tying it to value directly. function getWidth(el: HTMLElement) { return el.clientWidth; }

function Component({ value }: { value: string }) { const [width, setWidth] = useState(0); const ref = useRef<HTMLDivElement>(null);

useLayoutEffect(() => { if (!ref.current) return; const nextWidth = getWidth(ref.current); // Only update if width actually changed to avoid useless renders setWidth(prev => (prev === nextWidth ? prev : nextWidth)); }, [value]); // or [] if you only want to measure once

return (

{value} {width}

); }

[silverwind]

Yeah that may be an improvement but my example is not actually representative of the code where I had this issue.

It's specifically the ternary that triggers the bug. I can replace the ternary with a equivalent if and that passes the rule, so the bug is specifically related to the ternary. This raises no error:

useEffect(() => {
  if (ref.current) {
    setWidth(getWidth(ref.current));
  } else {
    setWidth(0);
  }
}, [value]);

This does:

useEffect(() => {
  setWidth(ref.current ? getWidth(ref.current) : 0);
}, [value]);

Both are exactly equivalent in behaviour, so it is clearly a bug in the compiler or rule.

[aweebit]

Actually, the bug here is that no error is reported for setWidth(getWidth(ref.current)). It should be reported because you're adjusting state synchronously from within an effect, and that is the anti-pattern that the ESLint rule is there to prevent.

The right thing to do here would be to use the useLayoutEffect hook made specifically for cases like this where you want to derive state from DOM measurements.

[silverwind]

Actually, the bug here is that no error is reported for setWidth(getWidth(ref.current)). It should be reported because you're adjusting state synchronously from within an effect, and that is the anti-pattern that the ESLint rule is there to prevent.

According to this example in the rule docs, values that come from a ref are fine and an exception.

The right thing to do here would be to use the useLayoutEffect

Yeah for this example, useLayoutEffect may be preferrable, but as far as this lint rule is concerned, useLayoutEffect and useEffect should be treated equally because they are both effects.

[Roma4455]

The current behavior