facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
225.08k stars 45.89k forks source link

eslint-plugin-react-hooks should report errors inside unnamed functions #14404

Open paboulos opened 5 years ago

paboulos commented 5 years ago

I want to report a bug for the hooks plugin.

What is the current behavior? There was no error report after running eslint, but the component failed when running in the browser. From the chrome dev console it reported "Uncaught Error: Rendered fewer hooks than expected. This may be caused by an accidental early return statement."

**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. Here is a link to the github repo: https://github.com/paboulos/react-hooks-eslint

What is the expected behavior? Followed The Hooks API guide which says React hooks provides a linter plugin to enforce these rules automatically.Therefore it should have reported a usage violation when the eslint hooks plugin is specified. Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React? Using window 10 OS and Chrome browser.

  1. First ran npx create-react-app Hooks
  2. Installed react 16.7.0-alpha.2 and react-dom 16.7.0-alpha.2
  3. Installed eslint dev dependencies: "babel-eslint": "9.0.0", "babel-loader": "8.0.4", "eslint": "5.9.0", "eslint-config-airbnb": "17.1.0", "eslint-loader": "2.1.1", "eslint-plugin-import": "2.14.0", "eslint-plugin-jsx-a11y": "6.1.2", "eslint-plugin-react": "7.11.1", "eslint-plugin-react-hooks": "0.0.0"
  4. Created the .eslintrc.json following the instructions from the Hooks API Doc Then ran package script lint as follows: "npm run lint" no errors reported. Then ran package script start as follows: "npm start" The React component CountHooks calls useState incorrectly and reports error in the browser dev console.
threepointone commented 5 years ago

I wasn't able to reproduce this in codesandbox (ie - it shows as a lint error correctly - https://codesandbox.io/s/n4pjo8xvjj).

It also appears you've set up eslint with this rule

 "react-hooks/rules-of-hooks": ["error","never"],

I'm not super familiar with eslint config options, but doesn't "never" mean the rule is disabled?

oskarv commented 5 years ago

Yeah, you should try it with config from documentation: https://reactjs.org/docs/hooks-rules.html#eslint-plugin // ESLint configuration "rules": { "react-hooks/rules-of-hooks": "error" } It is slightly different from what you have in your project.

paboulos commented 5 years ago

See below, and changed it to "error" as per the npm package doc for hooks, still doesn't work on windows 10 OS. Are you saying there is no support on windows? https://www.npmjs.com/package/eslint-plugin-react-hooks says this is what it should be. { "plugins": [ // ... "react-hooks" ], "rules": { // ... "react-hooks/rules-of-hooks": "error" } }

And this is what is in my lint config: { "plugins": [ "react", "react-hooks" ], "extends": [ "react-app", "airbnb" ], "rules": { "react-hooks/rules-of-hooks": "error", "react/jsx-filename-extension": [1, { "extensions": [".js", ".jsx"] }], "semi": ["error", "always"], "quotes": ["error", "single"], "jsx-quotes": ["error", "prefer-double"], "react/jsx-one-expression-per-line": "off" }, "settings": { "import/ignore": [".css$","node_modules/*"] } }

oskarv commented 5 years ago

Dumb question: Did you run npm install?

paboulos commented 5 years ago

Yes and did it now to be sure: "npm install" and "npm run lint". Still no problem from eslint reported.

paboulos commented 5 years ago

I just cloned the repo to Ubuntu and ran "npm install" and "npm run lint". Also shows no error from eslint. To confirm eslint detection I then deleted a semicolon. It reports missing semicolon error. So the lint problem is specific to the hooks error only.

gaearon commented 5 years ago

We'll need a full reproducing project with exact instructions to reproduce. Thanks.

Sorry, you provided one! I missed it.

gaearon commented 5 years ago

Seems like it doesn't work with a definition like this:

export default () => {
  if (condition) {
    useState()
  }
}

or like this:

export default function() {
  if (condition) {
    useState()
  }
}

but works with a named function:

export default function App() {
  if (condition) {
    useState()
  }
}

We strongly encourage you to give name to your components (so that they show up in DevTools and warnings properly). But we should probably fix this.

oskarv commented 5 years ago

Great catch! I was only looking at the config not the actual code.

Yurickh commented 5 years ago

The linter as of today is looking at the name of the function to check if it a component or a custom hook. I'm not really sure if it is ok for it to consider every anonymously exported function a component/custom hook. I don't think it makes sense to check for hook usage outside of these contexts, either.

nikek commented 5 years ago

I just stumbled upon this too. And changing the anonymous function to a named one indeed made the linter find the hooks issue i was expecting it to find. TIL: Name components and custom hooks that I export default.

paboulos commented 5 years ago

Yes it is a common practice which is why a naming components rule belongs in the linter.

wouterh commented 5 years ago

The same problem arises when you wrap a component in observer from https://github.com/mobxjs/mobx-react-lite.

This is caught as an error:

export const App = () => {
  if(condition) {
    useState()
  }
}

but this isn't:

export const App = observer(() => {
  if(condition) {
    useState()
  }
})

The workaround is to declare it as follows:

const AppBase = () => {
  if(condition) {
    useState()
  }
}

export const App = observer(AppBase)
thisissami commented 5 years ago

Ah, ok... I'm happy to have found this thread. I was confused! https://stackoverflow.com/questions/56605578/how-can-i-get-eslint-plugin-react-hooks-to-lint-functional-components-that-are

I'll just add names to my default exports then. :)

stale[bot] commented 4 years ago

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[bot] commented 4 years ago

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!

rickhanlonii commented 5 months ago

Here's a PR with tests for these cases and others https://github.com/facebook/react/pull/28147

Related issues:

upsuper commented 3 months ago

Two more cases I found:

Component returned from a factory:

function createView() {
  return () => {
    if (Math.random() > 0.5) {
      return null;
    }
    React.useEffect(() => {}, []); // should error but doesn't
    return <div/>;
}

Functional component as a field of class:

class Foo {
  private readonly View = () => {
    if (Math.random() > 0.5) {
      return null;
    }
    React.useEffect(() => {}, []); // should error but doesn't
    return <div />;
  };
}