fix #1801: no star-to-clause for external imports

evanw · Dec 3, 2021 · 44b2665 · 44b2665
1 parent 56da27e
commit 44b2665
Show file tree

Hide file tree

Showing 8 changed files with 259 additions and 104 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,25 @@
 # Changelog
 
+## Unreleased
+
+* Disable star-to-clause transform for external imports ([#1801](https://github.com/evanw/esbuild/issues/1801))
+
+    When bundling is enabled, esbuild automatically transforms `import * as x from 'y'; x.z()` into `import {z} as 'y'; z()` to improve tree shaking. This avoids needing to create the import namespace object `x` if it's unnecessary, which can result in the removal of large amounts of unused code. However, this transform shouldn't be done for external imports because that incorrectly changes the semantics of the import. If the export `z` doesn't exist in the previous example, the value `x.z` is a property access that is undefined at run-time, but the value `z` is an import error that will prevent the code from running entirely. This release fixes the problem by avoiding doing this transform for external imports:
+
+    ```js
+    // Original code
+    import * as x from 'y';
+    x.z();
+
+    // Old output (with --bundle --format=esm --external:y)
+    import { z } from "y";
+    z();
+
+    // New output (with --bundle --format=esm --external:y)
+    import * as x from "y";
+    x.z();
+    ```
+
 ## 0.14.1
 
 * Fix `imports` in `package.json` ([#1807](https://github.com/evanw/esbuild/issues/1807))

diff --git a/internal/bundler/bundler_css_test.go b/internal/bundler/bundler_css_test.go
@@ -160,7 +160,7 @@ func TestCSSFromJSMissingStarImport(t *testing.T) {
 			Mode:         config.ModeBundle,
 			AbsOutputDir: "/out",
 		},
-		expectedCompileLog: `entry.js: WARNING: Import "missing" will always be undefined because there is no matching export
+		expectedCompileLog: `entry.js: WARNING: Import "missing" will always be undefined because there is no matching export in "a.css"
 `,
 	})
 }

diff --git a/internal/bundler/bundler_importstar_test.go b/internal/bundler/bundler_importstar_test.go
@@ -1065,7 +1065,7 @@ func TestNamespaceImportMissingES6(t *testing.T) {
 			Mode:          config.ModeBundle,
 			AbsOutputFile: "/out.js",
 		},
-		expectedCompileLog: `entry.js: WARNING: Import "foo" will always be undefined because there is no matching export
+		expectedCompileLog: `entry.js: WARNING: Import "foo" will always be undefined because there is no matching export in "foo.js"
 `,
 	})
 }
@@ -1127,7 +1127,7 @@ func TestNamespaceImportUnusedMissingES6(t *testing.T) {
 			Mode:          config.ModeBundle,
 			AbsOutputFile: "/out.js",
 		},
-		expectedCompileLog: `entry.js: WARNING: Import "foo" will always be undefined because there is no matching export
+		expectedCompileLog: `entry.js: WARNING: Import "foo" will always be undefined because there is no matching export in "foo.js"
 `,
 	})
 }
@@ -1283,7 +1283,7 @@ func TestNamespaceImportReExportStarMissingES6(t *testing.T) {
 			Mode:          config.ModeBundle,
 			AbsOutputFile: "/out.js",
 		},
-		expectedCompileLog: `entry.js: WARNING: Import "foo" will always be undefined because there is no matching export
+		expectedCompileLog: `entry.js: WARNING: Import "foo" will always be undefined because there is no matching export in "foo.js"
 `,
 	})
 }
@@ -1307,7 +1307,7 @@ func TestNamespaceImportReExportStarUnusedMissingES6(t *testing.T) {
 			Mode:          config.ModeBundle,
 			AbsOutputFile: "/out.js",
 		},
-		expectedCompileLog: `entry.js: WARNING: Import "foo" will always be undefined because there is no matching export
+		expectedCompileLog: `entry.js: WARNING: Import "foo" will always be undefined because there is no matching export in "foo.js"
 `,
 	})
 }
@@ -1661,8 +1661,8 @@ func TestImportDefaultNamespaceComboIssue446(t *testing.T) {
 				},
 			},
 		},
-		expectedCompileLog: `internal-def.js: WARNING: Import "def" will always be undefined because there is no matching export
-internal-ns-def.js: WARNING: Import "def" will always be undefined because there is no matching export
+		expectedCompileLog: `internal-def.js: WARNING: Import "def" will always be undefined because there is no matching export in "internal.js"
+internal-ns-def.js: WARNING: Import "def" will always be undefined because there is no matching export in "internal.js"
 `,
 	})
 }
@@ -1687,19 +1687,113 @@ func TestImportDefaultNamespaceComboNoDefault(t *testing.T) {
 		options: config.Options{
 			Mode:         config.ModeBundle,
 			AbsOutputDir: "/out",
-			ExternalModules: config.ExternalModules{
-				NodeModules: map[string]bool{
-					"external": true,
-				},
-			},
 		},
 		expectedCompileLog: `entry-default-ns-prop.js: ERROR: No matching export in "foo.js" for import "default"
-entry-default-ns-prop.js: WARNING: Import "default" will always be undefined because there is no matching export
+entry-default-ns-prop.js: WARNING: Import "default" will always be undefined because there is no matching export in "foo.js"
 entry-default-ns.js: ERROR: No matching export in "foo.js" for import "default"
 entry-default-prop.js: ERROR: No matching export in "foo.js" for import "default"
-entry-default-prop.js: WARNING: Import "default" will always be undefined because there is no matching export
+entry-default-prop.js: WARNING: Import "default" will always be undefined because there is no matching export in "foo.js"
 entry-default.js: ERROR: No matching export in "foo.js" for import "default"
-entry-prop.js: WARNING: Import "default" will always be undefined because there is no matching export
+entry-prop.js: WARNING: Import "default" will always be undefined because there is no matching export in "foo.js"
+`,
+	})
+}
+
+func TestImportNamespaceUndefinedPropertyEmptyFile(t *testing.T) {
+	importstar_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry-nope.js": `
+				import * as js from './empty.js'
+				import * as mjs from './empty.mjs'
+				import * as cjs from './empty.cjs'
+				console.log(
+					js.nope,
+					mjs.nope,
+					cjs.nope,
+				)
+			`,
+
+			// Note: For CommonJS-style modules, we automatically assign the exports
+			// object to the "default" property if there is no property named "default".
+			// This is for compatibility with node. So this test intentionally behaves
+			// differently from the test above.
+			"/entry-default.js": `
+				import * as js from './empty.js'
+				import * as mjs from './empty.mjs'
+				import * as cjs from './empty.cjs'
+				console.log(
+					js.default,
+					mjs.default,
+					cjs.default,
+				)
+			`,
+
+			"/empty.js":  ``,
+			"/empty.mjs": ``,
+			"/empty.cjs": ``,
+		},
+		entryPaths: []string{
+			"/entry-nope.js",
+			"/entry-default.js",
+		},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
+		},
+		expectedCompileLog: `entry-default.js: WARNING: Import "default" will always be undefined because there is no matching export in "empty.mjs"
+entry-nope.js: WARNING: Import "nope" will always be undefined because the file "empty.js" has no exports
+entry-nope.js: WARNING: Import "nope" will always be undefined because the file "empty.mjs" has no exports
+entry-nope.js: WARNING: Import "nope" will always be undefined because the file "empty.cjs" has no exports
+`,
+	})
+}
+
+func TestImportNamespaceUndefinedPropertySideEffectFreeFile(t *testing.T) {
+	importstar_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry-nope.js": `
+				import * as js from './foo/no-side-effects.js'
+				import * as mjs from './foo/no-side-effects.mjs'
+				import * as cjs from './foo/no-side-effects.cjs'
+				console.log(
+					js.nope,
+					mjs.nope,
+					cjs.nope,
+				)
+			`,
+
+			// Note: For CommonJS-style modules, we automatically assign the exports
+			// object to the "default" property if there is no property named "default".
+			// This is for compatibility with node. So this test intentionally behaves
+			// differently from the test above.
+			"/entry-default.js": `
+				import * as js from './foo/no-side-effects.js'
+				import * as mjs from './foo/no-side-effects.mjs'
+				import * as cjs from './foo/no-side-effects.cjs'
+				console.log(
+					js.default,
+					mjs.default,
+					cjs.default,
+				)
+			`,
+
+			"/foo/package.json":        `{ "sideEffects": false }`,
+			"/foo/no-side-effects.js":  `console.log('js')`,
+			"/foo/no-side-effects.mjs": `console.log('mjs')`,
+			"/foo/no-side-effects.cjs": `console.log('cjs')`,
+		},
+		entryPaths: []string{
+			"/entry-nope.js",
+			"/entry-default.js",
+		},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
+		},
+		expectedCompileLog: `entry-default.js: WARNING: Import "default" will always be undefined because there is no matching export in "foo/no-side-effects.mjs"
+entry-nope.js: WARNING: Import "nope" will always be undefined because the file "foo/no-side-effects.js" has no exports
+entry-nope.js: WARNING: Import "nope" will always be undefined because the file "foo/no-side-effects.mjs" has no exports
+entry-nope.js: WARNING: Import "nope" will always be undefined because the file "foo/no-side-effects.cjs" has no exports
 `,
 	})
 }
diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go
@@ -2156,7 +2156,8 @@ loop:
 				// "undefined" instead of emitting an error.
 				symbol.ImportItemStatus = js_ast.ImportItemMissing
 				c.log.Add(logger.Warning, trackerFile.LineColumnTracker(), r, fmt.Sprintf(
-					"Import %q will always be undefined because there is no matching export", namedImport.Alias))
+					"Import %q will always be undefined because there is no matching export in %q",
+					namedImport.Alias, c.graph.Files[nextTracker.sourceIndex].InputFile.Source.PrettyPath))
 			} else {
 				c.log.Add(logger.Error, trackerFile.LineColumnTracker(), r, fmt.Sprintf("No matching export in %q for import %q",
 					c.graph.Files[nextTracker.sourceIndex].InputFile.Source.PrettyPath, namedImport.Alias))

diff --git a/internal/bundler/snapshots/snapshots_importstar.txt b/internal/bundler/snapshots/snapshots_importstar.txt
@@ -182,17 +182,13 @@ console.log(def, ns, ns.def);
 
 ---------- /out/external-default.js ----------
 // external-default.js
-import def, {
-  default as default2
-} from "external";
-console.log(def, default2);
+import def, * as ns from "external";
+console.log(def, ns.default);
 
 ---------- /out/external-def.js ----------
 // external-def.js
-import def, {
-  def as def2
-} from "external";
-console.log(def, def2);
+import def, * as ns from "external";
+console.log(def, ns.def);
 
 ---------- /out/internal-default2.js ----------
 // internal.js
@@ -291,6 +287,86 @@ var z = 4;
 // entry.js
 console.log(x, void 0, z);
 
+================================================================================
+TestImportNamespaceUndefinedPropertyEmptyFile
+---------- /out/entry-nope.js ----------
+// empty.js
+var require_empty = __commonJS({
+  "empty.js"() {
+  }
+});
+
+// empty.cjs
+var require_empty2 = __commonJS({
+  "empty.cjs"() {
+  }
+});
+
+// entry-nope.js
+var js = __toModule(require_empty());
+var cjs = __toModule(require_empty2());
+console.log(void 0, void 0, void 0);
+
+---------- /out/entry-default.js ----------
+// empty.js
+var require_empty = __commonJS({
+  "empty.js"() {
+  }
+});
+
+// empty.cjs
+var require_empty2 = __commonJS({
+  "empty.cjs"() {
+  }
+});
+
+// entry-default.js
+var js = __toModule(require_empty());
+var cjs = __toModule(require_empty2());
+console.log(js.default, void 0, cjs.default);
+
+================================================================================
+TestImportNamespaceUndefinedPropertySideEffectFreeFile
+---------- /out/entry-nope.js ----------
+// foo/no-side-effects.js
+var require_no_side_effects = __commonJS({
+  "foo/no-side-effects.js"() {
+    console.log("js");
+  }
+});
+
+// foo/no-side-effects.cjs
+var require_no_side_effects2 = __commonJS({
+  "foo/no-side-effects.cjs"() {
+    console.log("cjs");
+  }
+});
+
+// entry-nope.js
+var js = __toModule(require_no_side_effects());
+var cjs = __toModule(require_no_side_effects2());
+console.log(void 0, void 0, void 0);
+
+---------- /out/entry-default.js ----------
+// foo/no-side-effects.js
+var require_no_side_effects = __commonJS({
+  "foo/no-side-effects.js"() {
+    console.log("js");
+  }
+});
+
+// foo/no-side-effects.cjs
+var require_no_side_effects2 = __commonJS({
+  "foo/no-side-effects.cjs"() {
+    console.log("cjs");
+  }
+});
+
+// entry-default.js
+var js = __toModule(require_no_side_effects());
+var cjs = __toModule(require_no_side_effects2());
+console.log(js.default, void 0, cjs.default);
+
 ================================================================================
 TestImportOfExportStar
 ---------- /out.js ----------
@@ -378,8 +454,8 @@ var require_foo = __commonJS({
 
 // entry.js
 var ns = __toModule(require_foo());
-var foo = 234;
-console.log(ns, ns.foo, foo);
+var foo2 = 234;
+console.log(ns, ns.foo, foo2);
 
 ================================================================================
 TestImportStarCommonJSNoCapture

diff --git a/internal/bundler/snapshots/snapshots_importstar_ts.txt b/internal/bundler/snapshots/snapshots_importstar_ts.txt
@@ -43,8 +43,8 @@ var require_foo = __commonJS({
 
 // entry.ts
 var ns = __toModule(require_foo());
-var foo = 234;
-console.log(ns, ns.foo, foo);
+var foo2 = 234;
+console.log(ns, ns.foo, foo2);
 
 ================================================================================
 TestTSImportStarCommonJSNoCapture