eslint-plugin-react-hooks introduces a bug to minimal app unless overridden

[cefn]

Following up on #14920 ...

[ESLint] Feedback for 'exhaustive-deps' lint rule

...I believe I have a case which triggers the eslint rule and which creates a looping bug if the auto-fix is applied. I also propose a way the linting might be applied to resolve this.

I wrote a minimal app to explore the issue, to try to figure out an AST inspection which might mitigate the problem. Happy to refactor if you tell me an alternative way to present the backend server which would be preferred to an express dependency.

The app is a 'row editor' which edits rows from a backend, having an id and title. The UI offers a list of row links. The user can navigate to edit a row from the list or create a new row. The front end user is expected to edit the title field. However, the backend also needs to make changes(it adds the id value from the 'database' when a new record is saved).

There are two lines which require an override otherwise the app enters a loop between client and server, creating runaway saves and reloads and making the UI unusable. The override lines needed are at...

https://github.com/cefn/graphql-gist/blob/c0bece7e6b1fda832d57ccb363b1056f7cb2d37b/react-event-loop/client.js#L73
https://github.com/cefn/graphql-gist/blob/c0bece7e6b1fda832d57ccb363b1056f7cb2d37b/react-event-loop/client.js#L87

It could be possible to detect that setLocalRow is called within the body, and therefore allow a dependency of localRow to be commented in the array instead of declared.

Although the minimal client app is generic, the full example is quite detailed, because the async callbacks need a server endpoint to demonstrate the issue. Additionally, the interlocking concerns of e.g. navigating to rows, auto-saving rows, creating new rows contribute to defining the problem. With fewer features supported, the problem doesn't arise.

The backend code is super-simple so hopefully doesn't confuse. If there is a specific refactoring which would help, let me know as this codebase is purely there to demonstrate the issue.

npm install then npm run start then http://localhost:8080 should be enough to observe the working system. Removing the eslint overrides and then auto-fixing will show the issues which arise if the eslint rule is allowed to add localRow or remoteRow to the dependencies.

What is the current behavior?

A loop between client and server if the auto-fix is applied

What is the expected behavior?

The eslint rule should not introduce a dependency which causes a loop. For example, it could instead add a commented reference to localRow in the dependencies instead, to account for the case that setLocalRow is invoked in the body.

[localRow, remoteRow, saveItem]

...could look like...
[ /localRow,/ remoteRow, saveItem]

This would allow the user to decide whether to leave the commented reference in place, or uncomment it to declare the dependency, knowing that it could create a loop. Either comment or reference would satisfy the rule.

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

I observed the issue with "eslint-plugin-react-hooks": "^2.0.1"

[erwald]

I've noticed the same thing in my app. It turns this snippet:

useEffect(() => {
    const { myProp } = props;
    loadSomeData(myProp); // This results in some props changing via the redux store.
  }, []);

into:

useEffect(() => {
    const { myProp } = props;
    loadSomeData(myProp);
  }, [props]);

which causes the app to repeatedly call loadSomeData. I'm no expert but I think that's not how it should work?

[cefn]

The const myProp should be destructured outside of the useEffect. It is also unused.

[erwald]

The const myProp should be destructured outside of the useEffect. It is also unused.

Good point, I've updated the example.

So, well, even though it's wrong to destructure the props inside useEffects, in this case the linter turns a working (albeit wrongly coded) app into a non-working one. But shouldn't autocorrection be conservative enough that it doesn't break any working code?

If it were just a linter error, it would be fine, but when it does the change automatically I'd expect it not to leave the app worse off than it was before. :)

[cefn]

It's inevitable that the linter has to examine the references within the useEffect() callback function in order to do its job, so if you choose to access references there when you don't need to, it correctly assumes they are accessed in the useEffect for some functional reason - i.e. the access has to happen at the time useEffect runs.

If you implement an infinite loop by both accessing a changed variable and triggering a change to the variable within useEffect it will faithfully add the changed item to the variable list, which it must.

If the infinite loop is implemented in code outside the useEffect() (possibly via server-side code) it's impossible for the linter to trace this.

In the original case I shared, the localRow useState() hook combined with use of setLocalRow within the useEffect() means it's detectable by the linter that adding localRow to the variable list will cause an infinite loop. In my view a detectable infinite loop should be handled better by the linter (adding it as a commented item in the variable list, but allowing an uncommented item there to also satisfy the linter).

Does the auto-filling of props in the useEffect variables list stop if you declare the const outside useEffect (and hence don't reference props within useEffect) ?

[erwald]

I'd presume so.

Another place it happened is an admittedly stupid snippet like this:

  useEffect(() => {
    loadXs().then(() => {
      selectY(xs) // select some initial Y-value using our xs
    });
  }, []);

It makes no sense to use xs there as it won't be loaded yet, but this was working code. The autofixer turned it into

  useEffect(() => {
    loadXs().then(() => {
      selectY(xs) // select some initial Y-value using our xs
    });
  }, [loadXs, xs]);

which obviously results in an infinite loop.

I understand that it's not possible for the linter to be aware of these differences. But since wrong but working code is better than correct but broken code, maybe it should be more conservative when autofixing these things?

[erwald]

Anyway, sorry for hijacking your issue. :)

[gaearon]

Closing in favor of #15204.