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

Add eslint with react-hooks plugin #371

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add eslint with react-hooks plugin #371

wants to merge 1 commit into from

Conversation

steida
Copy link

@steida steida commented Jun 9, 2019

react-hooks plugin with the exhaustive check is the must.

Just check how many weird things and errors the code base contains: #370

Try VSCode cmd . to show available fixes. There are some cases where the rule must be ignored, for passed deps for example.

@wardoost
Copy link
Contributor

Great 👏 We should probably remove tslint at the same time?

@steida
Copy link
Author

steida commented Jun 12, 2019

@wardoost Yes, it should be removed. The teams behind tslint and eslint for TypeScript are collaborating.

@streamich
Copy link
Owner

Some thoughts on linter situation (in general, not related to this PR):

  • Remove TSLint.
  • Enable ESLint with similar rules to TSLint, but more relaxed (as current TSLint rules seem too strict).
  • Enable React hook rules in ESLint.
  • Remove Prettier rules from TSLint/ESLint.
  • Run Prettier separately prettier ... on pre-commit hook. (Or probably run pretty-quick instead.)

@steida
Copy link
Author

steida commented Jun 12, 2019

Check https://github.com/este/este/blob/master/.eslintrc.js for inspiration.

@wardoost
Copy link
Contributor

@streamich Wouldn't it be better to integrate Prettier in ESLint to avoid conflicting rules/autofixes? Are there any specific TSLint rules that you think of now that are too strict?

@streamich
Copy link
Owner

streamich commented Jun 13, 2019

@wardoost

... any specific TSLint rules that you think of now that are too strict?

There is one or more rules that require you to sort stuff in alphabetical order, don't remember if it was object keys or imports. Also, there was something else [don't remember now] ...

Why I would like to see Prettier outside of ESLint is so that commit or push never fails because of some code style problem. Prettier can be run any time on the whole project to fix all style "errors", I don't see necessity to fail a commit because of it, or for syntax highlighter to highlight all Prettier things as "errors".

Wouldn't it be better to integrate Prettier in ESLint to avoid conflicting rules/autofixes?

  • Prettier can be run any time, to re-format the whole project.
  • Also it can be run automatically run on every commit, so you don't need to ever reformat the whole project.
  • For TSLint there is tslint-config-prettier plugin that disables all stylistic rules so that TSLint does not conflict with Prettier, ESLint should have something similar.

@wardoost
Copy link
Contributor

Sounds perf 👌I also like the suggested config from @steida as a starting point, minus prettier and relay plugins

@xobotyi xobotyi force-pushed the master branch 22 times, most recently from 8558b58 to 21b01a7 Compare November 3, 2019 04:22
@xobotyi xobotyi force-pushed the master branch 16 times, most recently from 4baf4c2 to 7e57723 Compare November 6, 2019 20:50
@ayush987goyal
Copy link
Contributor

Hi @streamich ,

I have done some local eslint configuration with the default eslint-config-react-app plugin and I am seeing these many errors/warnings:

✖ 96 problems (18 errors, 78 warnings)
  0 errors and 52 warnings potentially fixable with the `--fix` option.

I was thinking of adding eslint-disable to all the affected file and merging that change. Then we have to fix the files one by one and create respective CRs.
I am down to do that but would require contributions along the way.
This would be a major version change since this might potentially break existing functionalities.
So, how should we go about this change?

@streamich
Copy link
Owner

@ayush987goyal I think eslint-config-react-app is a good option, unless there are other propositions.

I was thinking of adding eslint-disable to all the affected file and merging that change. Then we have to fix the files one by one and create respective CRs.

Sounds good to me.

@ayush987goyal ayush987goyal mentioned this pull request Feb 4, 2020
62 tasks
@ayush987goyal
Copy link
Contributor

@streamich Here is #948 to start this.

@ayush987goyal ayush987goyal mentioned this pull request Feb 4, 2020
13 tasks
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

Successfully merging this pull request may close these issues.

4 participants