Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

type imports are not removed #1525

Closed
swandir opened this issue Aug 16, 2021 · 10 comments
Closed

type imports are not removed #1525

swandir opened this issue Aug 16, 2021 · 10 comments
Labels

Comments

@swandir
Copy link

@swandir swandir commented Aug 16, 2021

In TypeScript modules type imports are not removed, unless importsNotUsedAsValues is "remove".

Given

tsconfg.json

{
  "compilerOptions": {
    "noEmit": true,
    "isolatedModules": true,
    "importsNotUsedAsValues": "error"
  }
}

module.ts

export type Type = unknown;
export const runtimeValue = undefined;

main.ts

import { Type, runtimeValue } from "./module";

export const value = runtimeValue as Type;

ESBuild emits

import { Type, runtimeValue } from "./module";
export const value = runtimeValue;

Which is not correct, because valid TypeScript is transformed into invalid JavaScript.

Note that tsc does not emit an error in that case despite what one might think "importsNotUsedAsValues": "error" means.

Basically, type imports should be removed always.

Update:
To clarify: import statements should be preserved, but imported types should still be removed.
So this code

import { Type } from "./module";

woud be transformed into

import "./module";

Related issue: vitejs/vite#4581
Repro: esbuild-type-import-repro

@hyrious
Copy link

@hyrious hyrious commented Aug 16, 2021

I think this behavior is right, since the TypeScript Docs said:

error: This preserves all imports (the same as the preserve option) ...

As of vite, I think it can do a hack in transforming, since you always want valid javascript output.

@swandir
Copy link
Author

@swandir swandir commented Aug 17, 2021

I think this behavior is right, since the TypeScript Docs said:

error: This preserves all imports (the same as the preserve option) ...

It does preserve imports, but still removes types.

So this code

import { Type } from "./module";

will be transformed into

import "./module";

@hyrious
Copy link

@hyrious hyrious commented Aug 17, 2021

I get it. However I'm in doubt that esbuild could do this checking, since it only knows some names not in use, instead of knowing which of them are types.

So this code

import { a, Type } from "./module"
let x: Type = 1

will be transformed into (when importsNotUsedAsValues is preserve or error)

import "./module"
let x = 1

Emm, it looks fine though.

@swandir
Copy link
Author

@swandir swandir commented Aug 17, 2021

ESBuild transforms this

import { T, v } from "./module";
let t: T;

into this

let t;

when importsNotUsedAsValues is remove.
Which is inline with that tsc does (except tsc also adds export {}, should esbuild do it too?..)

So I guess with "importsNotUsedAsValues": "remove"/"preserve" esbuild could just transform the same code into this

import "./module";
let t;

which would match what tsc does exactly.

@swandir
Copy link
Author

@swandir swandir commented Aug 17, 2021

So this code

import { a, Type } from "./module"
let x: Type = 1

will be transformed into (when importsNotUsedAsValues is preserve or error)

import "./module"
let x = 1

Emm, it looks fine though.

Yeah, that's what tsc does

@evanw
Copy link
Owner

@evanw evanw commented Sep 9, 2021

TypeScript might be changing their behavior for this soon. See microsoft/TypeScript#43393 and microsoft/TypeScript#44619. I think the new behavior will be something like --importsNotUsedAsValues=preserve still does not preserve imports that aren't used as values, but you can additionally add --preserveValueImports to actually preserve unused imported values. We'll have to see what the behavior ends up being when the next version of TypeScript is released. When that happens, esbuild can be changed to do what that version of TypeScript does.

FSMaxB added a commit to communityvi/communityvi that referenced this issue Sep 22, 2021
See vitejs/vite#4581
And evanw/esbuild#1525

This lead to npm run watch not working when upgrading svelte kit which in turn updated vite to 2.5
@evanw
Copy link
Owner

@evanw evanw commented Oct 14, 2021

4.5 is out soon, but is still in beta. Doing npm install typescript still gets you version 4.4. I plan to change esbuild's behavior to align with 4.5 once it comes out.

@evanw evanw added the breaking label Oct 14, 2021
@dummdidumm
Copy link

@dummdidumm dummdidumm commented Oct 21, 2021

The new behavior does not necessarily relate to what the expected existing behavior is. Essentially it comes down to what is inside the tsconfig.json.

  • If importsNotUsedAsValues is remove: Remove all unused values (which includes types, because they are never used as a value). If no value is left, remove the import altogether
  • If importsNotUsedAsValues is preserve: Remove all unused values (which includes types, because they are never used as a value). If no value is left, keep the import path.
  • If importsNotUsedAsValues is error: Remove all unused values (which includes types, because they are never used as a value). If no value is left, keep the import path. If all imports are only used as a type, emit an error
  • If preserveValueImports is turned on, keep all imports not prepended with type no matter if they are used or not.

For ESbuild this means:

  • If importsNotUsedAsValues is remove: Current behavior is correct
  • If importsNotUsedAsValues is preserve / error: These cases are the same to ESBuild as it doesn't deal with type-checking. In both cases, ESBuild needs to remove all unused imports (which includes type imports) and keep the import path no matter what. this is not what ESBuild does currently so this is a bug in my eyes
  • If preserveValueImports is turned on, keep all imports not prepended with type no matter if they are used or not. this is what ESBuild currently does for importsNotUsedAsValues = preserve / error

@evanw
Copy link
Owner

@evanw evanw commented Nov 18, 2021

This has now been released: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#preserve-value-imports. So I plan to implement support for preserveValueImports, which should be straightforward. However, now that this feature exists, this hack that was made so esbuild could be used for Svelte (see #604) can now be removed, which means esbuild's output for import statements can now match the TypeScript compiler (Svelte use cases will now have to enable preserveValueImports). Making esbuild align with TypeScript's behavior exactly is a breaking change, and should be done in a breaking change release. I'm adding this comment to remind myself to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants