Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
karangarg45 opened this issue Feb 22, 2019 · 7 comments

Comments

@karangarg45
Copy link

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

@Gelio
Copy link
Owner

Gelio commented Mar 6, 2019

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
Copy link

DavidBabel commented Mar 20, 2019

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
Copy link

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"
  }
}
  1. Now you can execute npx eslint . --ext .ts,.tsx,.js,.jsx to run the linter. Add that to your npm scripts.

  2. 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
Copy link

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
Copy link
Owner

Gelio commented May 21, 2019

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

@JemarJones
Copy link

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
Copy link
Owner

Gelio commented Nov 6, 2019

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

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 🙁

If you really miss the exhaustive deps checking, try migrating to linting TS files using ESLint: #9 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants