"importsNotUsedAsValues": "error" should include type-only imports that are in the same import statement as a value import

[lgarron]

Bug Report

🔎 Search Terms

type-only type modifier import importsNotUsedAsValues tsconfig.json

🕗 Version & Regression Information

Relevant since TypeScript 4.5: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html#type-modifiers-on-import-names

⏯ Playground Link

N/A, I can't see a way to use the playground or workbench to show an issue with multiple files, and/or a specific tsconfig.json setting.

💻 Code

// a.ts
import { myValue, MyType } from "./b";

const foo = myValue;
const x: MyType = { foo };
console.log(x);

// b.ts
export const myValue = 4;
export type MyType = { foo: number };

// tsconfig.json
{
  "compilerOptions": {
    "importsNotUsedAsValues": "error",
  }
}

🙁 Actual behavior

1. TypeScript does not error on: import { myValue, MyType } from "./b";

2. Further, edit a.ts as in VSCode follows and "Organize imports":

// a.ts
import { myValue, MyType } from "./b";

const foo = 4; // This line no longer uses `myValue`.
const x: MyType = { foo };
console.log(x);

This results in ts(1371) because the import is rewritten into import { MyType } from "./b";... which now violates tsconfig.json. This is arguably "safe" behaviour in case you want to preserve any side effects of importing myValue as a type, but it's not a great code editing experience. Even though "importsNotUsedAsValues": "error" has been set this whole time, TypeScript "suddenly starts caring" that MyType is actually only used as a type, whereas it did not do so before "Organize Imports". This makes sense if you learn the details of TypeScript, but I think it would be less confusing if TypeScript "started caring earlier".

🙂 Expected behavior

1. TypeScript requires (and suggests) one of the following:

• import { myValue, type MyType } from "./b";
• import type { MyType } from "./b"; on a separate line.

2. Ideally "Organize Imports" doesn't create new errors. But just the expected behaviour from the previous bullet would help reduce the chance of confusion with organizing imports. (That is, it would help train and guide the user to distinguish the need for a type-only import vs. adding a type modifier to the value import.)

---

Further, when refactoring it is valuable to be able to tell which names are "actually" value vs. type imports — in particular, to be able to distinguish the following at a glance. I'd like to rely on importsNotUsedAsValues to clearly indicate every type import across the codebase. But TypeScript currently doesn't care about the difference between the following:

// In order to avoid a runtime dependency on "foo", only one value import needs to change to a type!
import { v1, type v2, type v3, type v4, type v5, type v6 } from "foo";

// Looks like removing "foo" as a dependency would be a bunch of work.
import { v1, v2, v3, v4, v5, type v6 } from "foo";

---

I understand that the documentation of importsNotUsedAsValues states that it is "to explicitly create an import statement which should never be emitted into JavaScript". So from the original motivation of importsNotUsedAsValues (i.e. for compilers working on a particular snapshot of the code rather than humans maintaining code over time), this isn't a big deal. However, just above, the documentation clearly also states:

error: This preserves all imports (the same as the preserve option), but will error when a value import is only used as a type.

Since an entire import statement cannot be used as a type, I believe the only valid interpretation of the excerpt is that "value import" refers to an individual named import in an import statement. This means that MyType in import { myValue, MyType } from "./b"; is a "value import only used as a type" in and should error. I've been confused about this inconsistency for a long time, and am just now filing an issue because I've finally dissected the mismatch between TypeScript's documentation and behaviour.

If this fix for "importsNotUsedAsValues": "error" is not backwards-compatible enough at this point, I'd love to see at least a new value to enforce the type modifier on imported names not used as values, such as "importsNotUsedAsValues": "error-per-name".

Either way, for the sake of consistency, code clarity, and refactoring I think there should be a way for importsNotUsedAsValues to error like this (independent of isolatedModules):

import { myValue, MyType } from "./b";
                  ^-- `MyType` must be imported either using a type modifier (`import { myValue, type MyType } from "./b";`) or  a type-only import (`import type { MyType } from "./b"`)

[RyanCavanaugh]

@andrewbranch thoughts?

[andrewbranch]

Ideally "Organize Imports" doesn't create new errors.

Agreed; this is a bug.

Otherwise, though, this is working as intended. We made some terminology simplifications in the docs that I now regret, so maybe a more precise wording will clear things up:

error: This preserves all import declarations (the same as the preserve option), but will error when no imported identifier in a non-type-only import is referenced in a JS-emitting position.

That is, the error occurs whenever an import declaration will be emitted to JS unnecessarily. By this logic, you can see why it doesn’t care how many types are also imported in the same import declaration, as long as there is at least one identifier that will stick around in the JS emit:

// ts
import { Type1, Type2, Type3 } from "./types";
// is an error because it would emit:
import {} from "./types";

// but
import { Type1, Type2, Type3, someValue } from "./types";
someValue;
// is ok because it would emit:
import { someValue } from "./types";
someValue;

This should also explain:

TypeScript "suddenly starts caring" that MyType is actually only used as a type

That’s not what TypeScript was caring about. What the option cares about is that the entire import declaration suddenly became unused in the JS emit. You made an edit that fundamentally altered the runtime module dependency graph and you either need to signal that you intended to elide the runtime dependency or explicitly depend on it with a side-effect import of import "./b".

Further, when refactoring it is valuable to be able to tell which names are "actually" value vs. type imports — in particular, to be able to distinguish the following at a glance.

Many people have said this, but it just wasn’t a goal of importsNotUsedAsValues. I’ve grown more sympathetic to this idea, but I don’t want to abuse an already-confusing flag to do it. I’ve been thinking about whether it might be possible to retire this flag and preserveValueImports in favor of a less granular but more explainable flag that just always emits imports exactly as you write them, aside from eliding anything marked as type. This would necessitate marking types as type as you wanted for visibility reasons.

[lgarron]

Thanks!

Otherwise, though, this is working as intended. We made some terminology simplifications in the docs that I now regret, so maybe a more precise wording will clear things up:

error: This preserves all import declarations (the same as the preserve option), but will error when no imported identifier in a non-type-only import is referenced in a JS-emitting position.

If the current functionality won't change, I think this would definitely be a significant improvement and I'd love to see such a wording change.
("JS-emitting position" sounds like overly specific jargon, but I think it's probably better to be accurate than colloquial.)

That said, I still think it would be confusing that "(error on) imports not used as values" doesn't do what it says on the tin, for most people's colloquial understanding of "an import" as an individual named import rather than the an entire import statement (which seems to be so widespread that it was incorrectly used in the docs, as we are discussing).

That’s not what TypeScript was caring about. What the option cares about is that the entire import declaration suddenly became unused in the JS emit. You made an edit that fundamentally altered the runtime module dependency graph and you either need to signal that you intended to elide the runtime dependency or explicitly depend on it with a side-effect import of import "./b".

Yeah, unfortunately I understand the details all too well. 😭
As someone who maintains a library with custom elements (whose "definitions" are side effects), I'd be glad to avoid potential confusion for our library users as much as possible.

Many people have said this, but it just wasn’t a goal of importsNotUsedAsValues. I’ve grown more sympathetic to this idea, but I don’t want to abuse an already-confusing flag to do it. I’ve been thinking about whether it might be possible to retire this flag and preserveValueImports in favor of a less granular but more explainable flag that just always emits imports exactly as you write them, aside from eliding anything marked as type. This would necessitate marking types as type as you wanted for visibility reasons.

I would definitely be glad to see this. In theory I wouldn't care about this so much, but it would help a lot with codebase maintenance in practice, especially if you want code to be understood consistently by third-party tools in the ecosystem.

[fatcerberus]

a less granular but more explainable flag that just always emits imports exactly as you write them, aside from eliding anything marked as type

I second this; I’ve wanted that for quite a while. Since import is standard JS, TS ideally wouldn’t touch it (outside of transpilation) unless you explicitly tell it to via import type (similar to how declare works). The fact that TS can elide unused imports—potentially changing runtime behavior—was always a sore spot with me.

[iisaduan]

importsNotUsedAsValues has been deprecated in favor of verbatimModuleSyntax (#52203)