Repository navigation
Feedback on 'import type' UX for editors #36038
Description
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 Daniel Imms (@Tyriar) Connor Peet (@connor4312) for VS Code
/cc Mine Starks (@minestarks) Andrew Casey (@amcasey) for other editor feedback
Activity
mjbvz commentedon Jan 6, 2020
Another case we talked about is whether the
implement interfacequick fix should useimport type. Today, the quick fix adds inlineimport(...)types which we are not a fan of. But isimport typethe 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 interfacewe can pull in lots of indirectly referenced typesOn the other hand, adding a normal
importcould change the behavior of the program (although in practice, the added imports in theimplement interfacecase 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 commentedon Jan 6, 2020
I think for most users it's preferable to always use standard
importrather thanimport typeon auto import. It's clearly not good to automatically change fromimport type->importon auto-import, as that would corrode the safetyimport typeaims to bring. However, in most cases, users don't needimport 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 typeis 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 commentedon Jan 6, 2020
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 typeinstead ofimport(...).Type, and make that configurable.When
importsNotUsedAsValuesis notremove(i.e. it's set toerrororpreserve), we should probably always useimport type.Would a better behavior be to only do this when
importsNotUsedAsValuesis set topreserve? Maybe the better thing would be to just make this a refactoring, and eventual UX improvements in refactorings would surface the feature.Tyriar commentedon Jan 7, 2020
Daniel Rosenwasser (@DanielRosenwasser) wouldn't that just cause more problems for the user? I don't know why
import(...).Typeis ever used as it's only been annoying to me when that shows up after running implement interface, but addingimport typeanywhere will just cause friction imo as the user will need to manually intervene to remove it when they import something concrete. Reiterating what Connor Peet (@connor4312) said earlier:andrewbranch commentedon Jan 14, 2020
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 withor with
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 commentedon Jan 14, 2020
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):
(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 commentedon Jan 15, 2020
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 commentedon Jan 15, 2020
This is (sort of accidentally) the way it works today in the beta, and we immediately got independent bug reports about it (one from Daniel Rosenwasser (@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 commentedon Jan 15, 2020
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 commentedon Jan 15, 2020
The original PR gives a fairly thorough background and links to ~10 motivating issues. #35200
DanielRosenwasser commentedon Jan 15, 2020
Hey, I think it's important to note that the primary use-cases for
import typeandexport typewereisolatedModules/transpileModule/Babel users from re-exporting types, since many compilation tools are incapable of knowing when anexport { abc }can be dropped.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:
import typeto animport, I think that code review is the time to do that. If an import gets promoted from animport type, you can call that out. There are also tools like API Extractor to make this easier.mjbvz commentedon Jan 21, 2020
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 typeto normalimportbut the vast majority of users should never seeimport typeunless they explicitly add it to their TS fileDanielRosenwasser commentedon Jan 21, 2020
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 Andrew Branch (@andrewbranch)? Ideally it should just be a quick fix for users who turn on
"importsNotUsedAsValues": "error"and"isolatedModules": trueOtherwise, the work to address the gaps are being done on #36092 and should be in the RC itself.
andrewbranch commentedon Jan 21, 2020
The suggestion diagnostic is removed in #36092.
mjbvz commentedon Mar 4, 2020
Closing this issue as is seems all of our feedback has been addressed