React: [ESLint]react-hooks/exhaustive-deps rule autofix modifies code function, violating eslint best practices

[Alphy11]

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

What is the current behavior?
Consider:

function Example({ fetchData, someArg }) {
  let data = 'defaultValue';
  useEffect(() => {
    data = fetchData(someArg);
 }, [someArg]);

  return JSON.stringify(data);
}

auto fix will automatically change this to:

function Example({ fetchData, someArg }) {
  let data = 'defaultValue';
  useEffect(() => {
    data = fetchData(someArg);
 }, [someArg, fetchData]);

  return JSON.stringify(data);
}

Here we have a simple data fetch component that should refetch data when parameters change, but not when the fetch function changes. Whether this is right or not, it is legitimate code, that the lint fix will cause serious problems in the code if it is used like this:

function ExmapleUsage({ fetchData }) {
  return <Example fetchData={( arg ) => fetchData('Hello World', arg)} someArg="Goodbye" /> 
}

What is the expected behavior?

Eslint best practices say that fix rules should not change functionality of code, so that you can safely run fix and expect no functional changes to be made. This rule directly breaks that. I as a repo maintainer see the auto fix as a greater risk than the problems the lint rule prevents. If autofix was turned off, the rule would be entirely a positive.
Unfortunately, ESLint also has rejected the idea of disabling autofix for certain rules. So not following best practices is not ideal.
Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Not a React Bug but a lint rule.

[gaearon]

Yep. We’re hoping https://github.com/eslint/rfcs/pull/30 will let us resolve this in a nice way.

[devserkan]

I realized today that the linter does not warn about local functions anymore. Is it related to this issue? Is it possible that the warning is disabled right now?

[codepandy]

auto fix is really annoying.sometimes I didn't need the dependencies。
how to resolve it?

[gaearon]

sometimes I didn't need the dependencies。

This is usually a mistake and a misunderstanding.
Please read this:

• https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies
• https://reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often

As I said before, eslint/rfcs#30 will allow us to make autofix manual-only, but it's not ready yet.

[terrysahaidak]

@gaearon Hi Dan, here is something really similar in react-native world:

function App({ navigation }) {
  React.useEffect(() => {
    navigation.setParams({ myParam: 'test' });

    // it will add navigation here which will cause infinity loop
  }, []);

  return (
    ...
  )
}

Here is the link to the reproducing example.

[gaearon]

@terrysahaidak That can be fixed on the navigation library side since there's no way for React to tell whether a value is static or dynamic.

[gaearon]

There's movement on https://github.com/eslint/eslint/pull/12384, which should let us solve this. Let's keep an eye on it.

[peter-wan-intuit]

Completely agree that an eslint fix should NEVER change logic.

This can very easily introduce infinite loops, which in my case, surfaced only after a git hook ran eslint fix as part of the commit process. I.E. working code was tested locally, and but unknowingly broke after committing (due to this rule adding missing conditions), which is super easy to miss.

@gaearon That linked eslint PR has been accepted, but is it directly related to this problem? I'm guessing you mean that this react lint flag should be changed to add a "suggestion" rather than make the code change as part of an eslint fix?

[terrysahaidak]

@peter-wan-intuit what was basically happened to me on react native - it kept adding navigation prop to useEffect where I been setting a param on it which may change navigation ref link (but not the setParam method link), so it caused infinity loop.

[gaearon]

I'm guessing you mean that this react lint flag should be changed to add a "suggestion" rather than make the code change as part of an eslint fix?

Yes. That's what https://github.com/facebook/react/pull/17385 does.

[StephenHaney]

Just checking in on this - if #17385 gets held up for a while waiting on vscode-eslint, are there any options right now to disable autofixing for this rule besides turning the rule off?

I checked around in eslint docs and I don't see a way to just turn off autofix for one rule other than disabling the rule completely. Appreciate any pointers.

[gaearon]

I don't think there's any way, no.

[nashspence]

I just got hit by this today after enabling the recommended settings for this plugin. This is bad news guys. I had to spend 20 mins debugging my code only to figure out that the linter had auto-fixed my [] to [props]. Please do not include an auto fix for this. You will break others like you did me. Just because it’s not a common use case doesn’t mean people aren’t using it.

[gaearon]

@nashspence Sorry to hear that. To be clear, we understand the issue — it's what this thread is about. Once https://github.com/microsoft/vscode-eslint/pull/814 ships, we will merge and release the update that uses the Suggestions API instead.

[nashspence]

@gaearon Cool cool. No worries man. I just wanted to whine a bit. Great job on everything else and whatnot. Honestly, I’m all good with the ignore on the crucial line at this point.

[zsolt-dev]

I hope the mistake to autofix will be resolved soon, since I have the rule turned off completely.

It was a bad idea from the beginning and I did not understand why people were pushing it so hard even when it was breaking the "not change functionality of code" rule.

[zeorin]

I have just wasted _hours_ of time because the exhaustive-deps rule thought that a certain dependency was not a dependency, and the fix removed it. I didn't know that this rule had a fix that would change the execution of the code. ESLint fixes should never do that! So when I see a fix happen on save when I'm writing code, I don't anticipate that it'll change the execution of my code.

As a result, I was getting very strange behaviour which took me hours to debug, partly because I thought the problem was in a different file (where I was doing most of the work; I've now needlessly re-written it 3 times), and partly because I thought that I must be misunderstanding useMemo and useCallback. But really the issue was not mine, it was that exhaustive-deps thought that a dependency was not one and the rule has a fix. As a result of that fix, my mental model of what my code was and what the code _actually was_ were very different, something that no one that uses ESLint expects!

I think it is incredibly poor practice not to remove the fix! I appreciate that you wanted to use a new ESLint feature to offer users the option to fix something that might change the code execution, but in the meantime you should have just switched it off. See https://github.com/eslint/eslint/issues/10242 for a similar problem when no-debugger would also be "fixed" (which was changed because _ESLint fixes shouldn't ever automatically change the meaning of the code_).

The React team is usually really good about not breaking code when making changes. You have really well-documented and thought out upgrade paths, deprecations, documentation, etc., but in this case you've failed spectacularly on that front.

[gaearon]

@zeorin

I understand your frustration. But adding more to the thread when we're already aware of the problem, have a solution in testing, and asked to refrain from commenting unless you have something new to add, isn't helpful. I'm locking this thread because it's creating notification spam for people subscribed to it who would like to know about the solution rather than more "+1" reports.

I should add that I agree with you in principle. However, there is an argument that Suggestions API wouldn't have made it to the RFC stage and past it in the first place if this practical example (which a large part of the motivation for that RFC) wasn't so painful. So yes, it's unfortunate, but also — we're going to have a solution to this, and this temporary pain was a part of it.

[gaearon]

I published [email protected] which switches to use Suggestions API (needs eslint@>=6.7.0 to work). It includes https://github.com/facebook/react/pull/17385 and no other changes. Please give it a try! (Here's a corresponding release to try it with.)

If it doesn't work, please comment on https://github.com/facebook/react/pull/17385.

[gaearon]

[email protected] is out with a fix for this.
It will now use the Suggestions API. The autofix is gone.

To restore the IDE functionality, you'll need to have eslint@>=6.7.0 and a compatible IDE extension — such as this vscode-eslint prerelease.

[mihanizm56]

can't use your update - create-react-app 3.4.1 wants eslint 6.6.0 =(((

[gogoyqj]

why code below throw an eslint@7 message:

React Hook useCallback has a missing dependency: 'props'. Either include it or remove the dependency array. However, 'props' will change when any prop changes, so the preferred fix is to destructure the 'props' object outside of the useCallback call and refer to those specific props inside useCallback.

    const onSectionDragEnd = useCallback(
      () => {
        props.onSectionDragEnd && props.onSectionDragEnd();
      },
      [props.onSectionDragEnd]
    );

it should work, anything wrong?

[email protected] throw nothing

[gaearon]

can't use your update - create-react-app 3.4.1 wants eslint 6.6.0 =(((

The autofix was disabled in [email protected] which doesn't need any new ESLint version.

For Suggestions API yes, you'll need to wait for CRA to catch up. They're doing a release soon.

[gaearon]

it should work, anything wrong?

const { onSectionDragEnd } = props
const onSectionDragEnd = useCallback(() => {
  onSectionDragEnd && onSectionDragEnd();
}, [onSectionDragEnd]);

Did you have a chance to read the message? It tells you to do exactly that:

so the preferred fix is to destructure the 'props' object outside of the useCallback call and refer to those specific props inside useCallback.

[gogoyqj]

@gaearon certainly i did, i just wonder why, it should work and there's no logical mistake - although it could be fixed by a [props] or destructuring assignment:

    const onSectionDragEnd = useCallback(
      () => {
        props.onSectionDragEnd && props.onSectionDragEnd();
      },
      [props.onSectionDragEnd]
    );

[maximelebastard]

Hi
I still get my hooks dependencies modified when running eslint --fix. For what I understand it should not be the case, isn't it ?

[gaearon]

Not if you’re using version 2.4 of the plugin or more recent.

[maximelebastard]

Well, according to yarn info eslint-plugin-react-hooks, I'm using the version 4.0.8 and the command eslint ./src ./amplify --ext .js,.jsx,.ts,.tsx --format codeframe --fix adds items to my useEffect dependencies arrays. Is that a bug ?

[skitterm]

I'm also seeing autofixing for this rule.

My dependencies:

• eslint-plugin-react-hooks: 4.2.0
• eslint: 6.8.0
• typescript-eslint: 4.0.1
• vscode eslint extension: 2.1.13

My (simplified) config file:

{
  "extends": ["plugin:react/recommended"],
  "plugins": ["react", "react-hooks"],
  "rules": {
    "react-hooks/exhaustive-deps": "warn"
  }
}

[gaearon]

I can't help without a reproducing project.

[skitterm]

@gaearon I created a project from-scratch with eslint, React, and the eslint-plugin-react-hooks in VSCode and was unable to reproduce the autofixing issue. The autofixing must be caused by another tool in my specific project, sorry for the noise.

[sedghi]

@skitterm did you find out what tool is autofixing your code? I have similar issue, eslint is autofixing the dependency in the useEffect.