Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(eslint-plugin): [no-shadow] ignore ordering of type declarations#10593
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
fix(eslint-plugin): [no-shadow] ignore ordering of type declarations#10593
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@ronami! 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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedJan 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
nx-cloudbot commentedJan 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
View yourCI Pipeline Execution ↗ for commit7dd6b06.
☁️Nx Cloud last updated this comment at |
| { | ||
| code: ` | ||
| let x = foo((x, y) => {}); | ||
| let y; | ||
| `, | ||
| errors: [ | ||
| { | ||
| data: { | ||
| name: 'x', | ||
| shadowedColumn: 5, | ||
| shadowedLine: 2, | ||
| }, | ||
| messageId: 'noShadow', | ||
| type: AST_NODE_TYPES.Identifier, | ||
| }, | ||
| ], | ||
| languageOptions: { parserOptions: { ecmaVersion: 6 } }, | ||
| options: [{ hoist: 'functions' }], | ||
| }, |
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 isn't directly related to the PR, but it seemed there are very few tests that cover thefunctions option inhoist (which now has two extra options).
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.
👍👍👍 on adding in missing test coverage! Especially if it's at all related to a rule being changed.
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.
🚀
| @@ -63,7 +70,7 @@ export default createRule<Options, MessageIds>({ | |||
| type: 'string', | |||
| description: | |||
| 'Whether to report shadowing before outer functions or variables are defined.', | |||
| enum: ['all', 'functions', 'never'], | |||
| enum: ['all', 'functions', 'functions-and-types', 'never', 'types'], | |||
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.
I've expanded the existing
hoistconfiguration to includetypesandfunctions-and-types. I don't think this is optional, as permutations need to be manually set ...
Yeah, I'm thinking this exposes a general learning: don't useenum option types for"when do I X?" questions. Eventually folks will want to do something that could be better expressed as an object. I.e. here:
{optionName:{functions:true,types:true,variables:true}}
...but, to your point, I think it would be even more disruptive to extend the base rule option to a wholly different format in that way. For now I think theenum is fine. If this rule gets even more complicated over time then we can always switch to the object later.
tl;dr: agreed as-is 👍
| { | ||
| code: ` | ||
| let x = foo((x, y) => {}); | ||
| let y; | ||
| `, | ||
| errors: [ | ||
| { | ||
| data: { | ||
| name: 'x', | ||
| shadowedColumn: 5, | ||
| shadowedLine: 2, | ||
| }, | ||
| messageId: 'noShadow', | ||
| type: AST_NODE_TYPES.Identifier, | ||
| }, | ||
| ], | ||
| languageOptions: { parserOptions: { ecmaVersion: 6 } }, | ||
| options: [{ hoist: 'functions' }], | ||
| }, |
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.
👍👍👍 on adding in missing test coverage! Especially if it's at all related to a rule being changed.
| if (!outerDef) { | ||
| return true; | ||
| } |
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.
I went over this again and made one small change. I think I've introduced an unnecessary change to the original implementation (returningfalse instead oftrue ifouterDef isfalse, see7dd6b06).
I don't think this code is reachable (all tests seem to pass regardless of this change), and I couldn't reproduce this in a test. Still, just to be sure, I've changed this to match the original implementation.
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.
Now that it's on its ownif statement, it fails codecov (even though this wasn't covered previously but was in a single long binary operation). I'm a bit unsure what the best way to tackle this is. I'd normally usenullThrows or a type assertion, but I'm not entirely sure this really is an unreachable line.
This part of the code seems to originate fromthe original eslint rule andis there from the very beginning.
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.
Yeah I'd just ignore Codecov here. Its reporter isn't perfect.
codecovbot commentedJan 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #10593 +/- ##==========================================+ Coverage 86.90% 86.94% +0.04%========================================== Files 446 446 Lines 15502 15520 +18 Branches 4516 4523 +7 ==========================================+ Hits 13472 13494 +22+ Misses 1675 1671 -4 Partials 355 355
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.
👍 from me. Even if theouterDef issue is a bug, I suspect it'd be an existing one - and probably not as impactful as the root issue this PR is fixing. Good note!
63135f7 intotypescript-eslint:mainUh oh!
There was an error while loading.Please reload this page.
##### [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13)##### 🚀 Features- **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565))- **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585))- **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551))##### 🩹 Fixes- **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602))- **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593))##### ❤️ Thank You- Josh Goldberg ✨- Ronen Amiel- YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
##### [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13)##### 🚀 Features- **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565))- **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585))- **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551))##### 🩹 Fixes- **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602))- **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593))##### ❤️ Thank You- Josh Goldberg ✨- Ronen Amiel- YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
##### [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13)##### 🚀 Features- **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565))- **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585))- **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551))##### 🩹 Fixes- **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602))- **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593))##### ❤️ Thank You- Josh Goldberg ✨- Ronen Amiel- YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
##### [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13)##### 🚀 Features- **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565))- **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585))- **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551))##### 🩹 Fixes- **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602))- **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593))##### ❤️ Thank You- Josh Goldberg ✨- Ronen Amiel- YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
| datasource | package | from | to || ---------- | -------------------------------- | ------ | ------ || npm | @typescript-eslint/eslint-plugin | 8.19.1 | 8.20.0 || npm | @typescript-eslint/parser | 8.19.1 | 8.20.0 |## [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13)##### 🚀 Features- **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565))- **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585))- **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551))##### 🩹 Fixes- **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602))- **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593))##### ❤️ Thank You- Josh Goldberg ✨- Ronen Amiel- YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
| datasource | package | from | to || ---------- | -------------------------------- | ------ | ------ || npm | @typescript-eslint/eslint-plugin | 8.19.1 | 8.20.0 || npm | @typescript-eslint/parser | 8.19.1 | 8.20.0 |## [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13)##### 🚀 Features- **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565))- **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585))- **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551))##### 🩹 Fixes- **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602))- **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593))##### ❤️ Thank You- Josh Goldberg ✨- Ronen Amiel- YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
Uh oh!
There was an error while loading.Please reload this page.
PR Checklist
Overview
This PR attempts to tackle#5935.
Some thoughts/questions:
I've expanded the existing
hoistconfiguration to includetypesandfunctions-and-types. I don't think this is optimal, as permutations need to be manually set (e.g.functions-and-types). It also adds some complexity to this configuration, but I'm not sure if there's a non-breaking alternative.Since this is an extension rule, this somewhat changes thebase rule's option, though I did see other rules like
no-empty-functionwhich extend an enum type of their base rule.That said, I'm not sure if someone will ever want to configure this to just
functionsor justtypes. It may be better to have the currentfunctionsoptions includetypesbehavior and rename it in a future major (although this would mean the option would diverge from the base rule).I'm a bit unsure about this, so I'll be happy to hear other opinions.
I've changed the default from
functionstofunctions-and-types. This seems like the most sensible option, as for both, ordering doesn't matter.I've included documentation for the added option keys, similar to how I saw this done on other extension rules. I wasn't sure whether to use the correct/incorrect
<Tabs />or not. I have decided not to use<Tabs />to match the rest of the rule's documentation. Please let me know if it would be better to use<Tabs />regardless.