[eslint-plugin-react-hooks] setState in effect guard prevents cases like DOM measurement

[billyjanitsch]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

The new guard against a direct call to setState inside of an effect (#15184) seems to prevent a class of patterns where the value being set is dependent on something other than props. For example, the rule disallows storing a value read from the DOM via a ref (see below).

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

function MeasuredButton(props) {
  const buttonRef = useRef(null)
  const [buttonWidth, setButtonWidth] = useState(0)

  useLayoutEffect(() => {
    if (buttonRef.current) {
      // we rely on the same value bailout to avoid an infinite loop
      setButtonWidth(buttonRef.current.clientWidth)
      // we could bail out explicitly instead:
      // const {clientWidth} = buttonRef.current
      // if (clientWidth !== buttonWidth) setButtonWidth(clientWidth)
      // but the linter would still disallow it
    }
  })

  return (
    <>
      <button ref={buttonRef}>{props.children}</button>
      Button width: {buttonWidth}
    </>
  )
}

This code yields the error:

React Hook useLayoutEffect contains a call to 'setButtonWidth'. Without a list of dependencies, this can lead to an infinite chain of updates. To fix this, pass [] as a second argument to the useLayoutEffect Hook.

The auto-fix breaks the component because the width no longer updates on subsequent renders.

What is the expected behavior?

Basically, the guard assumes that the infinite loop problem can always be solved by adding a dependency array. This is true when setting a value derived from props (such as data returned from a request based on a prop), but not when the source of the value can only be retrieved inside of the effect (such as a DOM measurement). In the latter case, an infinite loop has to be avoided by adding a condition or relying on the same value bailout.

Is this known and/or intentional? I notice that #15184 considered early returns, which would help.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React: 16.8.6
eslint-plugin-react-hooks: 1.6.0

[eps1lon]

This can still cause infinite loops if the width of the <button /> depends on buttonwidth. It would create a size explosion if you set the width of the button to buttonWidth + 20 and then the layout reads a different value, updates and then the cycle starts again. There's no way that a linter can verify this statically.

The other issue is if you add other hooks that change the size of the button during render.

If you think it's safe you can eslint-ignore it. Otherwise you should probably use ResizeObserver or save the latest measurement in a ref and read from that when necessary.

[billyjanitsch]

This can still cause infinite loops if...

Yeah, there are many ways to cause an infinite loop, and the linter can't verify this either way. My point is that there are at least some cases which don't result in an infinite loop, and the linter errors in these cases even though there is no general way to refactor them using a dependency array. (And the refactor it suggests is incorrect.)

I guess it's a question of whether the rule intends to error in cases which might be incorrect, or only in cases which are definitely incorrect.

If you think it's safe you can eslint-ignore it.

Yeah, but the auto-fix is unfortunate, since it may break my component before I even realize there was a linter error to disable (e.g. if I have auto-fix on save enabled). I know the rule doesn't mind suggesting "breaking" auto-fixes if there's already a bug in the code, but maybe it should be more conservative about providing a potentially-incorrect auto-fix in cases where it's possible that the existing code is correct. (I know you're also considering adding an option to disable auto-fix entirely.)

Otherwise you should probably use ResizeObserver or save the latest measurement in a ref and read from that when necessary

ResizeObserver works when reading width/height but not when reading from the DOM in general. I don't see how the ref would work if the measurement needs to be read at render time (as in my example). It'll always render the previously-measured value?

[gaearon]

Note the linter suggestion just makes the existing problem in your approach more apparent:

1. Your original code doesn’t guarantee any particular size changes will be observed because they can be caused by children relayout.

2. Your original code can go into an infinite loop if the size itself isn’t stable and flaps between two values.

So I wouldn’t count it as a false positive. The linter’s suggestion exposes a broader flaw with the approach. The original code is fragile and would randomly work or not work depending on accidental timing of re-rendering. (Which easily can change over time as you add or remove state changes in and around this component.)

[eps1lon]

e.g. if I have auto-fix on save enabled

Yeah that could be annoying if you don't catch it. Honestly I wouldn't use this this lint errors are usually about code behavior. You could always move this into a git hook if you think you need this.

how the ref would work if the measurement needs to be read at render time

At render time you can only have the value from the previous commit (if we're talking layout effect + ref). Since render should be pure you shouldn't read from the DOM during render. So what you can do is write the value from the DOM into a ref and read that value if you need to. If you want to monitor the size of some element then using the ResizeObserver is probably the better option though it can still cause infinite loops if the size is unstable.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!