[react-hooks/exhaustive-deps] eslint --fix breaks the code

[EugeneDraitsev]

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

I want to report a bug

What is the current behavior?

eslint --fix trying to break my hook)

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:

I have a very simple hook:

import { useEffect, useState } from 'react'

export function useSwitchTab(trigger, tabsAmount, initialState = 0) {
  const [currentTab, setTab] = useState(initialState - 1)

  useEffect(() => {
    setTab(currentTab + 1 >= tabsAmount ? 0 : currentTab + 1)
  }, [tabsAmount, trigger])

  return [currentTab, setTab]
}

If props tabsAmount or trigger will change, I need to increase currentTab value by 1. It works fine and looks ok for me, but in eslint-plugin-react-hook rule react-hooks/exhaustive-deps will warn here:

React Hook useEffect has a missing dependency: 'currentTab'. Either include it or remove the dependency array. You can also do a functional update 'setTab(c => ...)' if you only need 'c
urrentTab' in the 'setTab' call react-hooks/exhaustive-deps

And eslint --fix will break my code like this:

import { useEffect, useState } from 'react'

export function useSwitchTab(trigger, tabsAmount, initialState = 0) {
  const [currentTab, setTab] = useState(initialState - 1)

  useEffect(() => {
    setTab(currentTab + 1 >= tabsAmount ? 0 : currentTab + 1)
  }, [currentTab, tabsAmount, trigger])

  return [currentTab, setTab]
}

It will add currentTab in deps for useEffect and this will create endless loop.

What is the expected behavior?

Eslint shouldn't fix this warning with --fix option, It may break the code.

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

react: 16.8.5
eslint-plugin-react-hooks: 1.6.0

[infodusha]

Your code is buggy. Hook must be like that:

import { useEffect, useState } from 'react'

export function useSwitchTab(trigger, tabsAmount, initialState = 0) {
  const [currentTab, setTab] = useState(initialState - 1)

  useEffect(() => {
    setTab(tab => tab + 1 >= tabsAmount ? 0 : tab + 1)
  }, [tabsAmount, trigger]) // eslint-disable-line react-hooks/exhaustive-deps
// also, trigger is unnecessary dependency by default so you need comment line in the top

  return [currentTab, setTab]
}

[EugeneDraitsev]

@infodusha ,
Thanks for reasonable code fix. Main problem here, that eslint --fix will break my version code at all. Eslint shouldn't break code in any case (it shouldn't create endless loops in any situation)

[infodusha]

@EugeneDraitsev
// eslint-disable-line react-hooks/exhaustive-deps
will help eslint --fix ignore this rule on line

[EugeneDraitsev]

@infodusha
I understand how eslint-disable works. Problem is in semantic of eslint --fix.

Just imagine situation I have my version of code - it works fine, I pushed my changes to remote. CI executes 'eslint --fix' and deploy broken version of code to production (just because this is autofixable warning)

Eslint --fix should never change logic of your code. But in my example eslint with this rule will change logic of my code. I pretty sure that this warning should be not-autofixable with --fix.

[infodusha]

@EugeneDraitsev I thought eslint-disabe shoud also disable eslint --fix
Googled that you can disable autofixig this rule as temporary solution: eslint --fix --rule 'react-hooks/exhaustive-deps: off'

[gaearon]

The warning is not a false positive.

You can see how to fix it here:

https://reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often

In this particular case, the fix would be to function form of setState, like in the above comment.

---

It's fair that there is an expectation that eslint --fix shouldn't break the code (even if the code is already broken). The root of the issue here is that ESLint offers no way to offer "suggestions". We'd like a way to indicate that the autofix isn't guaranteed to work, but is a good starting point for the fix (e.g. for IDE integration that lets you manually apply it by clicking a suggestion icon).

We could disable autofix by default, and add an option that enables it. I don't know if this is what most people want.

[behzad888]

@EugeneDraitsev
First, I think you shouldn't use setTab into useEffect because useEffect became for simplified some React's lifecycle methods such as componentDidMount, componentDidUpdate, and componentWillUnmount. Please read this for more information.
Second, Calling setState() in React’s lifecycle methods requires a certain level of caution. There are a few methods where calling setState() would lead to undesirable results and others where it should be avoided completely.

• componentDidMount
  componentDidMount() is invoked immediately after a component is mounted. You may call setState() immediately in componentDidMount(). It will trigger an extra rendering, but it will happen before the browser updates the screen thus render() will be called twice.

• componentDidUpdate
  componentDidUpdate() is invoked immediately after updating occurs. You may call setState() immediately here but know that it must be wrapped in a condition.

• componentWillUnmount
  You should not call setState() here because the component will never be re-rendered. Once a component instance is unmounted, it will never be mounted again.

So, I think if you want to use setTab into useEffect it must be wrapped in a condition.

[EugeneDraitsev]

@behzad888 I understand that my code is not correct from 'hooks best practice' point of view, but this code is still valid and will behave 'as designed'. Often peoples will create hooks, that will be not follow best practices or even documentation.

As I said above and as @gaearon mentioned in 2nd part of his comment:

eslint --fix shouldn't break the code (even if the code is already broken)

From my point of view make sense to change level of react-hooks/exhaustive-deps from warning to error and make it non-autofixable to prevent possibility to break user code. (or at least disable autofix by default)

Of Course important to add some "suggestions" for user (and IDE), but maybe just an error description will be enough in this case.

[gunn4r]

We ran into this issue... my team is new to hooks and we're making all kind of rookie mistakes ;) so when I turned on the new exhaustive deps rule and ran it through auto fix I had this unpleasant effect of code being utterly broken all over the place. So we have to disable the rule completely because people inadvertently run autofix and break code all over.

We're going through and fixing all of these instances but yeah having "working" (yes I realize its broken in a sense) code all of a sudden cause fatal failures seems unacceptable for an "autofix". I don't think we'll ever enable this rule with autofix because we can't trust that it won't produce a fatally broken build.

[ger86]

I have another case to add to this question.

Suppose I have a hook like this:

function useWindowPosition() {
  const [position, setPosition] = useState({ left: 0, top: 0 });
  return [position, setPosition];
}

If I use it in this way (is a simple example so excuse me about the logic, but suppose the useWindowPosition hook returns as the second element of the array a complex function):

  const [position, setPosition] = useWindowPosition();
  useEffect(() => {
    setPosition({ left: 1, top: 1 });
  }, [userId]);

Eslint fires the following:

React Hook useEffect has a missing dependency: 'setPosition'. Either include it or remove the dependency array react-hooks/exhaustive-deps

What is the difference with use:

  const [position, setPosition] = useState({a: 1});
  useEffect(() => {
    setPosition({ left: 1, top: 1 });
  }, [position]);

and why does eslint detect a possible dependency warning but when I use a native hook there is no warning?

[behzad888]

@ger86
native hooks are already into internal git hooks. You also can move your lint into a git hook if you think you need it.

[kpaxqin]

The warning is not a false positive.

You can see how to fix it here:

https://reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often

In this particular case, the fix would be to function form of setState, like in the above comment.

It's fair that there is an expectation that eslint --fix shouldn't break the code (even if the code is already broken). The root of the issue here is that ESLint offers no way to offer "suggestions". We'd like a way to indicate that the autofix isn't guaranteed to work, but is a good starting point for the fix (e.g. for IDE integration that lets you manually apply it by clicking a suggestion icon).

We could disable autofix by default, and add an option that enables it. I don't know if this is what most people want.

Here is my case : #16006

Disable autofix by default sounds good for me