Feedback on 'import type' UX for editors

[mjbvz]

I just updated VS Code to build using TypeScript 3.8 nightly. From this, the VS Code team has some feedback on the UX around the new import type syntax. I've captured some of our general feedback here. I'll also be opening issues for specific problems I've noticed

The ... suggestions to convert imports to import type feel intrusive

Currently, TypeScript returns a suggestion for every import that only imports types. These suggestions are rendered as... underlines by VS Code

We feel these suggestions are intrusive. In our understanding, import type seems like an advanced feature that is only really required in a few cases. There is nothing strictly wrong with using a normal import since TS has always been smart enough not to generate an import in the emitted code.

I believe we should disable these suggestions and instead leave them to the linter.

Auto import should not use import type

Currently, if you are only referencing a value as a type, auto import will try to import it using import type. This can get weird if you later try using the same symbol as a concrete value in the same file

Again, I believe that it would be better to add a standard import and let users explicitly change it to import type if they desire.

/cc @Tyriar @connor4312 for VS Code
/cc @minestarks @amcasey for other editor feedback

[mjbvz]

Another case we talked about is whether the implement interface quick fix should use import type. Today, the quick fix adds inline import(...) types which we are not a fan of. But is import type the solution?

This is a slightly different problem than auto import. With auto import the user has indicated they want to use a specific type whereas with implement interface we can pull in lots of indirectly referenced types

On the other hand, adding a normal import could change the behavior of the program (although in practice, the added imports in the implement interface case should never be emitted by TS since they are only being used as types).

Would be interested in hearing others thoughts on this case.

[connor4312]

I think for most users it's preferable to always use standard import rather than import type on auto import. It's clearly not good to automatically change from import type -> import on auto-import, as that would corrode the safety import type aims to bring. However, in most cases, users don't need import type, and requiring manual intervention if the first thing they auto-imported happened to be in an interface sounds confusing and annoying.

Maybe this could be configurable in a setting, but even for users who need it I think the number of cases where import type is required would comprise a small minority of imports.

Thinking back on cases where I could use this, it was always either for a Node module or some subset of files that would be split in my build, such as an Angular module or a dynamic React component. I'm not sure we'd want that level of logic (import type if among these files, but not among these other peer files) in config.

[DanielRosenwasser]

Auto import should not use import type

I kind of agree with this, but only because we don't have something that can automatically promote/demote an import; however, the issue with that is that for long import lists, I wouldn't want to think about formatting diffs.

At the very least, something reasonable would be to always prefer import type instead of import(...).Type, and make that configurable.

When importsNotUsedAsValues is not remove (i.e. it's set to error or preserve), we should probably always use import type.

The ... suggestions to convert imports to import type feel intrusive

Would a better behavior be to only do this when importsNotUsedAsValues is set to preserve? Maybe the better thing would be to just make this a refactoring, and eventual UX improvements in refactorings would surface the feature.

[Tyriar]

At the very least, something reasonable would be to always prefer import type instead of import(...).Type, and make that configurable.

@DanielRosenwasser wouldn't that just cause more problems for the user? I don't know why import(...).Type is ever used as it's only been annoying to me when that shows up after running implement interface, but adding import type anywhere will just cause friction imo as the user will need to manually intervene to remove it when they import something concrete. Reiterating what @connor4312 said earlier:

It's clearly not good to automatically change from import type -> import on auto-import, as that would corrode the safety import type aims to bring.

[andrewbranch]

It's clearly not good to automatically change from import type -> import on auto-import, as that would corrode the safety import type aims to bring.

I’m not sure I understand this statement. Let’s say you have an existing type-only import of something from module "./a". If you then accept an auto-import for another export of "./a" in a value position, it doesn’t matter whether you end up with

import type { FirstImport } from "./a";
import { SecondImport } from "./a";

or with

import { FirstImport, SecondImport } from "./a";

because the emit will be exactly the same. With either strategy, accepting the auto-import adds "./a" as a runtime dependency of your module.

[andrewbranch]

I think I agree that auto-import shouldn’t use type-only imports by default, but it’s unclear what to do when considering an auto-import from a module where the user already has a type-only import. I think I would break it down like this (in order of priority):

1. If there is no existing import declaration from the same module, add a non-type-only import.
2. If there is an existing non-type-only import from the same module, add a new import specifier to that.
3. If there is an existing type-only import from the same module and the auto-import source position is not an expression position, add a new import specifier to that.
4. If there is an existing type-only import from the same module and the auto-import source position is an expression position, convert the type-only import to a non-type-only import and add the new import specifier to it.

(This logic is incomplete because it doesn’t address existing namespace imports, which cannot have named imports added to them, but that’s a bit too far in the weeds at the moment.)

[Tyriar]

If there is an existing type-only import from the same module and the auto-import source position is an expression position, convert the type-only import to a non-type-only import and add the new import specifier to it.

This is a problem, the auto import here should add it to the type only import and get a TS error that they need to resolve manually. The only use case I can come up with for type only imports is when it's imperative that only types are imported for the purpose of typing dynamically imported modules. In my case, allowing this conversion to happen makes it very easy to accidentally re-introduce my component to the bundle and subsequently increase VS Code's start up time by 20-50ms.

It doesn't matter if the emit will be exactly the same or not, the resulting error in the above scenario would point out the problem that I made a mistake and I need to rethink what I'm importing.

[andrewbranch]

the auto import here should add it to the type only import and get a TS error that they need to resolve manually

This is (sort of accidentally) the way it works today in the beta, and we immediately got independent bug reports about it (one from @DanielRosenwasser): #36002, #36033. I’m not necessarily saying you’re wrong, but clearly it was unexpected right away. People expect quick fixes / auto imports to be valid—it’s jarring when an automated fix introduces a new error. And on the other hand, if we simply don’t offer auto-imports in value positions from type-imported modules, we will definitely get bug reports about that.

[Tyriar]

People expect quick fixes / auto imports to be valid—it’s jarring when an automated fix introduces a new error. And on the other hand, if we simply don’t offer auto-imports in value positions from type-imported modules, we will definitely get bug reports about that.

Well you'll get a bug report from me if you convert type imports to non-type imports with an auto import, or add separate import statements for the same file when a type import already exists 😉

Do you have links to why this was added; what are the goals, use cases, etc.?

[andrewbranch]

The original PR gives a fairly thorough background and links to ~10 motivating issues. #35200

[DanielRosenwasser]

Hey, I think it's important to note that the primary use-cases for import type and export type were

• to unblock isolatedModules/transpileModule/Babel users from re-exporting types, since many compilation tools are incapable of knowing when an export { abc } can be dropped.
• to provide people with an option for more spec-compliant behavior because they use a lot of side-effecting code

There's a use-case that I think keeps being brought up is "I want to avoid creating runtime dependencies", but I don't think that's one of the use-cases we're aiming to serve with this feature. In the cases where you're hyper-conscious of creating runtime API dependencies:

• This is already the concern by default for auto-import today. Anything that gets used is potentially a risk for runtime bloat. But unused local errors and import elision can get you pretty far. If you opt to turn off import elision, there's not much we can do, though several bundlers nowadays have ways to signal that a package has no side-effects.
• If there's a concern of promoting an import type to an import, I think that code review is the time to do that. If an import gets promoted from an import type, you can call that out. There are also tools like API Extractor to make this easier.

[mjbvz]

How do we plan to handle this before the 3.8 RC? My feeling is that we can probably push out features such as converting import type to normal import but the vast majority of users should never see import type unless they explicitly add it to their TS file

[DanielRosenwasser]

Can you be more specific on what we want to handle before the RC?

As far as I understand, the suggestion diagnostic is gone, right @andrewbranch? Ideally it should just be a quick fix for users who turn on "importsNotUsedAsValues": "error" and "isolatedModules": true

Otherwise, the work to address the gaps are being done on #36092 and should be in the RC itself.

[andrewbranch]

The suggestion diagnostic is removed in #36092.

[mjbvz]

Closing this issue as is seems all of our feedback has been addressed