Note: I've labelled this as afeat on purpose to mark it for a minor bump. It's technically a "bug fix" but it's also a change in behaviour. One might classify it as a breaking change because a specific usecase will require a config change - but given that usecases is also already broken, I didn't think that it truly qualified as breaking.

This PR implements a sledge-hammer approach to fixing the problem in#5468 - I have opted to just completely ignore a file if it passes all three of the following conditions:

there is a decorator in the file
experimentalDecorators: true
emitDecoratorMetadata: true

Originally in#5468 I suggested going with the the approach of specifically ignoring imports that are used in locations that might be passed to decorator metadata. However I ultimately chose not to go down this path because:

As of TS 5.0experimentalDecorators is considered legacy
The stable TC39 decorator metadata proposal does not include type annotations
verbatimModuleSyntax exists for the above usecase
The logic required to concretely detect if a specifier is used within decorator metadata is complex and a maintenance burden and only specifically applies toexperimentalDecorators

This PR:

Removes the decorator metadata detection logic fromscope-manager
Adds a newexperimentalDecorators parser option that is auto-populated from type info if available
Exposes bothexperimentalDecorators andemitDecoratorMetadata on the parser services
- I did this to make it easy for rules to derive this state without having to manually interact with the program to extract the compiler options.
Updatesconsistent-type-imports:
- so that it bails out of reporting on a file if the aforementioned 3 conditions are met.
- removes the error messages relating to decorator metatadata
Refactors the tests to validate the conditions are correctly applied.

NOTE:: I strongly suggest turning on the no-whitespace diff when reviewing this PR.

bradzacher added the enhancementNew feature or request label

Feb 1, 2024

Copy link

Contributor

typescript-eslintbot commentedFeb 1, 2024

Thanks for the PR,@bradzacher!

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.

Copy link

netlifybot commentedFeb 1, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`5749a57`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6600b24e92a82200086d6b88
😎 Deploy Preview	https://deploy-preview-8335--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 98 (no change from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

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

feat(eslint-plugin): [consistent-type-imports] ignore files with deco…

1c1f9f4

…rators, experimentalDecorators, and emitDecoratorMetadata

bradzacher force-pushed the5468-CTI-ignore-exp-deco-metadata branch fromf3525d4 to1c1f9f4Compare

February 1, 2024 13:31

Copy link

codecovbot commentedFeb 1, 2024•
edited
Loading

Codecov Report

Attention: Patch coverage is92.38095% with8 lines in your changes are missing coverage. Please review.

Project coverage is 87.98%. Comparing base(4132374) to head(5749a57).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@##             main    #8335      +/-   ##==========================================+ Coverage   87.38%   87.98%   +0.59%==========================================  Files         254      404     +150       Lines       12489    14045    +1556       Branches     3919     4110     +191     ==========================================+ Hits        10914    12358    +1444- Misses       1304     1382      +78- Partials      271      305      +34

Flag	Coverage Δ
unittest	`87.98% <92.38%> (+0.59%)`	⬆️

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

Files	Coverage Δ
packages/scope-manager/src/ScopeManager.ts	`77.64% <ø> (ø)`
packages/scope-manager/src/analyze.ts	`61.11% <ø> (ø)`
...kages/scope-manager/src/referencer/ClassVisitor.ts	`78.78% <100.00%> (ø)`
...ackages/scope-manager/src/referencer/Referencer.ts	`95.52% <100.00%> (ø)`
...ages/typescript-estree/src/createParserServices.ts	`60.00% <50.00%> (-11.43%)`	⬇️
...eslint-plugin/src/rules/consistent-type-imports.ts	`94.77% <93.54%> (-1.21%)`	⬇️

... and146 files with indirect coverage changes

bradzacher requested a review fromJoshuaKGoldberg

February 2, 2024 00:04

JoshuaKGoldberg reviewed

Feb 3, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-imports.md OutdatedShow resolvedHide resolved

JoshuaKGoldberg reviewed

Feb 3, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Really nice solution, +1 from me on the direction!

Requesting changes mainly on extracting the deeper explanations out of the rule file and into a blog post. Also for at least resolving the breaking change discussion.

But the rule's new implementation looks great to me (thanks for the whitespace suggestion!). ✨

packages/eslint-plugin/docs/rules/consistent-type-imports.md OutdatedShow resolvedHide resolved

packages/eslint-plugin/src/rules/consistent-type-imports.ts OutdatedShow resolvedHide resolved

packages/parser/src/parser.tsShow resolvedHide resolved

packages/scope-manager/src/analyze.ts OutdatedShow resolvedHide resolved

packages/eslint-plugin/src/rules/consistent-type-imports.tsShow resolvedHide resolved

docs/packages/Parser.mdx OutdatedShow resolvedHide resolved

packages/scope-manager/src/analyze.ts OutdatedShow resolvedHide resolved

packages/scope-manager/src/analyze.tsShow resolvedHide resolved

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Feb 3, 2024

This was referencedFeb 3, 2024

Bug: [consistent-type-imports] Attempts to fix the wrong import statement#7209

Closed

fix(eslint-plugin): [consistent-type-imports] handle imports without specifiers#8308

Merged

Copy link

ZackMFleischman commentedFeb 9, 2024

@bradzacher Thanks for building this solution! I'm eagerly awaiting it being merged so I can use it in my project where I've been forced to just turn this rule off entirely for the moment.

Also hi@JoshuaKGoldberg! Always fun to see your name pop up. Thanks for fighting the good fight and keeping high-quality open source software available! 😄

Copy link

Member

JoshuaKGoldberg commentedFeb 10, 2024

Haha hey@ZackMFleischman, great to see you pop up too! 😄

bradzacher added2 commits

March 18, 2024 21:24

Merge branch 'main' into 5468-CTI-ignore-exp-deco-metadata

01e2d4a

fixup after merge

d786f7c

Copy link

nx-cloudbot commentedMar 18, 2024•
edited
Loading

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 31 targets

Sent with 💌 fromNxCloud.

bradzacher added2 commits

March 18, 2024 23:54

updoots pt 1

3673cd1

updoots pt 2

4d28e81

bradzacher requested a review fromJoshuaKGoldberg

March 18, 2024 13:43

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Mar 18, 2024

docs

4f0d04e

JoshuaKGoldberg requested changes

Mar 18, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Awesome! I'm super happy with the implementation, and think I understand this much better now thanks to the docs & comments. Thanks!

Left some proofreading thoughts on the blog post. That's the only real blocker from me - and there's nothing I likely can't be convinced otherwise on.

🔥

docs/packages/Parser.mdxShow resolvedHide resolved

packages/scope-manager/src/analyze.tsShow resolvedHide resolved

packages/typescript-estree/src/createParserServices.tsShow resolvedHide resolved

packages/website/blog/2024-03-25-changes-to-consistent-type-imports-with-decorators.md OutdatedShow resolvedHide resolved

packages/website/blog/2024-03-25-changes-to-consistent-type-imports-with-decorators.mdShow resolvedHide resolved

packages/website/blog/2024-03-25-changes-to-consistent-type-imports-with-decorators.md OutdatedShow resolvedHide resolved

JoshuaKGoldberg reviewed

Mar 18, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-imports.mdxShow resolvedHide resolved

JoshuaKGoldberg reviewed

Mar 18, 2024

View reviewed changes

packages/website/blog/2024-03-25-changes-to-consistent-type-imports-with-decorators.mdShow resolvedHide resolved

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Mar 18, 2024

bradzacher added3 commits

March 24, 2024 14:31

review

1ec72ce

fix broken link

69fbd1b

Merge branch 'main' into 5468-CTI-ignore-exp-deco-metadata

50a0f5c

bradzacher requested a review fromJoshuaKGoldberg

March 24, 2024 04:03

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Mar 24, 2024

JoshuaKGoldberg previously approved these changes

Mar 24, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Whoop, this is great - useful userland fixing and really solid explanations@bradzacher! 👏

packages/website/blog/2024-03-25-changes-to-consistent-type-imports-with-decorators.md Outdated


		<!-- prettier-ignore-->
		```ts
		var _a;

Copy link

Member

JoshuaKGoldbergMar 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nit: this isn't used later.

Suggested change

var _a;

If TypeScript is producing an_a despite never using it, then that's a bug we can ignore.

Copy link

MemberAuthor

bradzacherMar 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oh nah it was there cos the code has no types so this is an extra case I (purposely) didn't mention. In this case when TS can't determine the type but it can see its a value-import - instead TS emits[typeof _a = (typeof Foo !== "undefined" && Foo) === "function" ? _a : Object]. Which is some funky code-golf heh.

Can remove it though yeah as it's not necessary for the examples I'm mentioning.

Update 2024-03-25-changes-to-consistent-type-imports-with-decorators.md

e85e919

bradzacher dismissedJoshuaKGoldberg’sstale review viae85e919

March 24, 2024 23:05

Update 2024-03-25-changes-to-consistent-type-imports-with-decorators.md

5749a57

bradzacher merged commite408b93 intomain

Mar 24, 2024

bradzacher deleted the 5468-CTI-ignore-exp-deco-metadata branch

March 24, 2024 23:35

peanutenthusiast pushed a commit to peanutenthusiast/typescript-eslint that referenced this pull request

Mar 27, 2024

feat(eslint-plugin): [consistent-type-imports] ignore files with deco…

7c805f4

…rators, experimentalDecorators, and emitDecoratorMetadata (typescript-eslint#8335)

yeonjuan pushed a commit to yeonjuan/typescript-eslint that referenced this pull request

Mar 31, 2024

feat(eslint-plugin): [consistent-type-imports] ignore files with deco…

d3cdf95

…rators, experimentalDecorators, and emitDecoratorMetadata (typescript-eslint#8335)

github-actionsbot locked asresolvedand limited conversation to collaborators

Apr 4, 2024

Labels

enhancement

New feature or request

Movatterモバイル変換

Uh oh!

feat(eslint-plugin): [consistent-type-imports] ignore files with decorators, experimentalDecorators, and emitDecoratorMetadata#8335

feat(eslint-plugin): [consistent-type-imports] ignore files with decorators, experimentalDecorators, and emitDecoratorMetadata#8335

Uh oh!

Conversation

bradzacher commentedFeb 1, 2024

PR Checklist

Overview

Uh oh!

typescript-eslintbot commentedFeb 1, 2024

Uh oh!

netlifybot commentedFeb 1, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

✅ Deploy Preview fortypescript-eslint ready!

Uh oh!

codecovbot commentedFeb 1, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZackMFleischman commentedFeb 9, 2024

Uh oh!

JoshuaKGoldberg commentedFeb 10, 2024

Uh oh!

nx-cloudbot commentedMar 18, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldbergMar 24, 2024

Choose a reason for hiding this comment

Uh oh!

bradzacherMar 24, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlifybot commentedFeb 1, 2024•
edited
Loading

codecovbot commentedFeb 1, 2024•
edited
Loading

nx-cloudbot commentedMar 18, 2024•
edited
Loading