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
fix(eslint-plugin): [no-unnecessary-type-constraint] correctly fix in cts/mts files #6795
fix(eslint-plugin): [no-unnecessary-type-constraint] correctly fix in cts/mts files #6795
Conversation
Thanks for the PR, @WoodyWoodsta! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
I've specifically left out unit tests for this, as it doesn't appear that any of the fixtures for |
I've noticed two things:
|
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@bradzacher I'm trying to understand why the proposition you made which was simply to use the Regardless, there was no logic in the rule which checked for jsx/js, only the fix method. So I've added a new util to serve as a bypass for this rule. I have no idea if this is the correct approach: to me it doesn't seem elegant. |
packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts
Outdated
Show resolved
Hide resolved
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6795 +/- ##
=======================================
Coverage 87.47% 87.47%
=======================================
Files 379 379
Lines 13228 13234 +6
Branches 3905 3906 +1
=======================================
+ Hits 11571 11577 +6
Misses 1279 1279
Partials 378 378
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies - it appears that I've lead you astray due to my misunderstanding of TS.
I've never actually used .mts
for JSX, but I knew it errored on JSX-ambiguous syntax, so I had assumed it actually supported JSX.
In testing I now see that this isn't actually the case - .mts
/.cts
doesn't support JSX, it just errors on ambiguous syntax, and does not actually support JSX in the files. I assume that this just the TS team future-proofing - reading the design meeting notes it appears like they didn't want to lock in one-way or the other for the first release, so they just made sure they could make it JSX later if they wanted to by banning ambiguous syntax.
This obviously the reason why the getScriptKind
function returns "TS" as the kind for the file instead of "TSX" - which was another thing I hadn't noticed in passing earlier.
Which provides a problem as you've noticed our suggested solution obviously won't fix it.
So what would be a good solution?
Now that I know what we know - I think we should just go back to hard-coding this. Something like this would do the job:
function requiresGenericDeclarationDisambiguation(filename: string) {
const ext = path.ext(filename).toLowerCase();
switch (ext) {
case ts.Extension.Tsx:
case ts.Extension.Cts:
case ts.Extension.Mts:
return true;
default:
return false;
}
}
packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts
Outdated
Show resolved
Hide resolved
@bradzacher To sum up your comments and relate to the issue: Original Issue
Fix
No changes to eslint fix required. Let me know if this sounds correct. Of course this could also be resolved in some way in formatters (i.e. to not strip the dangling comma here), but:
|
If prettier is stripping the syntactically necessary comma when formatting a It has already been reported: prettier/prettier#14518
Formatters are powered by the AST - they know exactly and unambiguously that The formatter then decides how to print the code from the AST - if it decides to prints the type parameter on a single line, its trivial to also insert the comma. |
This is not correct, no. The rule should always report on the unnecessary The current rule behaviour is to always report - but the erroneous behaviour is that it removes it in |
@bradzacher Ok you confirm what I thought. So if I understand correctly, there is actually nothing to do here in |
My last comment describes the change we need to make to the rule - we need to fix to |
@bradzacher Hopefully I've now understood. Please see 6bfccc6 |
@bradzacher Any thoughts on the above? |
packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts
Outdated
Show resolved
Hide resolved
@bradzacher is this still something you have time to review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this LGTM!
PR Checklist
Overview
.mts
,cts
as well as jsx variant files