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

feat: update eslint config in React templates #13550

Merged
merged 3 commits into from
Jun 29, 2023
Merged

Conversation

ArnaudBarre
Copy link
Member

Closes #10772

I wanted to improve a bit the ling rules for the TS template, adding more type aware rules. The strict set is too strict for a starter, but I added manually my preferred one: @typescript-eslint/no-unnecessary-condition (catches so much dead branch when refactoring). Also added one to remove this as I wanted to remove since way to long (see #10772)

Also discovered that no-non-null-assertion is enabled by default, which is bad IMO. I don't agree with the opinion of the TS-ESLint team. TS will never be a sound type system (can't find the issue/discussion from the core team that says it's out of scope). This tool is a sharp knife, but a useful one, we use ourself quite a lot in the Vite codebase. And the diff in the TS template clearly shows that this is a better assertion than a as.

@stackblitz
Copy link

stackblitz bot commented Jun 18, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@ArnaudBarre ArnaudBarre marked this pull request as ready for review June 18, 2023 19:59
@patak-dev
Copy link
Member

@nickmccurdy would you help us with a review here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the new ESLint config and existing TS config still work in monorepos?

'react-refresh/only-export-components': 'warn',
'react-refresh/only-export-components': [
'warn',
{ allowConstantExport: true },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs mention that Vite supports this option, but what if inside this Vite project are files run by another HMR implementation that doesn't support it?

Copy link
Member Author

@ArnaudBarre ArnaudBarre Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this complex setup is out of scope of a Vite template. I don't consider this config to be perfect for every use cases, just a good starting for people using Vite in place of CRA.
(Idem for monorepo stuff, we people have already their linting configure they can delete the file.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying this template should set up other tools, I'm saying it should avoid breaking other tools when we can't make assumptions about what they are. I think this is actually more important with a minimal template, as we don't include common tools like doc generators or test frameworks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickmccurdy do you have a common example of vite + a tool that will break after adding this config? I agree with the idea you're pushing here, but I don't know if it would apply to this particular option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of any specific counterexamples, sorry. Hopefully the docs for allowConstantExport help:

Don't warn when a constant (string, number, boolean, templateLiteral) is exported aside one or more components.

This should be enabled if the fast refresh implementation correctly handles this case (HMR when the constant doesn't change, propagate update to importers when the constant changes.). Vite supports it, PR welcome if you notice other integrations works well.

bluwy
bluwy previously approved these changes Jun 29, 2023
@patak-dev
Copy link
Member

Let's move forward to get more feedback from users. We can keep the discusion open.

@patak-dev patak-dev merged commit 6fe1491 into main Jun 29, 2023
@patak-dev patak-dev deleted the update-react-template branch June 29, 2023 21:40
@nickserv
Copy link
Contributor

Sounds good! 👍 I'll still check these notifications.

xinxinhe1810 pushed a commit to xinxinhe1810/vite that referenced this pull request Jul 4, 2023
@essenmitsosse
Copy link

I just scaffolded a project and was a bit confused, by only two rules being overwritten, one of them turning of @typescript-eslint/no-unnecessary-condition. I understand that one might find this rule too strict, but that seems like a very personal preference to me. First thing I did was remove the line (to turn on the rule).

My opinion would be that a starter template should rely on the tools its using (i.e. plugin:@typescript-eslint) and shouldn't add unnecessary noice on top. To a new user, having this one rule explicitly turned of would look very important and they might assume it is necessary for a React/TS/Vite template, when in fact it has nothing to do with a template, and is just a personal preference.

@xixixao
Copy link

xixixao commented Jul 7, 2023

We are in the process of upgrading our quickstart to the latest version of Vite and found that the addition of 'plugin:@typescript-eslint/recommended-requiring-type-checking' has two issues:

  1. It requires a much stricter adherence to types than before. This can be cumbersome for prototyping phases with more liberal use of any and for using less-than-ideally typed libraries.
  2. More importantly, ESLint doesn't pick up changes to other files, and seems to be very eagerly caching lint results (at least in VS Code). This means that while TypeScript has already picked up new types, ESLint is still showing a Lint warning, which can be quite frustrating.

I'd leave the addition of this ruleset to users who are TypeScript experts who want the additional type safety.

@ArnaudBarre
Copy link
Member Author

@essenmitsosse I don't understand, the only rule turn off is no-non-null-assertion. While I agree we should not change too much thing from the default, this one is harmful as it was suggesting people that as HTMLElement is better than ! which is not the case. This rule will be moved to strict in the next version of TS-ESLint: typescript-eslint/typescript-eslint#6014

@ArnaudBarre
Copy link
Member Author

@xixixao Thanks for the feedback, that's true that the cache issues with types in ESLint can be annoying.

I wanted to promote a bit this kind of more advanced rules, but the usage of Vite templates is more wide than the start of a new app. I will revisit this to see how we can keep an eslint config for hooks without giving the impression that this is the right config for a team building a production application (I've seen to many teams sticking with the default rules of CRA that are lacking support for TS)

@essenmitsosse
Copy link

@ArnaudBarre I am not arguing for cast as HTMLElement (I think it's just as harmful as !). My personal opinion is to use neither us as nor ! and rather have an additional type check, even though it might never be needed (but avoid normalizing casting in the code base). But I also didn't want to argue about my personal preference on this rule but rather point out that it is still a personal preference (if this matter would really be this clear cut, the discussion could be opened with @typescript-eslint and would be resolved there). And in this case, I was arguing for keeping things simple by just leaving them as they come from upstream (i.e., don't overwrite the rule).

TLDR; I was confused by this single rule being overwritten. Had to spend time figuring out why this (from my perspective) reasonable rule was turned off (and assumed it was because of some problem with React). Went ahead and turned it back on.

@essenmitsosse
Copy link

@xixixao I see your point, but I was happy all the setup for ESLint with types was already done. Turning the rules off seems a lot simpler, in this case, than turning them on. It would be a pity to have it removed.

@ArnaudBarre
Copy link
Member Author

@essenmitsosse good news v6 was just so I will update the template and remove this line while keeping the !
In that exact case as an example, adding a custom check will not make the error more easy to debug (error that is instantly detectable if it happens).

Yeah this is not trivial to setup, but the question is should it be the role of the default template to do so. I will discuss this with the team

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.

6 participants