diff --git a/CHANGELOG.md b/CHANGELOG.md index 93742b32034..54b97d03097 100644 --- 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 index d58cf2de2b7..0d2d3e374e3 100644 --- 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 index d52ae925ff4..11ac73ac165 100644 --- 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 index d3b474857d9..2fa440a5897 100644 --- 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 index 59281fe0faf..a645578d140 100644 --- 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 index 0ff0a4304e0..6bbade4d118 100644 --- 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 diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index a6e142102cd..85abe368f63 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -13538,36 +13538,6 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result importsEx if p.options.mode != config.ModePassThrough { if s.StarNameLoc != nil { - // If we're bundling a star import and the namespace is only ever - // used for property accesses, then convert each unique property to - // a clause item in the import statement and remove the star import. - // That will cause the bundler to bundle them more efficiently when - // both this module and the imported module are in the same group. - // - // Before: - // - // import * as ns from 'foo' - // console.log(ns.a, ns.b) - // - // After: - // - // import {a, b} from 'foo' - // console.log(a, b) - // - // This is not done if the namespace itself is used, because in that - // case the code for the namespace will have to be generated. This is - // determined by the symbol count because the parser only counts the - // star import as used if it was used for something other than a - // property access: - // - // import * as ns from 'foo' - // console.log(ns, ns.a, ns.b) - // - convertStarToClause := p.symbols[s.NamespaceRef.InnerIndex].UseCountEstimate == 0 - if convertStarToClause && !keepUnusedImports { - s.StarNameLoc = nil - } - // "importItemsForNamespace" has property accesses off the namespace if importItems, ok := p.importItemsForNamespace[s.NamespaceRef]; ok && len(importItems) > 0 { // Sort keys for determinism @@ -13577,52 +13547,32 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result importsEx } sort.Strings(sorted) - if convertStarToClause { - // Create an import clause for these items. Named imports will be - // automatically created later on since there is now a clause. - items := make([]js_ast.ClauseItem, 0, len(importItems)) - for _, alias := range sorted { - name := importItems[alias] - originalName := p.symbols[name.Ref.InnerIndex].OriginalName - items = append(items, js_ast.ClauseItem{ - Alias: alias, - AliasLoc: name.Loc, - Name: name, - OriginalName: originalName, - }) - p.declaredSymbols = append(p.declaredSymbols, js_ast.DeclaredSymbol{ - Ref: name.Ref, - IsTopLevel: true, - }) - } - if s.Items != nil { - // The syntax "import {x}, * as y from 'path'" isn't valid - panic("Internal error") + // Create named imports for these property accesses. This will + // cause missing imports to generate useful warnings. + // + // It will also improve bundling efficiency for internal imports + // by still converting property accesses off the namespace into + // bare identifiers even if the namespace is still needed. + for _, alias := range sorted { + name := importItems[alias] + p.namedImports[name.Ref] = js_ast.NamedImport{ + Alias: alias, + AliasLoc: name.Loc, + NamespaceRef: s.NamespaceRef, + ImportRecordIndex: s.ImportRecordIndex, } - s.Items = &items - } else { - // If we aren't converting this star import to a clause, still - // create named imports for these property accesses. This will - // cause missing imports to generate useful warnings. - // - // It will also improve bundling efficiency for internal imports - // by still converting property accesses off the namespace into - // bare identifiers even if the namespace is still needed. - for _, alias := range sorted { - name := importItems[alias] - p.namedImports[name.Ref] = js_ast.NamedImport{ - Alias: alias, - AliasLoc: name.Loc, - NamespaceRef: s.NamespaceRef, - ImportRecordIndex: s.ImportRecordIndex, - } - // Make sure the printer prints this as a property access - p.symbols[name.Ref.InnerIndex].NamespaceAlias = &js_ast.NamespaceAlias{ - NamespaceRef: s.NamespaceRef, - Alias: alias, - } + // Make sure the printer prints this as a property access + p.symbols[name.Ref.InnerIndex].NamespaceAlias = &js_ast.NamespaceAlias{ + NamespaceRef: s.NamespaceRef, + Alias: alias, } + + // Also record these automatically-generated top-level namespace alias symbols + p.declaredSymbols = append(p.declaredSymbols, js_ast.DeclaredSymbol{ + Ref: name.Ref, + IsTopLevel: true, + }) } } } diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 19ab8b2d0ad..cc0f58ee551 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -1480,7 +1480,7 @@ `, 'node_modules/pkg/index.mjs': ``, }, { - expectedStderr: `▲ [WARNING] Import "default" will always be undefined because there is no matching export + expectedStderr: `▲ [WARNING] Import "default" will always be undefined because there is no matching export in "node_modules/pkg/index.mjs" in.js:3:15: 3 │ if (ns.default !== void 0) throw 'fail' @@ -1495,7 +1495,7 @@ `, 'node_modules/pkg/index.mts': ``, }, { - expectedStderr: `▲ [WARNING] Import "default" will always be undefined because there is no matching export + expectedStderr: `▲ [WARNING] Import "default" will always be undefined because there is no matching export in "node_modules/pkg/index.mts" in.js:3:15: 3 │ if (ns.default !== void 0) throw 'fail' @@ -1523,7 +1523,7 @@ }`, 'node_modules/pkg/index.js': ``, }, { - expectedStderr: `▲ [WARNING] Import "default" will always be undefined because there is no matching export + expectedStderr: `▲ [WARNING] Import "default" will always be undefined because there is no matching export in "node_modules/pkg/index.js" in.js:3:15: 3 │ if (ns.default !== void 0) throw 'fail' @@ -1531,6 +1531,20 @@ `, }), + test(['in.js', '--outfile=node.js', '--bundle', '--external:pkg'], { + 'in.js': ` + import * as ns from 'pkg' + if (ns.default === void 0) throw 'fail' + `, + 'node_modules/pkg/index.js': ``, + }), + test(['in.js', '--outfile=node.js', '--bundle', '--external:pkg'], { + 'in.js': ` + import * as ns from 'pkg' + if (ns.foo !== void 0) throw 'fail' + `, + 'node_modules/pkg/index.js': ``, + }), ) // Test imports not being able to access the namespace object