-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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(typescript-estree): split TSMappedType typeParameter into constraint and key #7065
feat(typescript-estree): split TSMappedType typeParameter into constraint and key #7065
Conversation
Thanks for the PR, @JoshuaKGoldberg! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## v6 #7065 +/- ##
==========================================
+ Coverage 87.51% 87.53% +0.02%
==========================================
Files 375 375
Lines 13148 13153 +5
Branches 3894 3894
==========================================
+ Hits 11506 11514 +8
+ Misses 1262 1256 -6
- Partials 380 383 +3
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.
this change introduces a bug into the scope analysis because previously it used to implicitly rely on the fact that typeParameter
was a TSTypeParemeter
- i.e. declared a type variable.
So now we'll need to update our visitor so that we declare the key as a type variable, then visit the constraint, then the name type, then the type annotation.
typescript-eslint/packages/scope-manager/src/referencer/TypeVisitor.ts
Lines 198 to 203 in 2f46341
protected TSMappedType(node: TSESTree.TSMappedType): void { | |
// mapped types key can only be referenced within their return value | |
this.#referencer.scopeManager.nestMappedTypeScope(node); | |
this.visitChildren(node); | |
this.#referencer.close(node); | |
} |
packages/scope-manager/tests/fixtures/type-declaration/mapped-named.ts.shot
Outdated
Show resolved
Hide resolved
packages/scope-manager/tests/fixtures/type-declaration/mapped-named.ts.shot
Outdated
Show resolved
Hide resolved
packages/scope-manager/tests/fixtures/type-declaration/mapped.ts.shot
Outdated
Show resolved
Hide resolved
packages/scope-manager/tests/fixtures/type-declaration/mapped-named.ts.shot
Show resolved
Hide resolved
We didn't make it in time. I'll move this to v7. |
This was unintentionally auto-closed when we merged the |
Or, actually, since this needs to wait till v7 anyway - I'll just leave it closed. |
@@ -382,6 +382,7 @@ export default createRule<Options, MessageIds>({ | |||
TSMappedType(node: TSESTree.TSMappedType) { | |||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |||
const squareBracketStart = context.sourceCode.getTokenBefore( | |||
// eslint-disable-next-line deprecation/deprecation |
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.
Since this rule is getting removed in #8074 anyway.
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.
overall looks good - one definite bug to fix though.
packages/scope-manager/tests/fixtures/type-declaration/mapped-named.ts.shot
Show resolved
Hide resolved
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
BREAKING CHANGE:
Deprecates an existing property on the AST
PR Checklist
TSMappedType
#6433Overview
Per the issue, deprecates the
typeParameter
property in favor ofconstraint
andkey
properties.