feat: update eslint config in React templates

[ArnaudBarre]

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]

Run & review this pull request in StackBlitz Codeflow.

[patak-dev]

@nickmccurdy would you help us with a review here?

[nickmccurdy]

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

[nickmccurdy]

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?

[ArnaudBarre]

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.)

[nickmccurdy]

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.

[patak-dev]

@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.

[nickmccurdy]

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.

[patak-dev]

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

[nickmccurdy]

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

[essenmitsosse]

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]

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]

@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]

@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]

@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]

@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]

@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