fix(ast-spec): replace `attributes` with `options` property in `TSImportType` by fisker · Pull Request #10691 · typescript-eslint/typescript-eslint

fisker · 2025-01-21T00:45:04Z

PR Checklist

Addresses an existing open issue: fixes #000
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

The AST should align with ImportExpression which contains .options with Property

typescript-eslint · 2025-01-21T00:45:06Z

Thanks for the PR, @fisker!

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 · 2025-01-21T00:45:21Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`9d8e54f`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/679255df297b75000854be45
😎 Deploy Preview	https://deploy-preview-10691--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 93 (🔴 down 5 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.

nx-cloud · 2025-01-21T00:47:19Z

View your CI Pipeline Execution ↗ for commit 9d8e54f.

Command	Status	Duration	Result
`nx test eslint-plugin --coverage=false`	✅ Succeeded	7m 37s	View ↗
`nx test eslint-plugin`	✅ Succeeded	5m 59s	View ↗
`nx test typescript-estree --coverage=false`	✅ Succeeded	1m 31s	View ↗
`nx test utils --coverage=false`	✅ Succeeded	7s	View ↗
`nx test visitor-keys --coverage=false`	✅ Succeeded	3s	View ↗
`nx run types:build`	✅ Succeeded	<1s	View ↗
`nx test type-utils --coverage=false`	✅ Succeeded	22s	View ↗
`nx run-many --target=build --exclude website --...`	✅ Succeeded	1m 16s	View ↗
`Additional runs (24)`	✅ Succeeded	...	View ↗

☁️ Nx Cloud last updated this comment at 2025-01-23 15:03:50 UTC

codecov · 2025-01-21T01:03:52Z

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 87.12%. Comparing base (79af426) to head (9d8e54f).
Report is 86 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/typescript-estree/src/convert.ts	0.00%	10 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10691      +/-   ##
==========================================
- Coverage   87.17%   87.12%   -0.05%     
==========================================
  Files         448      450       +2     
  Lines       15598    15633      +35     
  Branches     4555     4566      +11     
==========================================
+ Hits        13597    13621      +24     
- Misses       1645     1655      +10     
- Partials      356      357       +1

Flag	Coverage Δ
unittest	`87.12% <0.00%> (-0.05%)`	⬇️

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

Files with missing lines	Coverage Δ
packages/visitor-keys/src/visitor-keys.ts	`100.00% <ø> (ø)`
packages/typescript-estree/src/convert.ts	`28.55% <0.00%> (-0.33%)`	⬇️

... and 7 files with indirect coverage changes

JoshuaKGoldberg · 2025-01-21T12:33:35Z

@fisker can you say more, why should it use options and not `attributes?

fisker · 2025-01-21T13:10:48Z

For values, in ImportDeclaration, with is a keyword, the attributes don't need location information for it, but in import("foo", {with: {type: "json"}}) (ImportExpression, not TSImportType) it already use ObjectExpression, so it contains the location information for the second "argument"(at least it looks like one), without location information of the open brace, it will be hard for Prettier and ESLint to use.
In babel, with ts plugin, it already support TSImportType with .options which is an ObjectExpression

I'm fine to accept if you have a better AST shape to suggest, but I still hope it contains the location information of {, }, and with.

fisker · 2025-01-21T13:14:48Z

Since ImportExpression and TSImportType are exactly the same code, it will be much easier if they share same AST shape, I'd suggest rename .argument to .source in future.

JoshuaKGoldberg

Gotcha, thanks - this makes sense. I see why you'd want to align them to the options from ImportExpression.

Since we only just released support yesterday for this relatively-uncommon AST usage I think it's fine to treat this as a non-breaking-change fix.

cc @ronami as I might have (again) missed something.

ronami

Oh, I missed this one too, I didn't consider it not having the location information on the entire second "argument" 🙈

+1 from me to treat this as non-breaking; I didn't even know this was correct TypeScript syntax until a few days ago 😁

Some thoughts:

It seems TypeScript's AST uses ts.ImportAttributes over ts.Expression for this (see here), which is also used in ts.ImportDeclaration and ts.ExportDeclaration.

To my understanding, typescript-eslint's AST coverts these to ImportAttribute[] (e.g. 1, 2), so this would be the first time a ts.ImportAttributes node is converted into something other than ImportAttribute[].

Still, I think the change is correct, as the existing AST doesn't include location information for the second "argument", making it rather hard to use 👍

JoshuaKGoldberg

Noting #10649: is there more work to be done here?

fisker · 2025-01-23T14:45:25Z

Updated vistorKeys.

JoshuaKGoldberg

One question ~

JoshuaKGoldberg

LGTM, thanks. I don't want to keep delaying on this - it's a good change and the longer we wait the more likely an issue will pop up. Merging now to get in today's release.

Use TSImportType.options instead of TSImportType.attributes

bfd1407

Linting

296a03c

fisker marked this pull request as draft January 21, 2025 00:56

fisker added 6 commits January 21, 2025 09:29

Fix

af7713e

Fix AST shape

4c1b8f1

reduce diff

6190da7

Fix range

339f60b

Fix range

bf53544

Fix types

2c7d51b

fisker changed the title ~~Use TSImportType.options instead of TSImportType.attributes~~ fix(ast-spec): Use TSImportType.options instead of TSImportType.attributes Jan 21, 2025

fisker changed the title ~~fix(ast-spec): Use TSImportType.options instead of TSImportType.attributes~~ fix(ast-spec): use TSImportType.options instead of TSImportType.attributes Jan 21, 2025

fisker added 2 commits January 21, 2025 10:43

Fix types

bd6dea3

Remove outdated comment

c6fba45

fisker changed the title ~~fix(ast-spec): use TSImportType.options instead of TSImportType.attributes~~ fix(ast-spec): use TSImportType.options instead of .attributes Jan 21, 2025

fisker changed the title ~~fix(ast-spec): use TSImportType.options instead of .attributes~~ fix(ast-spec): replace attributes with options property in TSImportType Jan 21, 2025

Format

f7e7d7f

fisker marked this pull request as ready for review January 21, 2025 04:13

fisker added 2 commits January 21, 2025 13:40

Linting

5edd9cd

Fix location

e2a1b8c

JoshuaKGoldberg previously approved these changes Jan 21, 2025

View reviewed changes

ronami previously approved these changes Jan 21, 2025

View reviewed changes

bradzacher previously approved these changes Jan 22, 2025

View reviewed changes

fisker mentioned this pull request Jan 23, 2025

Release blog for 3.5 prettier/prettier#17020

Merged

4 tasks

JoshuaKGoldberg mentioned this pull request Jan 23, 2025

fix(visitor-keys): update keys for ImportAttribute #10649

Merged

3 tasks

JoshuaKGoldberg reviewed Jan 23, 2025

View reviewed changes

Update visitor-keys.ts

9d8e54f

fisker dismissed stale reviews from bradzacher, ronami, and JoshuaKGoldberg via 9d8e54f January 23, 2025 14:44

JoshuaKGoldberg requested changes Jan 27, 2025

View reviewed changes

Comment thread packages/visitor-keys/src/visitor-keys.ts

JoshuaKGoldberg approved these changes Feb 24, 2025

View reviewed changes

JoshuaKGoldberg merged commit 476c2c0 into typescript-eslint:main Feb 24, 2025

fisker deleted the TSImportType-options branch February 24, 2025 20:27

github-actions Bot locked as resolved and limited conversation to collaborators Mar 4, 2025

Uh oh!

Conversation

fisker commented Jan 21, 2025

PR Checklist

Overview

Uh oh!

typescript-eslint Bot commented Jan 21, 2025

Uh oh!

netlify Bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for typescript-eslint ready!

Uh oh!

nx-cloud Bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JoshuaKGoldberg commented Jan 21, 2025

Uh oh!

fisker commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisker commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

ronami left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

fisker commented Jan 23, 2025

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify Bot commented Jan 21, 2025 •

edited

Loading

nx-cloud Bot commented Jan 21, 2025 •

edited

Loading

codecov Bot commented Jan 21, 2025 •

edited

Loading

fisker commented Jan 21, 2025 •

edited

Loading

fisker commented Jan 21, 2025 •

edited

Loading

ronami left a comment •

edited

Loading