-
Notifications
You must be signed in to change notification settings - Fork 3k
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
style: automatic type-only imports #7382
Conversation
Hey @demensky, we've already had similar PRs in past and unfortunately, they didn't work for us. Please take a look at this PR #6943 where you can find why it didn't work for us. Did you maybe try to run the docs app and check whether the issue explained in the mentioned PR applies to the changes form this PR? |
Hehe, have I now completely missed the point? It looks like However, I'd still like to know whether the docs app has issues with these changes. |
c0f4add
to
ebfb35a
Compare
@jakovljevic-mladen, I checked the docs app and didn't find any problems with it. Thanks to you, I also decided to add ![]() P.S. I deleted |
Actually, I don't think we had an issue with So, to test it, please open |
@jakovljevic-mladen. Here's video proof. Although I would be very grateful if you also checked locally. 2023-11-22.14.54.59.webm |
Cool, I think that should be good enough. I also checked pipelines and it seems that we don't have issues there as well, so I think it's fine. |
ebfb35a
to
46a3fb5
Compare
"rules": { | ||
"@typescript-eslint/no-explicit-any": "off", | ||
"@typescript-eslint/no-unused-vars": "off", | ||
"@typescript-eslint/no-unsafe-declaration-merging": "off", | ||
"@typescript-eslint/consistent-type-imports": "warn", | ||
"@typescript-eslint/consistent-type-exports": "warn", |
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.
Question: Should these be "error" instead of warn?
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.
#7391. I also discovered that eslint also checks dist. I will create a separate PR.
In general, I think this is fine. It doesn't really "fix" my ask, which is I legit wish that VS Code would highlight types differently... but besides that, I think it's a solid add. It makes the code easier to grep, if maybe mildly annoying to refactor. We might want to make the eslint rule an error if we want to keep it clean. |
Automatically add
type
toimport
/export
if the symbol is used only for type annotations and declarations.Fixes: https://x.com/BenLesh/status/1724808188988633490