Skip to content

Support a user preference for preferring the type keyword in auto-imports #54664

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

Closed
5 tasks done
schontz opened this issue Jun 15, 2023 · 21 comments
Closed
5 tasks done
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Domain: Auto-import Suggestion An idea for TypeScript

Comments

@schontz
Copy link

schontz commented Jun 15, 2023

Suggestion

Add support in the TS server to include import type and inline type imports when auto-importing.

// TS server auto-imports like so:
import { Foo } from './foo';

// But I want to be able to specify that it should treat types like so:
import type { Foo } from './foo';
// or
import { type Foo } from './foo';

🔍 Search Terms

  • inline type import
  • import type isolatedmodules

✅ Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

See above

📃 Motivating Example

Many build systems require isolated modules (e.g., esbuild), and often require that types be thusly marked when imported. According to my understanding, that is only done manually, as enabling isolatedModules doesn't seem to affect things.

💻 Use Cases

Work better with isolated module build system requirements. One could also argue that legibility is improved with type imports.

@schontz
Copy link
Author

schontz commented Jun 15, 2023

If there is some way to accomplish this in current versions, fantastic, but I cannot find anything in the docs.

@fatcerberus
Copy link

verbatimModuleSyntax might do what you want.

@DanielRosenwasser
Copy link
Member

That's correct, I believe we chose not to have specific user options here, in favor of performing this based on verbatimModuleSyntax.

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Declined The issue was declined as something which matches the TypeScript vision Domain: Auto-import labels Jun 16, 2023
@DanielRosenwasser DanielRosenwasser changed the title Support type imports Support a user preference for preferring the type keyword in auto-imports Jun 16, 2023
@guillaumebrunerie
Copy link

Why not always do it? Are there some drawbacks to this syntax?

@schontz
Copy link
Author

schontz commented Jun 16, 2023

The docs indicate there are some other side-effects of using verbatimModuleSyntax.

How can you get type imports without other side effects?

@DanielRosenwasser DanielRosenwasser added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed Declined The issue was declined as something which matches the TypeScript vision labels Jun 16, 2023
@benasher44
Copy link

benasher44 commented Jun 20, 2023

For folks needing isolatedModules: true and moduleResolution: "NodeNext", it feels like TypeScript is auto-adding incorrect imports (i.e. auto-adds imports that won't compile unless they're a type imports, in this mode), so an eslint rule is needed to correct them. It would be great to at least do this by default when the TypeScript compiler requires it. There is some nuance too that eslint is missing today, where it doesn't correctly respect this combination with emitDecoratorMetadata: true.

@benasher44
Copy link

benasher44 commented Jun 20, 2023

Actually now that I've written this, I would push for this to not be a user preference (or at least be fixed in this mode, in addition to being a preference in other cases) and instead be "fixed". If you're using this specific combination of flags, you're going to always be fighting TS's auto-imports, when you go to do a full build.

@DanielRosenwasser
Copy link
Member

@benasher44 it's unclear what you're referring to - can you file a separate issue if you think there's a bug?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 20, 2023

The docs indicate there are some other side-effects of using verbatimModuleSyntax.

How can you get type imports without other side effects?

I think there's a misunderstanding. The docs are pointing out to the fact that verbatimModuleSyntax will preserve side-effects of any module that you import if it's not a type import. We don't inject any side-effects, we are simply preserving the behavior of your code.

This is in contrast to the original behavior - where if you imported something and never used it as a JavaScript value, then TypeScript would silently drop the import.

@schontz
Copy link
Author

schontz commented Jun 20, 2023

For example:

That does have some implications when it comes to module interop though. Under this flag, ECMAScript imports and exports won’t be rewritten to require calls when your settings or file extension implied a different module system.

My goal is to make TS automatically add type to imports where it can, without changing any other behavior.

@DanielRosenwasser
Copy link
Member

Okay, gotcha - that makes sense.

@andrewbranch
Copy link
Member

auto-adds imports that won't compile unless they're a type imports

If this ever happens it’s a bug.

I’m pretty open to a user preference for type-only imports. Auto-imports had to get a bit more sophisticated around type-only imports with verbatimModuleSyntax so I think it would be fairly easy to respect a preference now.

@benasher44
Copy link

Got it! Filed #54717

@schontz
Copy link
Author

schontz commented Jun 20, 2023

Nice, NodeNext + isolated modules will be the flags that affect things? I'm fine with that approach, so long as we can get it properly documented.

@andrewbranch
Copy link
Member

The flags that affect type-only auto imports are verbatimModuleSyntax, and combinations of isolatedModules, preserveValueImports, and importsNotUsedAsValues (the latter two are deprecated). They affect things in precisely the way that the compiler requires, minus bugs. None of them are specifically designed to control auto-imports. It sounds like an antipattern to use compiler options that are supposed to control real things in order to arrive at an editor behavior that matches a particular stylistic preference, so I would much rather give you a user preference.

@schontz
Copy link
Author

schontz commented Jun 20, 2023

Seems good. Although I would call it a bit more than a stylistic preference because some build systems require it.

@guillaumebrunerie
Copy link

Is there any situation where it is desirable to do import { Foo } from "./foo" instead of import { type Foo } from "./foo", even though Foo is only used as a type?
If not, type qualifiers could always be added during auto-import, regardless of the options that are currently turned on. But I guess I am missing something.

@andrewbranch
Copy link
Member

some build systems require it

Which ones? isolatedModules is supposed to be enough for basically every third party transpiler, and that alone enforces import type where transpilers don’t have enough info to figure it out. Pretty much everything else is stylistic. I would love to know if there’s something else out there that requires more type-only imports than isolatedModules requires.

Is there any situation where it is desirable to do import { Foo } from "./foo" instead of import { type Foo } from "./foo", even though Foo is only used as a type?

In the vast majority of cases, there’s no observable difference between these two. Some might say it’s desirable not to add unnecessary syntax. Some might say it’s desirable to be explicit about the intended usage. So, it’s a matter of taste, and we generally try not to change long-standing default editor behaviors over matters of taste.

@schontz
Copy link
Author

schontz commented Jun 20, 2023

This whole issue was prompted by migrating something to vite. Upon further investigation, it seems to be more focused and related to re-exporting types, e.g.:

// types/foo.ts
export type Foo: number;

// types/index.ts
export { Foo } from './foo';
// Error: Re-exporting a type when 'isolatedModules' is enabled requires using 'export type'. (tsserver 1205)

It sounds like I have a different issue to solve. I don't imagine TS server can solve this? eslint perhaps?

@andrewbranch
Copy link
Member

isolatedModules tells you that you need export type { Foo } from './foo' there. If you’re building with any tool except tsc, you absolutely need isolatedModules turned on.

@schontz
Copy link
Author

schontz commented Jun 20, 2023

Got it. Since exports are not generated by the TS server, I'm going to close this request. Thank you, all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Domain: Auto-import Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants