Proposals for ESLint rules regarding React app #1208
Conversation
|
jinmayamashita
changed the title
| { | ||
| files: ['**/*.{jsx,ts,tsx}'], | ||
| rules: { | ||
| 'react/jsx-pascal-case': 2, |
There was a problem hiding this comment.
I wrote it in the discussion page, but I would like to discuss whether setting the severity to 2 (error) is appropriate.
It may be beneficial, especially for beginners, to enforce best practice, although it may become an annoyance to more experienced developers. Of course, it will vary on the rule, so I'd like to discuss with this in mind.
Regarding naming conventions, it may be enough to set it to 1 (warning).
There was a problem hiding this comment.
@ptrkdan Normal syntax errors such as quotes and semicolons do not have a big impact on the normal working flow. I think these errors should be in the form of warning. For errors that affect the entire team's work flow, such as file name and export method, I think we should set level to error.
There was a problem hiding this comment.
@ptrkdan The main reason for setting rule error is to prevent human oversight during code reviews. (Most of rule error is related to team development guidelines. If the rules are defined to a reasonable extent without being overwhelming, it might eliminate the need to explain them, and violations can be caught by CI failures, relieving reviewers from having to worry about them.)
If there are rules that may become noise for experienced developers, I believe it's possible to discuss and adjust them to rule warning.
| extends: [ | ||
| 'eslint:recommended', | ||
| 'plugin:@typescript-eslint/recommended', | ||
| 'plugin:unicorn/recommended', |
There was a problem hiding this comment.
@jinmayamashita It is so big plugin. Because it has more than 100 eslint rules, I wonder if there are any rules that conflict with the rules from other official plugins 🧐
There was a problem hiding this comment.
@mvn-tiennguyen-hn It is indeed so. I have added rules to ensure code efficiency, avoiding duplication with other rules, and enforcing them as errors rather than warnings regarding team development rules. bcdb969
jinmayamashita
requested review from
G-N555,
mvn-luanvu-hn,
mvn-tiennguyen-hn and
seiya0914
and removed request for
mvn-tiennguyen-hn
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I suggest the following rules to be added
"no-unused-vars": 2,
"no-var": 2,
"prefer-const": 2,
"no-eval": 2,
"no-magic-numbers": 2,
There was a problem hiding this comment.
Thank you for the proposal! I've reviewed rules and it looks good, so I've added it.
It would be helpful if you could discuss with @mvn-luanvu-hn whether to approve the PR after reviewing the final rules.
There was a problem hiding this comment.
@jinmayamashita Thank you for your work! 🙇 I left some comments. Please help to check them.
In addition, we can change to new config system of eslint v9 later: https://eslint.org/blog/2023/10/flat-config-rollout-plans/
| "no-eval": 2, | ||
| "no-magic-numbers": 2, | ||
| "unicorn/filename-case": [2, { case: "kebabCase" }], | ||
| "react/jsx-filename-extension": [2, { extensions: [".tsx"] }], |
There was a problem hiding this comment.
I think we should add the following rule to disable when not import React in the component file
'react/react-in-jsx-scope': "off",
There was a problem hiding this comment.
In addition, we should add React Compiler's ESLint plugin to prepare our code to adapt to React Compiler in the future
https://react.dev/learn/react-compiler#installing-eslint-plugin-react-compiler
There was a problem hiding this comment.
I think we should add the following rule to disable when not import React in the component file
We can add this plugin. I'll check it.
If you are using the new JSX transform from React 17, extend react/jsx-runtime in your eslint config (add "plugin:react/jsx-runtime" to "extends") to disable the relevant rules.
| rules: { | ||
| 'no-console': 2, | ||
| eqeqeq: 2, | ||
| "no-unused-vars": 2, |
There was a problem hiding this comment.
Maybe we should use only single quotes or double quotes.
There was a problem hiding this comment.
I agree with you.
@mvn-luanvu-hn @seiya0914
What’s the difference between double and single quotes?
Can your team decide on the best practice?
|
FYI @mvn-luanvu-hn @seiya0914
|
5 participants
Pull Request Checklist
console.log())npm run test:cli)npm run test:modules)npm run e2e)Description
This is a PR for discussing rules in the codebase. Let's start by evaluating whether the ESLint rules are appropriate. (Considering if anything has been overlooked or if there's overkill, etc.)
Feel free to also discuss Biome mentioned in #1201 (comment)
FYI @mvn-huynguyen-hn @mvn-tiennguyen-hn
Context
#1201 (comment)