fix #604: keep unused imports for svelte

CHANGELOG.md

@@ -15,6 +15,35 @@
     ⚡ Done in 43ms
     ```
 
+* Keep unused imports in TypeScript code in one specific case ([#604](https://github.com/evanw/esbuild/issues/604))
+
+    The official TypeScript compiler always removes imported symbols that aren't used as values when converting TypeScript to JavaScript. This is because these symbols could be types and not removing them could result in a run-time module instantiation failure because of missing exports. This even happens when the `tsconfig.json` setting `"importsNotUsedAsValues"` is set to `"preserve"`. Doing this just keeps the import statement itself but confusingly still removes the imports that aren't used as values.
+
+    Previously esbuild always exactly matched the behavior of the official TypeScript compiler regarding import removal. However, that is problematic when trying to use esbuild to compile a partial module such as when converting TypeScript to JavaScript inside a file written in the [Svelte](https://svelte.dev/) programming language. Here is an example:
+
+    ```html
+    <script lang="ts">
+      import Counter from './Counter.svelte';
+      export let name: string = 'world';
+    </script>
+    <main>
+      <h1>Hello {name}!</h1>
+      <Counter />
+    </main>
+    ```
+
+    The current Svelte compiler plugin for TypeScript only provides esbuild with the contents of the `<script>` tag so to esbuild, the import `Counter` appears to be unused and is removed.
+
+    In this release, esbuild deliberately deviates from the behavior of the official TypeScript compiler if all of these conditions are met:
+
+    * The `"importsNotUsedAsValues"` field in `tsconfig.json` must be present and must not be set to `"remove"`. This is necessary because this is the only case where esbuild can assume that all imports are values instead of types. Any imports that are types will cause a type error when the code is run through the TypeScript type checker. To import types when the `importsNotUsedAsValues` setting is active, you must use the TypeScript-specific `import type` syntax instead.
+
+    * You must not be using esbuild as a bundler. When bundling, esbuild needs to assume that it's not seeing a partial file because the bundling process requires renaming symbols to avoid cross-file name collisions.
+
+    * You must not have identifier minification enabled. It's useless to preserve unused imports in this case because referencing them by name won't work anyway. And keeping the unused imports would be counter-productive to minification since they would be extra unnecessary data in the output file.
+
+    This should hopefully allow esbuild to be used as a TypeScript-to-JavaScript converter for programming languages such as Svelte, at least in many cases. The build pipeline in esbuild wasn't designed for compiling partial modules and this still won't be a fully robust solution (e.g. some variables may be renamed to avoid name collisions in rare cases). But it's possible that these cases are very unlikely to come up in practice. Basically this change to keep unused imports in this case should be useful at best and harmless at worst.
+
 ## 0.8.27
 
 * Mark `import.meta` as supported in node 10.4+ ([#626](https://github.com/evanw/esbuild/issues/626))

internal/bundler/bundler_tsconfig_test.go

@@ -841,7 +841,7 @@ func TestTsconfigWarningsInsideNodeModules(t *testing.T) {
 	})
 }
 
-func TestTsconfigRemoveTypeImports(t *testing.T) {
+func TestTsconfigRemoveUnusedImports(t *testing.T) {
 	tsconfig_suite.expectBundled(t, bundled{
 		files: map[string]string{
 			"/Users/user/project/src/entry.ts": `
@@ -862,7 +862,7 @@ func TestTsconfigRemoveTypeImports(t *testing.T) {
 	})
 }
 
-func TestTsconfigPreserveTypeImports(t *testing.T) {
+func TestTsconfigPreserveUnusedImports(t *testing.T) {
 	tsconfig_suite.expectBundled(t, bundled{
 		files: map[string]string{
 			"/Users/user/project/src/entry.ts": `
@@ -887,3 +887,36 @@ func TestTsconfigPreserveTypeImports(t *testing.T) {
 		},
 	})
 }
+
+// This must preserve the import clause even though all imports are not used as
+// values. THIS BEHAVIOR IS A DEVIATION FROM THE TYPESCRIPT COMPILER! It exists
+// to support the use case of compiling partial modules for compile-to-JavaScript
+// languages such as Svelte.
+func TestTsconfigPreserveUnusedImportClause(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)
+			`,
+			"/Users/user/project/src/tsconfig.json": `{
+				"compilerOptions": {
+					"importsNotUsedAsValues": "preserve"
+				}
+			}`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.ts"},
+		options: config.Options{
+			Mode:          config.ModeConvertFormat,
+			OutputFormat:  config.FormatESModule,
+			AbsOutputFile: "/Users/user/project/out.js",
+			ExternalModules: config.ExternalModules{
+				AbsPaths: map[string]bool{
+					"/Users/user/project/src/foo": true,
+				},
+			},
+		},
+	})
+}

internal/bundler/snapshots/snapshots_tsconfig.txt

@@ -274,14 +274,22 @@ var import_util = __toModule(require_util());
 console.log(import_util.default());
 
 ================================================================================
-TestTsconfigPreserveTypeImports
+TestTsconfigPreserveUnusedImportClause
+---------- /Users/user/project/out.js ----------
+import {x, y} from "./foo";
+import z from "./foo";
+import * as ns from "./foo";
+console.log(1, 2, 3);
+
+================================================================================
+TestTsconfigPreserveUnusedImports
 ---------- /Users/user/project/out.js ----------
 // Users/user/project/src/entry.ts
 import "./src/foo";
 console.log(1);
 
 ================================================================================
-TestTsconfigRemoveTypeImports
+TestTsconfigRemoveUnusedImports
 ---------- /Users/user/project/out.js ----------
 // Users/user/project/src/entry.ts
 console.log(1);

internal/js_parser/js_parser.go

@@ -10150,10 +10150,70 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result scanForIm
 	for _, stmt := range stmts {
 		switch s := stmt.Data.(type) {
 		case *js_ast.SImport:
+
+			// The official TypeScript compiler always removes unused imported
+			// symbols. However, we deliberately deviate from the official
+			// TypeScript compiler's behavior doing this in a specific scenario:
+			// we are not bundling, symbol renaming is off, and the tsconfig.json
+			// "importsNotUsedAsValues" setting is present and is not set to
+			// "remove".
+			//
+			// This exists to support the use case of compiling partial modules for
+			// compile-to-JavaScript languages such as Svelte. These languages try
+			// to reference imports in ways that are impossible for esbuild to know
+			// about when esbuild is only given a partial module to compile. Here
+			// is an example of some Svelte code that might use esbuild to convert
+			// TypeScript to JavaScript:
+			//
+			//   <script lang="ts">
+			//     import Counter from './Counter.svelte';
+			//     export let name: string = 'world';
+			//   </script>
+			//   <main>
+			//     <h1>Hello {name}!</h1>
+			//     <Counter />
+			//   </main>
+			//
+			// Tools that use esbuild to compile TypeScript code inside a Svelte
+			// file like this only give esbuild the contents of the <script> tag.
+			// These tools work around this missing import problem when using the
+			// official TypeScript compiler by hacking the TypeScript AST to
+			// remove the "unused import" flags. This isn't possible in esbuild
+			// because esbuild deliberately does not expose an AST manipulation
+			// API for performance reasons.
+			//
+			// We deviate from the TypeScript compiler's behavior in this specific
+			// case because doing so is useful for these compile-to-JavaScript
+			// languages and is benign in other cases. The rationale is as follows:
+			//
+			//   * If "importsNotUsedAsValues" is absent or set to "remove", then
+			//     we don't know if these imports are values or types. It's not
+			//     safe to keep them because if they are types, the missing imports
+			//     will cause run-time failures because there will be no matching
+			//     exports. It's only safe keep imports if "importsNotUsedAsValues"
+			//     is set to "preserve" or "error" because then we can assume that
+			//     none of the imports are types (since the TypeScript compiler
+			//     would generate an error in that case).
+			//
+			//   * If we're bundling, then we know we aren't being used to compile
+			//     a partial module. The parser is seeing the entire code for the
+			//     module so it's safe to remove unused imports. And also we don't
+			//     want the linker to generate errors about missing imports if the
+			//     imported file is also in the bundle.
+			//
+			//   * If identifier minification is enabled, then using esbuild as a
+			//     partial-module transform library wouldn't work anyway because
+			//     the names wouldn't match. And that means we're minifying so the
+			//     user is expecting the output to be as small as possible. So we
+			//     should omit unused imports.
+			//
+			keepUnusedImports := p.options.ts.Parse && p.options.preserveUnusedImportsTS &&
+				p.options.mode != config.ModeBundle && !p.options.minifyIdentifiers
+
 			// TypeScript always trims unused imports. This is important for
 			// correctness since some imports might be fake (only in the type
 			// system and used for type-only imports).
-			if p.options.mangleSyntax || p.options.ts.Parse {
+			if (p.options.mangleSyntax || p.options.ts.Parse) && !keepUnusedImports {
 				foundImports := false
 				isUnusedInTypeScript := true
 
@@ -10272,7 +10332,7 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result scanForIm
 					//   console.log(ns, ns.a, ns.b)
 					//
 					convertStarToClause := p.symbols[s.NamespaceRef.InnerIndex].UseCountEstimate == 0
-					if convertStarToClause {
+					if convertStarToClause && !keepUnusedImports {
 						s.StarNameLoc = nil
 					}