Auto-import will always use the first import instead of looking for the first import of the correct kind

[bradzacher]

Bug Report

🔎 Search Terms

auto import type-only value

🕗 Version & Regression Information

• This is the behaviour in every version I tried

⏯ Playground Link

Playground link with relevant code

💻 Code

import type { ComponentType } from 'react';
import { useState } from 'react';

export function Component({ prop } : { prop: ComponentType }) {
  const codeIsUnimportant = useState(1);

  // trigger auto-import for this line
  useEffect(() => {}, []);
}

🙂 Expected behaviour

When I trigger auto-import for the useEffect function, TS should look at all of the imports and find the first value import declaration with the correct source to use.
I.e. I would expect this result:

import type { ComponentType } from 'react';
import { useState, useEffect } from 'react';

🙁 Actual behaviour

When I trigger auto-import for the useEffect function, TS looks for the first import declaration with the correct source, even if it's a type-only import. If it is type-only, TS will remove the qualifier entirely.

I.e. it results in this code

import { ComponentType, useEffect } from 'react';
import { useState } from 'react';

Which is pretty bad because it means TS changes the meaning of the code for some compilers!

If 5.0's verbatimModuleSyntax is turned on, then instead you get this:

import { useEffect, type ComponentType } from 'react';
import { useState } from 'react';

Which is marginally better as it at least preserves the meaning of the code.

---

Note that the same thing happens if you reverse the situation.
playground

import { useState } from 'react';
import type { ComponentType } from 'react';

export function Component({ prop } : { prop: ComponentType }) {
  const codeIsUnimportant = useState(1);

  // trigger auto-import for this line
  type T = InvalidEvent;
}

By default TS will insert the type import in the value import:

import { InvalidEvent, useState } from 'react';
import type { ComponentType } from 'react';

If 5.0's verbatimModuleSyntax is turned on, then instead you get this:

import { type InvalidEvent, useState } from 'react';
import type { ComponentType } from 'react';

---

Suggested change:

• When auto-importing, gather all of the import declaratinos.
• If the imported thing is a type, then prefer using the first type-only import declaration.
• If the imported thing is a value, then prefer using the first value import declaration.
• If there is no matching declaration, then I think one of the following should occur:
  • If the thing is a value and the only import declarations are type-only, then insert a new value import declaration. DO NOT mutate the existing import declaration(s) to remove the top-level type-only qualifier.
  • If the thing is a type and the only import declarations are value, then one of the following should occur (perhaps configurably?):
    • Insert a new specifier into the value declaration, qualifying the specifier with type
    • Insert a new type-only import declaration.

The most important bit, IMO, is that TS should NEVER remove the top-level type qualifier from an import when performing an auto-import.

[andrewbranch]

Agreed that auto-imports should make the non-kind-mutating change whenever multiple imports from the same source exist—auto-imports just has a bad assumption that you’re only going to have one declaration per source. I don’t agree with this:

TS should NEVER remove the top-level type qualifier from an import when performing an auto-import

In our view, the distinction between these two code snippets is unimportant:

import { useEffect, type ComponentType } from "react";

import { useEffect } from "react";
import type { ComponentType } from "react";

Which of these you want feels like an aesthetic choice. The only things we care about are whether there is a “real” import from react, and whether ComponentType can be syntactically excluded from emit. In both versions, both of these are true. What reasons could we have for preferring one over the other?

[bradzacher]

What reasons could we have for preferring one over the other?

In short: Codebase standardisation.

It just so happens that the codebases I'm working in right now (typescript-eslint and Canva) both prefer top-level type specifiers as their standard and both (using different lint rules!!) sort imports so that the type import comes first.

So I very, very frequently hit this problem and have to run the lint rules to fix things.

Completely off-topic rant about codebase style

Creating a codebase style involves pushing to remove unnecessary choices and ambiguity from style guide so that engineers can focus more on solving problems instead of making irrelevant decisions about code.
Additionally allowing choice leaves the door open for:
a) people to change things due to personal preference unnecessarily in PRs (creating noise in the PR and the blame history).
b) reviewers to comment on the style and ask for their personal preference.

This sort of standardisation isn't just good for large engineering teams, but it's good for codebases that have infrequent contributors or regularly have new contributors, such as OSS projects. Setting one way of doing things with lint rules similarly helps focus contributors on solving their problem instead of wasting time learning the codebase style standards.

Note - the above isn't specific to import styles, but is applicable to code style in general!

For imports - you want to either standardise on either always using the inline-type specifier, or always using the top-level type specifier, and enforce it via lint rules.

Some lint rule examples:

• @typescript-eslint/consistent-type-imports enforces all imports are properly qualified.
• import/no-duplicates warns against multiple top-level imports of the same source unless:
  • they have different top-level type specifiers, or
  • the "prefer-inline": true option is set.
• import/consistent-type-specifier-style warns against either top-level or inline specifiers, depending on config.

[andrewbranch]

I agree it’s desirable to have some sort of internal consistency, I just couldn’t tell if there was a strong reason to prefer two imports from "react" over the equivalent one.

TypeScript’s auto-import’s intended behavior is to avoid adding a new import declaration when it doesn’t have to, but if there are already two, it should insert into the one it doesn’t have to modify (this is the part where we definitely just have a bug right now).

For someone like me who would never write two separate imports from the same source in the first place, this means I consistently get a single import declaration per source. For someone who prefers to have values and types split into two import declarations, auto-imports should support that after a type-only and non-type-only import are already present. But it will take a manual step, or running a linter fix, to get yourself into the situation where you have two import declarations. Sound good?

[fatcerberus]

Devil's advocate on the "not making the linter mad" rationale - this arguments works in the other direction too; someone could easily have a style rule that there should never be more than one import for a given module specifier, which TS would then violate by being forced to preserve the import type when auto-importing a value, and that would be equally as annoying to them.

[bradzacher]

@fatcerberus which is why I mentioned a config option if that's something the team is interested in.

Though TBH I don't care about TS making the linter happy or not - it really wasn't the point of the issue. The mention of linters was just because (a) they happen to enforce the top-level style and (b) I have to remember to run the fixers to undo TS's mistake in removing the type qualifier.

Instead this issue is to highlight that TS is misbehaving here because:

1. there's already a value import that it could use instead of mutating the type-only import.
2. if the desired behaviour is to avoids an additional import declaration at all costs and thus remove the top-level type qualifier, then TS must preserve the information the qualifier encoded by updating all specifiers to be correctly qualified.

So for some examples:

Single-import with just a top-level type import:

import type { Foo, Bar, Baz } from 'mod';

// ❌ BAD AUTO IMPORT ALWAYS -- this is the current behaviour!!!
import { Foo, Bar, Baz, Thing } from 'mod';

// ✅ GOOD AUTO IMPORT (if the desired behaviour is avoid adding an import)
import { type Foo, type Bar, type Baz, Thing } from 'mod';

// ✅ GOOD AUTO IMPORT (if the desired behaviour is add a second import)
import type { Foo, Bar, Baz } from 'mod';
import { Thing } from 'mod';

Single (or multiple) imports that use inline type qualifiers:

import { type Foo, type Bar, Baz } from 'mod';

// ❌ BAD AUTO IMPORT ALWAYS
import { Foo, Bar, Baz, Thing } from 'mod';

// ✅ GOOD AUTO IMPORT ALWAYS
import { type Foo, type Bar, Baz, Thing } from 'mod';

Two (or more) imports where at least one is NOT a top-level type import:

import type { Foo } from 'mod';
import { Bar } from 'mod';

// ❌ BAD AUTO IMPORT ALWAYS
import { Foo, Thing } from 'mod';
import { Bar } from 'mod';

// ❌ BAD AUTO IMPORT ALWAYS
import { type Foo, Thing } from 'mod';
import { Bar } from 'mod';

// ✅ GOOD AUTO IMPORT ALWAYS
import type { Foo } from 'mod';
import { Bar, Thing } from 'mod';

[fatcerberus]

Indeed, type imports should never become value imports, especially since that can actually break the build under certain configurations. I think @andrewbranch agreed with you (as do I) that that part is a bug.

[andrewbranch]

Weeeell, this part is settings-dependent, but I can possibly be convinced to pretend it’s not. Without verbatimModuleSyntax (or various confusing combinations of isolatedModules, preserveValueImports, and importsNotUsedAsValues), your settings would indicate that type-only imports are almost never necessary. So the auto-import from

import type { ComponentType } from "react";

to

import { ComponentType, useEffect } from "react";

is completely coherent. We know that after the auto-import, you’re going to have a runtime dependency on the "react" module no matter what. We also know that according to your compiler options, ComponentType can reliably be elided from JS emit without needing a type modifier. Adding that modifier changes nothing about your compilation, according to your settings. You cannot draw an equivalence between the type modifier on the import declaration that existed before and the hypothetical type modifier on ComponentType—they mean different things. The top-level modifier means “I assert that I do not have a runtime dependency on this module.” But the fact that you accept an auto-import in a value position negates this assertion. Since there is no rule enforcing that ComponentType be marked as type-only by merit of it being a type, we don’t interpret the top-level type modifier as satisfying such a rule.

When you enable verbatimModuleSyntax, there is such a rule, which is why you see the behavior you find preferable.

The question here is whether to impute the top-level type modifier with meaning it does not strictly have. I don’t like that, because I think it confuses people about what matters. However, in the years since we introduced type-only imports, I have learned that people get a real kick out of imputing type-only imports with all manner of meaning they don’t have, and I’ve gotten tired of being the stick in the mud telling people to use them only when they matter. verbatimModuleSyntax was my compromise: I think the simplicity of its mental model has real value, and its natural side effect is that you have to be a type-only import maximalist.

So, should auto-imports act more like you have verbatimModuleSyntax enabled when you don’t, and assume a hand-crafted type-only import has author intention behind it, taking care to place your type keyword somewhere else if it has to remove it from the top level? 🤷‍♂️

[fatcerberus]

FWIW, I personally love verbatimModuleSyntax because it aligns nicely with TS design goals - TS should not modify the code I write if it’s valid ES, so if I write import { foo } from "bar" I can be sure that’s what I’m going to get in the output (expected side effects and all), and what will be erased is completely deducible from syntax so I know the compiler isn’t making emit decisions behind my back. Non-vMS behavior is de facto type-directed emit which rubs me completely the wrong way. Thus I tend to use type imports even when not strictly necessary.

[bradzacher]

I definitely get where you're coming from - without the option the checker doesn't enforce one way or the other and behaves exactly the same. IMO if that's the intention, then TS shouldn't allow the type import syntax without the compiler option so the user can't get into these weird situations.
But given that currently you can use the syntax without the option, IMO it would be correct for TS to treat the syntax as if it has meaning, even if it thinks it has none.

Also I'd hazard a guess that the vast majority of codebases that use type imports don't use verbatimModuleSyntax (or its predecessors) or even isolatedModules! Could probably use sourcegraph to check for usages of the option to gather this data.