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

cefn
创建于
2019-09-28 15:50:37

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"

7条回答
erwald
回复于
2019-12-11 09:48:56
#1

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
回复于
2019-12-11 12:52:21
#2

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

erwald
回复于
2019-12-11 13:46:42
#3

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
回复于
2019-12-11 14:00:20
#4

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
回复于
2019-12-11 14:31:09
#5

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
回复于
2019-12-11 14:32:16
#6

Anyway, sorry for hijacking your issue. :)

gaearon
回复于
2019-12-11 16:25:19
#7

Closing in favor of #15204.

当前位于第1页,总计7 条回复

推荐相似问题

Initial State object modified by React, No Warning

Feature request - Warning for direct state mutation & no mutation on initial state object What is the current behavior?
讨论数 2
react
创建时间:2019-09-28 10:16:14

React 16.10 broke Next.js/SSR applications

Do you want to request a feature or report a bug? Bug What is the current behavior? React 16.10.0 has broken all Next.js
讨论数 11
react
创建时间:2019-09-28 03:18:10

Error: "Minified React error

Do you want to request a feature or report a bug? What is the current behavior? If the current behavior is a bug, please
讨论数 3
react
创建时间:2019-09-27 21:35:51

Error: "Minified React error

Do you want to request a feature or report a bug? What is the current behavior? If the current behavior is a bug, please
讨论数 3
react
创建时间:2019-09-27 21:30:25

Feature request: adding dangerouslySetOuterHTML (or HTML-comment component)

Currently there is only the option to have dangerouslySetInnerHTML, but there is no way to create a raw HTML output (lik
讨论数 4
react
创建时间:2019-09-27 19:42:46

Provide Context.currentValue

Now that hooks have an easy, non-HOC, non-renderprop, way of accessing context: const value = useContext(MyContext); It
讨论数 8
react
创建时间:2019-09-27 19:09:52

React 16.8.6 upgrade - Warning The component appears to be a function

When trying to upgrade my codebase to React 16.8.6 from React 16.6.0, all of my functional components are throwing the b
讨论数 5
react
创建时间:2019-09-27 18:30:42

Error: "Minified React error

Do you want to request a feature or report a bug? What is the current behavior? If the current behavior is a bug, please
讨论数 3
react
创建时间:2019-09-27 15:11:30

Error: "Minified React error" in React dev tools when using Components tab

Do you want to request a feature or report a bug? Bug What is the current behavior? React Dev tools gets this error when
讨论数 8
react
创建时间:2019-09-27 14:29:52

Error: "Minified React error #301"

Do you want to request a feature or report a bug? Bug What is the current behavior? The Chrome React Developement Tool t
讨论数 7
react
创建时间:2019-09-27 13:32:31