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

bradzacher · 2024-02-01T13:13:24Z

PR Checklist

Addresses an existing open issue: fixes Bug: [consistent-type-imports] Using with isolatedModules and emitDecoratorMetadata #5468, fixes Docs: consistent-type-imports vs importsNotUsedAsValues #6393
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Note: I've labelled this as a feat 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.0 experimentalDecorators 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 to experimentalDecorators

This PR:

Removes the decorator metadata detection logic from scope-manager
Adds a new experimentalDecorators parser option that is auto-populated from type info if available
Exposes both experimentalDecorators and emitDecoratorMetadata 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.
Updates consistent-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.

typescript-eslint · 2024-02-01T13:13:26Z

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.

netlify · 2024-02-01T13:13:29Z

✅ Deploy Preview for typescript-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 your Netlify site configuration.

…rators, experimentalDecorators, and emitDecoratorMetadata

codecov · 2024-02-01T13:56:01Z

Codecov Report

Attention: Patch coverage is 92.38095% with 8 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%)`	⬇️

... and 146 files with indirect coverage changes

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

JoshuaKGoldberg

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

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

packages/parser/src/parser.ts

packages/scope-manager/src/analyze.ts

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

docs/packages/Parser.mdx

packages/scope-manager/src/analyze.ts

ZackMFleischman · 2024-02-09T01:41:46Z

@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! 😄

JoshuaKGoldberg · 2024-02-10T03:08:05Z

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

nx-cloud · 2024-03-18T11:09:14Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 5749a57. 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 💌 from NxCloud.

JoshuaKGoldberg

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.mdx

packages/scope-manager/src/analyze.ts

packages/typescript-estree/src/createParserServices.ts

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

packages/eslint-plugin/docs/rules/consistent-type-imports.mdx

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

JoshuaKGoldberg

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

JoshuaKGoldberg · 2024-03-24T22:17:35Z

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

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


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.

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.

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

bradzacher added the enhancement label Feb 1, 2024

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

1c1f9f4

…rators, experimentalDecorators, and emitDecoratorMetadata

bradzacher force-pushed the 5468-CTI-ignore-exp-deco-metadata branch from f3525d4 to 1c1f9f4 Compare February 1, 2024 13:31

bradzacher requested a review from JoshuaKGoldberg February 2, 2024 00:04

JoshuaKGoldberg reviewed Feb 3, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-imports.md Outdated Show resolved Hide resolved

JoshuaKGoldberg reviewed Feb 3, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response label Feb 3, 2024

This was referenced Feb 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

bradzacher added 2 commits March 18, 2024 21:24

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

01e2d4a

fixup after merge

d786f7c

bradzacher added 2 commits March 18, 2024 23:54

updoots pt 1

3673cd1

updoots pt 2

4d28e81

bradzacher requested a review from JoshuaKGoldberg March 18, 2024 13:43

github-actions bot removed the awaiting response label Mar 18, 2024

docs

4f0d04e

JoshuaKGoldberg requested changes Mar 18, 2024

View reviewed changes

JoshuaKGoldberg reviewed Mar 18, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/consistent-type-imports.mdx Show resolved Hide resolved

JoshuaKGoldberg reviewed Mar 18, 2024

View reviewed changes

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

JoshuaKGoldberg added the awaiting response label Mar 18, 2024

bradzacher added 3 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 from JoshuaKGoldberg March 24, 2024 04:03

github-actions bot removed the awaiting response label Mar 24, 2024

JoshuaKGoldberg previously approved these changes Mar 24, 2024

View reviewed changes

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

e85e919

bradzacher dismissed JoshuaKGoldberg’s stale review via e85e919 March 24, 2024 23:05

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

5749a57

bradzacher merged commit e408b93 into main Mar 24, 2024
57 of 59 checks passed

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-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

bradzacher commented Feb 1, 2024

typescript-eslint bot commented Feb 1, 2024

netlify bot commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

ZackMFleischman commented Feb 9, 2024

JoshuaKGoldberg commented Feb 10, 2024

nx-cloud bot commented Mar 18, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment

JoshuaKGoldberg Mar 24, 2024

bradzacher Mar 24, 2024

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

Conversation

bradzacher commented Feb 1, 2024

PR Checklist

Overview

typescript-eslint bot commented Feb 1, 2024

netlify bot commented Feb 1, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

codecov bot commented Feb 1, 2024 • edited Loading

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

ZackMFleischman commented Feb 9, 2024

JoshuaKGoldberg commented Feb 10, 2024

nx-cloud bot commented Mar 18, 2024 • edited Loading

☁️ Nx Cloud Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 24, 2024

Choose a reason for hiding this comment

bradzacher Mar 24, 2024

Choose a reason for hiding this comment

netlify bot commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

nx-cloud bot commented Mar 18, 2024 •

edited

Loading