Gelio / tslint-react-hooks

TSLint rule for detecting invalid uses of React Hooks
MIT License
218 stars 10 forks source link

[Feature Request] Exhaustive deps lint rule integration in tslint react hook #9

Open karangarg45 opened 5 years ago

karangarg45 commented 5 years ago

Recently Dan published the new rule for react hooks...is it possible to add that feature in the tslint hook? https://github.com/facebook/react/issues/14920

Gelio commented 5 years ago

Thank you for posting this feature request. I will try to investigate the effort required to add this one into this rule 😄 Analyzing eslint's rule implementation would probably be a good starting point. There are, however, some differences when it comes to the internals that eslint and tslint expose to rule walkers (classes that explore the AST and find violations), so that may not be trivial 😄

If anyone has any ideas on this one - let me know, or, even better, file a PR yourself

DavidBabel commented 5 years ago

I came here to ask to exact same request. It's actually a must have feature for this plugin, but i can easily understand that's harder than it looks.

Their are some hard thing to manage ...

function SearchResults() {
  const [query, setQuery] = useState('react');

  // Imagine this function is also long
  function getFetchUrl() {
    return 'https://hn.algolia.com/api/v1/search?query=' + query;
  }

  // Imagine this function is also long
  async function fetchData() {
    const result = await axios(getFetchUrl());
    setData(result.data);
  }

  useEffect(() => {
    fetchData();
  }, []); // error, `query` should be there

  // ...
}
antonpetkoff commented 5 years ago

First, @Gelio, thank you for your efforts to port Rules of Hooks to tslint!

Surely, the exhaustive-deps rule is a must-have feature!

We are in a transitioning period from tslint to eslint and sooner or later we have to migrate to eslint. I think it is acceptable to have both tslint and eslint temporarily, until we migrate.

I decided to add eslint and use the original eslint-plugin-react-hooks. Note that I have tslint configured with a lot of custom rules and plugins, which aren't yet ported to eslint, so I still use them, but I use only Rules of Hooks from eslint and nothing else.

Here's what I did:

1) npm install --save-dev eslint @typescript-eslint/parser eslint-plugin-react-hooks

2) I configured my .eslintrc:

{
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "useJSXTextNode": true,
    "ecmaVersion": 6,
    "sourceType": "module",
    "ecmaFeatures": {
      "modules": true,
      "jsx": true
    }
  },
  "plugins": ["react-hooks"],
  "rules": {
    "react-hooks/rules-of-hooks": "error",
    "react-hooks/exhaustive-deps": "warn"
  }
}

3) Now you can execute npx eslint . --ext .ts,.tsx,.js,.jsx to run the linter. Add that to your npm scripts.

4) You can also configure your editor to autofix your eslint errors/warning on save. For VSCode you can add the ESLint extension with ID dbaeumer.vscode-eslint and see its documentation how to configure autofixing.

References: This article helped me with the setup.

alicerocheman commented 5 years ago

Hi, The rule is working fine, but I'm having trouble making it error instead of warn.

tslint.json:

{
  "extends": [
    "tslint:recommended",
    "tslint-react-hooks"
  ],
  ...
 "react-hooks-nesting": "error",
 "react-hooks/exhaustive-deps": "error",
  ...
}

Any idea how to do this?

Thanks

Gelio commented 5 years ago

@alicerocheman Are you using TSLint from inside ESLint? Could you create a separate issue and describe your problem there? :slightly_smiling_face:

JemarJones commented 5 years ago

Any update on this? I find this to be the most useful feature of the eslint version! I know it's not a trivial task as the eslint implementation looks like it's about 1500 lines.

Gelio commented 5 years ago

@JemarJones Unfortunately, no progress so far. Moreover, I do not plan to add it in the near future :slightly_frowning_face:

The problem is not only with the fact that the ESLint's version is long. ESLint exposes different features for rule validators which TSLint does not. If I'm not mistaken, one of those features is scope analysis. TSLint does not provide that. Thus, to analyze from which scope a variable comes we would have to implement it directly in the rule, which adds a lot of engineering work and has a toll on the performance :slightly_frowning_face:

If you really miss the exhaustive deps checking, try migrating to linting TS files using ESLint: https://github.com/Gelio/tslint-react-hooks/issues/9#issuecomment-475776920