fix #2268: bug with "preserveValueImports": true

CHANGELOG.md

@@ -2,6 +2,33 @@
 
 ## Unreleased
 
+* Correct esbuild's implementation of `"preserveValueImports": true` ([#2268](https://github.com/evanw/esbuild/issues/2268))
+
+    TypeScript's [`preserveValueImports` setting](https://www.typescriptlang.org/tsconfig#preserveValueImports) tells the compiler to preserve unused imports, which can sometimes be necessary because otherwise TypeScript will remove unused imports as it assumes they are type annotations. This setting is useful for programming environments that strip TypeScript types as part of a larger code transformation where additional code is appended later that will then make use of those unused imports, such as with [Svelte](https://svelte.dev/) or [Vue](https://vuejs.org/).
+
+    This release fixes an issue where esbuild's implementation of `preserveValueImports` diverged from the official TypeScript compiler. If the import clause is present but empty of values (even if it contains types), then the import clause should be considered a type-only import clause. This was an oversight, and has now been fixed:
+
+    ```ts
+    // Original code
+    import "keep"
+    import { k1 } from "keep"
+    import k2, { type t1 } from "keep"
+    import {} from "remove"
+    import { type t2 } from "remove"
+
+    // Old output under "preserveValueImports": true
+    import "keep";
+    import { k1 } from "keep";
+    import k2, {} from "keep";
+    import {} from "remove";
+    import {} from "remove";
+
+    // New output under "preserveValueImports": true (matches the TypeScript compiler)
+    import "keep";
+    import { k1 } from "keep";
+    import k2 from "keep";
+    ```
+
 * Add Opera to more internal feature compatibility tables ([#2247](https://github.com/evanw/esbuild/issues/2247), [#2252](https://github.com/evanw/esbuild/pull/2252))
 
     The internal compatibility tables that esbuild uses to determine which environments support which features are derived from multiple sources. Most of it is automatically derived from [these ECMAScript compatibility tables](https://kangax.github.io/compat-table/), but missing information is manually copied from [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/), GitHub PR comments, and various other websites. Version 0.14.35 of esbuild introduced Opera as a possible target environment which was automatically picked up by the compatibility table script, but the manually-copied information wasn't updated to include Opera. This release fixes this omission so Opera feature compatibility should now be accurate.

internal/bundler/bundler.go

@@ -1185,8 +1185,8 @@ func (s *scanner) maybeParseFile(
 	if resolveResult.UseDefineForClassFieldsTS != config.Unspecified {
 		optionsClone.UseDefineForClassFields = resolveResult.UseDefineForClassFieldsTS
 	}
-	if resolveResult.UnusedImportsTS != config.UnusedImportsRemoveStmt {
-		optionsClone.UnusedImportsTS = resolveResult.UnusedImportsTS
+	if resolveResult.UnusedImportFlagsTS != 0 {
+		optionsClone.UnusedImportFlagsTS = resolveResult.UnusedImportFlagsTS
 	}
 	optionsClone.TSTarget = resolveResult.TSTarget
 

internal/bundler/bundler_tsconfig_test.go

@@ -1162,13 +1162,55 @@ func TestTsconfigPreserveValueImports(t *testing.T) {
 	tsconfig_suite.expectBundled(t, bundled{
 		files: map[string]string{
 			"/Users/user/project/src/entry.ts": `
-				import {x, y} from "./foo"
-				import z from "./foo"
-				import * as ns from "./foo"
-				console.log(1 as x, 2 as z, 3 as ns.y)
+				import {} from "a"
+				import {b1} from "b"
+				import {c1, type c2} from "c"
+				import {d1, d2, type d3} from "d"
+				import {type e1, type e2} from "e"
+				import f1, {} from "f"
+				import g1, {g2} from "g"
+				import h1, {type h2} from "h"
+				import * as i1 from "i"
+				import "j"
+			`,
+			"/Users/user/project/src/tsconfig.json": `{
+				"compilerOptions": {
+					"preserveValueImports": true
+				}
+			}`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.ts"},
+		options: config.Options{
+			Mode:          config.ModeConvertFormat,
+			OutputFormat:  config.FormatESModule,
+			AbsOutputFile: "/Users/user/project/out.js",
+			ExternalSettings: config.ExternalSettings{
+				PostResolve: config.ExternalMatchers{Exact: map[string]bool{
+					"/Users/user/project/src/foo": true,
+				}},
+			},
+		},
+	})
+}
+
+func TestTsconfigPreserveValueImportsAndImportsNotUsedAsValuesPreserve(t *testing.T) {
+	tsconfig_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.ts": `
+				import {} from "a"
+				import {b1} from "b"
+				import {c1, type c2} from "c"
+				import {d1, d2, type d3} from "d"
+				import {type e1, type e2} from "e"
+				import f1, {} from "f"
+				import g1, {g2} from "g"
+				import h1, {type h2} from "h"
+				import * as i1 from "i"
+				import "j"
 			`,
 			"/Users/user/project/src/tsconfig.json": `{
 				"compilerOptions": {
+					"importsNotUsedAsValues": "preserve",
 					"preserveValueImports": true
 				}
 			}`,

internal/bundler/snapshots/snapshots_tsconfig.txt

@@ -393,10 +393,28 @@ console.log(1);
 ================================================================================
 TestTsconfigPreserveValueImports
 ---------- /Users/user/project/out.js ----------
-import { x, y } from "./foo";
-import z from "./foo";
-import * as ns from "./foo";
-console.log(1, 2, 3);
+import { b1 } from "b";
+import { c1 } from "c";
+import { d1, d2 } from "d";
+import f1 from "f";
+import g1, { g2 } from "g";
+import h1 from "h";
+import * as i1 from "i";
+import "j";
+
+================================================================================
+TestTsconfigPreserveValueImportsAndImportsNotUsedAsValuesPreserve
+---------- /Users/user/project/out.js ----------
+import {} from "a";
+import { b1 } from "b";
+import { c1 } from "c";
+import { d1, d2 } from "d";
+import {} from "e";
+import f1, {} from "f";
+import g1, { g2 } from "g";
+import h1, {} from "h";
+import * as i1 from "i";
+import "j";
 
 ================================================================================
 TestTsconfigRemoveUnusedImports

internal/config/config.go

@@ -274,7 +274,7 @@ type Options struct {
 	WriteToStdout bool
 
 	OmitRuntimeForTests     bool
-	UnusedImportsTS         UnusedImportsTS
+	UnusedImportFlagsTS     UnusedImportFlagsTS
 	UseDefineForClassFields MaybeBool
 	ASCIIOnly               bool
 	KeepNames               bool
@@ -303,27 +303,49 @@ const (
 	TargetWasConfiguredAndAtLeastES2022
 )
 
-type UnusedImportsTS uint8
-
+type UnusedImportFlagsTS uint8
+
+// With !UnusedImportKeepStmt && !UnusedImportKeepValues:
+//
+//   "import 'foo'"                      => "import 'foo'"
+//   "import * as unused from 'foo'"     => ""
+//   "import { unused } from 'foo'"      => ""
+//   "import { type unused } from 'foo'" => ""
+//
+// With UnusedImportKeepStmt && !UnusedImportKeepValues:
+//
+//   "import 'foo'"                      => "import 'foo'"
+//   "import * as unused from 'foo'"     => "import 'foo'"
+//   "import { unused } from 'foo'"      => "import 'foo'"
+//   "import { type unused } from 'foo'" => "import 'foo'"
+//
+// With !UnusedImportKeepStmt && UnusedImportKeepValues:
+//
+//   "import 'foo'"                      => "import 'foo'"
+//   "import * as unused from 'foo'"     => "import * as unused from 'foo'"
+//   "import { unused } from 'foo'"      => "import { unused } from 'foo'"
+//   "import { type unused } from 'foo'" => ""
+//
+// With UnusedImportKeepStmt && UnusedImportKeepValues:
+//
+//   "import 'foo'"                      => "import 'foo'"
+//   "import * as unused from 'foo'"     => "import * as unused from 'foo'"
+//   "import { unused } from 'foo'"      => "import { unused } from 'foo'"
+//   "import { type unused } from 'foo'" => "import {} from 'foo'"
+//
 const (
-	// "import { unused } from 'foo'" => "" (TypeScript's default behavior)
-	UnusedImportsRemoveStmt UnusedImportsTS = iota
-
-	// "import { unused } from 'foo'" => "import 'foo'" ("importsNotUsedAsValues" != "remove")
-	UnusedImportsKeepStmtRemoveValues
-
-	// "import { unused } from 'foo'" => "import { unused } from 'foo'" ("preserveValueImports" == true)
-	UnusedImportsKeepValues
+	UnusedImportKeepStmt   UnusedImportFlagsTS = 1 << iota // "importsNotUsedAsValues" != "remove"
+	UnusedImportKeepValues                                 // "preserveValueImports" == true
 )
 
-func UnusedImportsFromTsconfigValues(preserveImportsNotUsedAsValues bool, preserveValueImports bool) UnusedImportsTS {
+func UnusedImportFlagsFromTsconfigValues(preserveImportsNotUsedAsValues bool, preserveValueImports bool) (flags UnusedImportFlagsTS) {
 	if preserveValueImports {
-		return UnusedImportsKeepValues
+		flags |= UnusedImportKeepValues
 	}
 	if preserveImportsNotUsedAsValues {
-		return UnusedImportsKeepStmtRemoveValues
+		flags |= UnusedImportKeepStmt
 	}
-	return UnusedImportsRemoveStmt
+	return
 }
 
 type TSTarget struct {

internal/js_parser/js_parser.go

@@ -395,7 +395,7 @@ type optionsThatSupportStructuralEquality struct {
 	treeShaking             bool
 	dropDebugger            bool
 	mangleQuoted            bool
-	unusedImportsTS         config.UnusedImportsTS
+	unusedImportFlagsTS     config.UnusedImportFlagsTS
 	useDefineForClassFields config.MaybeBool
 }
 
@@ -426,7 +426,7 @@ func OptionsFromConfig(options *config.Options) Options {
 			treeShaking:             options.TreeShaking,
 			dropDebugger:            options.DropDebugger,
 			mangleQuoted:            options.MangleQuoted,
-			unusedImportsTS:         options.UnusedImportsTS,
+			unusedImportFlagsTS:     options.UnusedImportFlagsTS,
 			useDefineForClassFields: options.UseDefineForClassFields,
 		},
 	}
@@ -6628,11 +6628,38 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
 		}
 
 		pathLoc, pathText, assertions := p.parsePath()
+		p.lexer.ExpectOrInsertSemicolon()
+
+		// If TypeScript's "preserveValueImports": true setting is active, TypeScript's
+		// "importsNotUsedAsValues": "preserve" setting is NOT active, and the import
+		// clause is present and empty (or is non-empty but filled with type-only
+		// items), then the import statement should still be removed entirely to match
+		// the behavior of the TypeScript compiler:
+		//
+		//   // Keep these
+		//   import 'x'
+		//   import { y } from 'x'
+		//   import { y, type z } from 'x'
+		//
+		//   // Remove these
+		//   import {} from 'x'
+		//   import { type y } from 'x'
+		//
+		//   // Remove the items from these
+		//   import d, {} from 'x'
+		//   import d, { type y } from 'x'
+		//
+		if p.options.ts.Parse && p.options.unusedImportFlagsTS == config.UnusedImportKeepValues && stmt.Items != nil && len(*stmt.Items) == 0 {
+			if stmt.DefaultName == nil {
+				return js_ast.Stmt{Loc: loc, Data: &js_ast.STypeScript{}}
+			}
+			stmt.Items = nil
+		}
+
 		stmt.ImportRecordIndex = p.addImportRecord(ast.ImportStmt, pathLoc, pathText, assertions)
 		if wasOriginallyBareImport {
 			p.importRecords[stmt.ImportRecordIndex].Flags |= ast.WasOriginallyBareImport
 		}
-		p.lexer.ExpectOrInsertSemicolon()
 
 		if stmt.StarNameLoc != nil {
 			name := p.loadNameFromRef(stmt.NamespaceRef)
@@ -14164,7 +14191,7 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result importsEx
 			//     user is expecting the output to be as small as possible. So we
 			//     should omit unused imports.
 			//
-			keepUnusedImports := p.options.ts.Parse && p.options.unusedImportsTS == config.UnusedImportsKeepValues &&
+			keepUnusedImports := p.options.ts.Parse && (p.options.unusedImportFlagsTS&config.UnusedImportKeepValues) != 0 &&
 				p.options.mode != config.ModeBundle && !p.options.minifyIdentifiers
 
 			// TypeScript always trims unused imports. This is important for
@@ -14180,7 +14207,7 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result importsEx
 					symbol := p.symbols[s.DefaultName.Ref.InnerIndex]
 
 					// TypeScript has a separate definition of unused
-					if p.options.ts.Parse && p.tsUseCounts[s.DefaultName.Ref.InnerIndex] != 0 {
+					if p.options.ts.Parse && (p.tsUseCounts[s.DefaultName.Ref.InnerIndex] != 0 || (p.options.unusedImportFlagsTS&config.UnusedImportKeepValues) != 0) {
 						isUnusedInTypeScript = false
 					}
 
@@ -14196,7 +14223,7 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result importsEx
 					symbol := p.symbols[s.NamespaceRef.InnerIndex]
 
 					// TypeScript has a separate definition of unused
-					if p.options.ts.Parse && p.tsUseCounts[s.NamespaceRef.InnerIndex] != 0 {
+					if p.options.ts.Parse && (p.tsUseCounts[s.NamespaceRef.InnerIndex] != 0 || (p.options.unusedImportFlagsTS&config.UnusedImportKeepValues) != 0) {
 						isUnusedInTypeScript = false
 					}
 
@@ -14219,7 +14246,7 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result importsEx
 						symbol := p.symbols[item.Name.Ref.InnerIndex]
 
 						// TypeScript has a separate definition of unused
-						if p.options.ts.Parse && p.tsUseCounts[item.Name.Ref.InnerIndex] != 0 {
+						if p.options.ts.Parse && (p.tsUseCounts[item.Name.Ref.InnerIndex] != 0 || (p.options.unusedImportFlagsTS&config.UnusedImportKeepValues) != 0) {
 							isUnusedInTypeScript = false
 						}
 
@@ -14251,7 +14278,7 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result importsEx
 				//
 				// We do not want to do this culling in JavaScript though because the
 				// module may have side effects even if all imports are unused.
-				if p.options.ts.Parse && foundImports && isUnusedInTypeScript && p.options.unusedImportsTS == config.UnusedImportsRemoveStmt {
+				if p.options.ts.Parse && foundImports && isUnusedInTypeScript && (p.options.unusedImportFlagsTS&config.UnusedImportKeepStmt) == 0 {
 					// Ignore import records with a pre-filled source index. These are
 					// for injected files and we definitely do not want to trim these.
 					if !record.SourceIndex.IsValid() {

internal/resolver/resolver.go

@@ -122,7 +122,7 @@ type ResolveResult struct {
 	UseDefineForClassFieldsTS config.MaybeBool
 
 	// This is the "importsNotUsedAsValues" and "preserveValueImports" fields from "package.json"
-	UnusedImportsTS config.UnusedImportsTS
+	UnusedImportFlagsTS config.UnusedImportFlagsTS
 }
 
 type DebugMeta struct {
@@ -626,7 +626,7 @@ func (r resolverQuery) finalizeResolve(result *ResolveResult) {
 						result.JSXFactory = dirInfo.enclosingTSConfigJSON.JSXFactory
 						result.JSXFragment = dirInfo.enclosingTSConfigJSON.JSXFragmentFactory
 						result.UseDefineForClassFieldsTS = dirInfo.enclosingTSConfigJSON.UseDefineForClassFields
-						result.UnusedImportsTS = config.UnusedImportsFromTsconfigValues(
+						result.UnusedImportFlagsTS = config.UnusedImportFlagsFromTsconfigValues(
 							dirInfo.enclosingTSConfigJSON.PreserveImportsNotUsedAsValues,
 							dirInfo.enclosingTSConfigJSON.PreserveValueImports,
 						)

pkg/api/api_impl.go

@@ -1361,7 +1361,7 @@ func transformImpl(input string, transformOpts TransformOptions) TransformResult
 	})
 
 	// Settings from the user come first
-	unusedImportsTS := config.UnusedImportsRemoveStmt
+	var unusedImportFlagsTS config.UnusedImportFlagsTS
 	useDefineForClassFieldsTS := config.Unspecified
 	jsx := config.JSXOptions{
 		Preserve: transformOpts.JSXMode == JSXModePreserve,
@@ -1388,7 +1388,7 @@ func transformImpl(input string, transformOpts TransformOptions) TransformResult
 			if result.UseDefineForClassFields != config.Unspecified {
 				useDefineForClassFieldsTS = result.UseDefineForClassFields
 			}
-			unusedImportsTS = config.UnusedImportsFromTsconfigValues(
+			unusedImportFlagsTS = config.UnusedImportFlagsFromTsconfigValues(
 				result.PreserveImportsNotUsedAsValues,
 				result.PreserveValueImports,
 			)
@@ -1436,7 +1436,7 @@ func transformImpl(input string, transformOpts TransformOptions) TransformResult
 		AbsOutputFile:           transformOpts.Sourcefile + "-out",
 		KeepNames:               transformOpts.KeepNames,
 		UseDefineForClassFields: useDefineForClassFieldsTS,
-		UnusedImportsTS:         unusedImportsTS,
+		UnusedImportFlagsTS:     unusedImportFlagsTS,
 		Stdin: &config.StdinInfo{
 			Loader:     validateLoader(transformOpts.Loader),
 			Contents:   input,