microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.12k stars 12.5k forks source link

`organizeImports` does not merge value and type imports #59460

Open terrablue opened 3 months ago

terrablue commented 3 months ago

🔍 Search Terms

organizeImports type

✅ Viability Checklist

⭐ Suggestion

Merge value and type imports into one import.

📃 Motivating Example

If I have the following code

import { CascadeFunction } from "@rcompat/async/cascade";
import cascade from "@rcompat/async/cascade";

And I run organizeImports, then it will be merged into

import cascade, { CascadeFunction } from "@rcompat/async/cascade";

However, if I have

import type { CascadeFunction } from "@rcompat/async/cascade";
import cascade from "@rcompat/async/cascade";

Running organizeImports will do nothing, although I would expect it to merge the imports into

import cascade, { type CascadeFunction } from "@rcompat/async/cascade";

💻 Use Cases

  1. What do you want to use this for? Keeping imports clean.
  2. What shortcomings exist with current approaches? Imports may be doubled, and I might not notice it despite running organizeImports.
  3. What workarounds are you using in the meantime? Merge manually if I notice it.
RyanCavanaugh commented 3 months ago

Note that these merge when they're fully equivalent, e.g.

import { type CascadeFunction } from "@rcompat/async/cascade";
import cascade from "@rcompat/async/cascade";
rwe commented 3 months ago

As @RyanCavanaugh points out, they merge when fully equivalent. I believe this suggestion should be rejected, because it would introduce lossy behavior.

Imagine in that example, starting with:

import type { CascadeFunction } from "@rcompat/async/cascade";

In that case, no runtime import is generated. But then say you add a single runtime import (perhaps temporarily autoimported during a refactor), and this suggestion were implemented:

import cascade, { type CascadeFunction } from "@rcompat/async/cascade";

Now, on second thought, remove that new import. The result still contains an empty runtime import, instead of the original intentional type import:

import { type CascadeFunction } from "@rcompat/async/cascade";

Note that the above is not equivalent, and couldn't be automatically converted back to an import type, because sometimes you want a runtime import (for impure modules), and sometimes you don't, so it needs to preserve the import as written.

terrablue commented 3 months ago

@rwe thank you for the explanation. While this is a fair enough reason to not accept this suggestion, it was not at all clear to me that import type { a } from "b"; and import { type a } from "b";

are not semantically equivalent. The way I read it, and the way I assume most people would read it, is that having type outside of the braces is equivalent to writing type in front of the individual imports inside the braces, in case it's all type imports. I.e., that the former allows only for type imports, while the latter allows combining value and type import, and as such, in case there are no value imports inside the braces, is equivalent to the former.