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(typescript-estree): split TSMappedType typeParameter into constraint and key #7065

Conversation

JoshuaKGoldberg
Copy link
Member

BREAKING CHANGE:
Deprecates an existing property on the AST

PR Checklist

Overview

Per the issue, deprecates the typeParameter property in favor of constraint and key properties.

@typescript-eslint
Copy link
Contributor

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.

@netlify
Copy link

netlify bot commented May 25, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 933e00a
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/64a835d4c886bc0008006de9
😎 Deploy Preview https://deploy-preview-7065--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0 milestone May 25, 2023
@nx-cloud
Copy link

nx-cloud bot commented May 25, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 933e00a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #7065 (933e00a) into v6 (c8bb00d) will increase coverage by 0.02%.
The diff coverage is 83.33%.

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     
Flag Coverage Δ
unittest 87.53% <83.33%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/visitor-keys/src/visitor-keys.ts 100.00% <ø> (ø)
packages/typescript-estree/src/convert.ts 30.60% <75.00%> (+0.57%) ⬆️
packages/eslint-plugin/src/rules/indent.ts 88.09% <100.00%> (ø)
...s/eslint-plugin/src/util/collectUnusedVariables.ts 93.16% <100.00%> (ø)
...ckages/scope-manager/src/referencer/TypeVisitor.ts 98.03% <100.00%> (+0.05%) ⬆️

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft May 25, 2023 15:36
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review May 25, 2023 18:01
Copy link
Member

@bradzacher bradzacher left a 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.

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);
}

@JoshuaKGoldberg
Copy link
Member Author

We didn't make it in time. I'll move this to v7.

@JoshuaKGoldberg JoshuaKGoldberg modified the milestones: 6.0.0, 7.0.0 Jul 10, 2023
@JoshuaKGoldberg JoshuaKGoldberg deleted the branch typescript-eslint:v8 July 10, 2023 17:52
@JoshuaKGoldberg
Copy link
Member Author

This was unintentionally auto-closed when we merged the v6 branch 🙃 it'll be recreated.

@JoshuaKGoldberg
Copy link
Member Author

Or, actually, since this needs to wait till v7 anyway - I'll just leave it closed.

@JoshuaKGoldberg JoshuaKGoldberg removed this from the 7.0.0 milestone Jul 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2023
@JoshuaKGoldberg JoshuaKGoldberg changed the base branch from v6 to v8 April 14, 2024 22:45
@JoshuaKGoldberg JoshuaKGoldberg removed the request for review from bradzacher April 14, 2024 22:51
@@ -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
Copy link
Member Author

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.

@bradzacher bradzacher added the breaking change This change will require a new major version to be released label Apr 15, 2024
Copy link
Member

@bradzacher bradzacher left a 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.

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Apr 15, 2024
@bradzacher bradzacher added this to the 8.0.0 milestone Apr 15, 2024
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 15, 2024
@JoshuaKGoldberg JoshuaKGoldberg merged commit 6548240 into typescript-eslint:v8 Apr 19, 2024
53 checks passed
@JoshuaKGoldberg JoshuaKGoldberg deleted the ts-mapped-type-constraint-and-key branch April 19, 2024 15:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change This change will require a new major version to be released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants