diff --git a/CHANGELOG.md b/CHANGELOG.md index ec31861d912..a6937f33c47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,16 @@ This initial release only has limited support for top-level await. It is only supported with the `esm` output format, but not with the `iife` or `cjs` output formats. In addition, the compilation is not correct in that two modules that both contain top-level await and that are siblings in the import graph will be evaluated in serial instead of in parallel. Full support for top-level await will come in a future release. +* Change whether certain files are interpreted as ESM or CommonJS ([#1043](https://github.com/evanw/esbuild/issues/1043)) + + The bundling algorithm currently doesn't contain any logic that requires flagging modules as CommonJS vs. ESM beforehand. Instead it handles a superset and then sort of decides later if the module should be treated as CommonJS vs. ESM based on whether the module uses the `module` or `exports` variables and/or the `exports` keyword. + + With this release, files that follow [node's rules for module types](https://nodejs.org/api/packages.html#packages_type) will be flagged as explicitly ESM. This includes files that end in `.mjs` and files within a package containing `"type": "module"` in the enclosing `package.json` file. The CommonJS `require`, `module`, and `exports` features will be unavailable in these files. This matters most for files without any exports, since then it's otherwise ambiguous what the module type is. + + In addition, files without exports should now accurately fall back to being considered CommonJS. They should now generate a `default` export of an empty object when imported using an `import` statement, since that's what happens in node when you import a CommonJS file into an ESM file in node. Previously the default export could be undefined because these export-less files were sort of treated as ESM but with missing import errors turned into warnings instead. + + This is an edge case that rarely comes up in practice, since you usually never import things from a module that has no exports. + ## Unreleased * Add the ability to set `sourceRoot` in source maps ([#1028](https://github.com/evanw/esbuild/pull/1028)) diff --git a/internal/ast/ast.go b/internal/ast/ast.go index 879e80f67bf..6eb5d1d53b2 100644 --- a/internal/ast/ast.go +++ b/internal/ast/ast.go @@ -77,6 +77,10 @@ type ImportRecord struct { // CommonJS wrapper or not. ContainsImportStar bool + // If this is true, the import contains an import for the alias "default", + // either via the "import x from" or "import {default as x} from" syntax. + ContainsDefaultAlias bool + // If true, this "export * from 'path'" statement is evaluated at run-time by // calling the "__exportStar()" helper function CallsRunTimeExportStarFn bool diff --git a/internal/bundler/bundler.go b/internal/bundler/bundler.go index 7b683ab0d04..2a61da18f87 100644 --- a/internal/bundler/bundler.go +++ b/internal/bundler/bundler.go @@ -1023,6 +1023,15 @@ func (s *scanner) maybeParseFile( optionsClone.PreserveUnusedImportsTS = true } + // Set the module type preference using node's module type rules + if strings.HasSuffix(path.Text, ".mjs") { + optionsClone.ModuleType = config.ModuleESM + } else if strings.HasSuffix(path.Text, ".cjs") { + optionsClone.ModuleType = config.ModuleCommonJS + } else { + optionsClone.ModuleType = resolveResult.ModuleType + } + // Enable bundling for injected files so we always do tree shaking. We // never want to include unnecessary code from injected files since they // are essentially bundled. However, if we do this we should skip the diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index 29aecc6b83c..3e3869e1905 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -693,10 +693,8 @@ func TestImportMissingNeitherES6NorCommonJS(t *testing.T) { Mode: config.ModeBundle, AbsOutputDir: "/out", }, - expectedCompileLog: `named.js: warning: Import "default" will always be undefined because the file "foo.js" has no exports -named.js: warning: Import "x" will always be undefined because the file "foo.js" has no exports + expectedCompileLog: `named.js: warning: Import "x" will always be undefined because the file "foo.js" has no exports named.js: warning: Import "y" will always be undefined because the file "foo.js" has no exports -star.js: warning: Import "default" will always be undefined because the file "foo.js" has no exports star.js: warning: Import "x" will always be undefined because the file "foo.js" has no exports star.js: warning: Import "y" will always be undefined because the file "foo.js" has no exports `, diff --git a/internal/bundler/bundler_splitting_test.go b/internal/bundler/bundler_splitting_test.go index 40a4cb0f55d..a986b499499 100644 --- a/internal/bundler/bundler_splitting_test.go +++ b/internal/bundler/bundler_splitting_test.go @@ -272,7 +272,7 @@ func TestSplittingMissingLazyExport(t *testing.T) { OutputFormat: config.FormatESModule, AbsOutputDir: "/out", }, - expectedCompileLog: `common.js: warning: Import "missing" will always be undefined because there is no matching export + expectedCompileLog: `common.js: warning: Import "missing" will always be undefined because the file "empty.js" has no exports `, }) } diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go index 2a84e53d106..f56440d6208 100644 --- a/internal/bundler/linker.go +++ b/internal/bundler/linker.go @@ -1278,14 +1278,16 @@ func (c *linkerContext) scanImportsAndExports() { // We emit a warning in this case but try to avoid turning the module // into a CommonJS module if possible. This is possible with named // imports (the module stays an ECMAScript module but the imports are - // rewritten with undefined) but is not possible with star imports: + // rewritten with undefined) but is not possible with star or default + // imports: // // import * as ns from './empty-file' - // console.log(ns) + // import defVal from './empty-file' + // console.log(ns, defVal) // // In that case the module *is* considered a CommonJS module because // the namespace object must be created. - if record.ContainsImportStar && otherRepr.ast.ExportsKind == js_ast.ExportsNone && !otherRepr.ast.HasLazyExport { + if (record.ContainsImportStar || record.ContainsDefaultAlias) && otherRepr.ast.ExportsKind == js_ast.ExportsNone && !otherRepr.ast.HasLazyExport { otherRepr.ast.ExportsKind = js_ast.ExportsCommonJS } @@ -2498,7 +2500,11 @@ func (c *linkerContext) advanceImportTracker(tracker importTracker) (importTrack // Is this a named import of a file without any exports? otherRepr := c.files[otherSourceIndex].repr.(*reprJS) - if !namedImport.AliasIsStar && otherRepr.ast.ExportsKind == js_ast.ExportsNone && !otherRepr.ast.HasLazyExport { + if !namedImport.AliasIsStar && !otherRepr.ast.HasLazyExport && + // CommonJS exports + otherRepr.ast.ExportKeyword.Len == 0 && namedImport.Alias != "default" && + // ESM exports + !otherRepr.ast.UsesExportsRef && !otherRepr.ast.UsesModuleRef { // Just warn about it and replace the import with "undefined" return importTracker{sourceIndex: otherSourceIndex, importRef: js_ast.InvalidRef}, importCommonJSWithoutExports, nil } diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index fd51f2ea481..569fdd40feb 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -977,17 +977,23 @@ console.log((0, import_foo.default)(import_foo.x, import_foo.y)); TestImportMissingNeitherES6NorCommonJS ---------- /out/named.js ---------- // foo.js -console.log("no exports here"); +var require_foo = __commonJS(() => { + console.log("no exports here"); +}); // named.js -console.log((void 0)(void 0, void 0)); +var import_foo = __toModule(require_foo()); +console.log((0, import_foo.default)(void 0, void 0)); ---------- /out/star.js ---------- // foo.js -console.log("no exports here"); +var require_foo = __commonJS(() => { + console.log("no exports here"); +}); // star.js -console.log((void 0)(void 0, void 0)); +var ns = __toModule(require_foo()); +console.log(ns.default(void 0, void 0)); ---------- /out/star-capture.js ---------- // foo.js diff --git a/internal/config/config.go b/internal/config/config.go index 3cc9c8a8be6..e30addce1bd 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -160,8 +160,17 @@ const ( ModeBundle ) +type ModuleType uint8 + +const ( + ModuleUnknown ModuleType = iota + ModuleCommonJS + ModuleESM +) + type Options struct { Mode Mode + ModuleType ModuleType PreserveSymlinks bool RemoveWhitespace bool MinifyIdentifiers bool diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 1a29fed3fb0..080f2a9145d 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -286,6 +286,7 @@ type optionsThatSupportStructuralEquality struct { mode config.Mode platform config.Platform outputFormat config.Format + moduleType config.ModuleType asciiOnly bool keepNames bool mangleSyntax bool @@ -309,6 +310,7 @@ func OptionsFromConfig(options *config.Options) Options { mode: options.Mode, platform: options.Platform, outputFormat: options.OutputFormat, + moduleType: options.ModuleType, asciiOnly: options.ASCIIOnly, keepNames: options.KeepNames, mangleSyntax: options.MangleSyntax, @@ -11705,6 +11707,7 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result scanForIm for _, stmt := range stmts { switch s := stmt.Data.(type) { case *js_ast.SImport: + record := &p.importRecords[s.ImportRecordIndex] // The official TypeScript compiler always removes unused imported // symbols. However, we deliberately deviate from the official @@ -11852,7 +11855,7 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result scanForIm if p.options.ts.Parse && foundImports && isUnusedInTypeScript && !p.options.preserveUnusedImportsTS { // 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 := &p.importRecords[s.ImportRecordIndex]; !record.SourceIndex.IsValid() { + if !record.SourceIndex.IsValid() { record.IsUnused = true continue } @@ -11983,7 +11986,17 @@ func (p *parser) scanForImportsAndExports(stmts []js_ast.Stmt) (result scanForIm p.importRecordsForCurrentPart = append(p.importRecordsForCurrentPart, s.ImportRecordIndex) if s.StarNameLoc != nil { - p.importRecords[s.ImportRecordIndex].ContainsImportStar = true + record.ContainsImportStar = true + } + + if s.DefaultName != nil { + record.ContainsDefaultAlias = true + } else if s.Items != nil { + for _, item := range *s.Items { + if item.Alias == "default" { + record.ContainsDefaultAlias = true + } + } } case *js_ast.SFunction: @@ -12886,7 +12899,8 @@ func (p *parser) prepareForVisitPass() { // CommonJS-style exports are only enabled if this isn't using ECMAScript- // style exports. You can still use "require" in ESM, just not "module" or // "exports". You can also still use "import" in CommonJS. - if p.options.mode != config.ModePassThrough && p.es6ExportKeyword.Len == 0 && p.topLevelAwaitKeyword.Len == 0 { + if p.options.moduleType != config.ModuleESM && p.options.mode != config.ModePassThrough && + p.es6ExportKeyword.Len == 0 && p.topLevelAwaitKeyword.Len == 0 { p.exportsRef = p.declareCommonJSSymbol(js_ast.SymbolHoisted, "exports") p.moduleRef = p.declareCommonJSSymbol(js_ast.SymbolHoisted, "module") } else { @@ -13161,8 +13175,27 @@ func (p *parser) toAST(source logger.Source, parts []js_ast.Part, hashbang strin exportsKind = js_ast.ExportsESM } else if usesExportsRef || usesModuleRef || p.hasTopLevelReturn { exportsKind = js_ast.ExportsCommonJS - } else if p.es6ImportKeyword.Len > 0 { - exportsKind = js_ast.ExportsESM + } else { + // If this module has no exports, try to determine what kind of module it + // is by looking at node's "type" field in "package.json" and/or whether + // the file extension is ".mjs" or ".cjs". + switch p.options.moduleType { + case config.ModuleCommonJS: + // "type: module" or ".mjs" + exportsKind = js_ast.ExportsCommonJS + + case config.ModuleESM: + // "type: commonjs" or ".cjs" + exportsKind = js_ast.ExportsESM + + case config.ModuleUnknown: + // Treat unknown modules containing an import statement as ESM. Otherwise + // the bundler will treat this file as CommonJS if it's imported and ESM + // if it's not imported. + if p.es6ImportKeyword.Len > 0 { + exportsKind = js_ast.ExportsESM + } + } } return js_ast.AST{ diff --git a/internal/resolver/package_json.go b/internal/resolver/package_json.go index 424a643395d..d3d3274cc0d 100644 --- a/internal/resolver/package_json.go +++ b/internal/resolver/package_json.go @@ -19,6 +19,7 @@ import ( type packageJSON struct { source logger.Source absMainFields map[string]string + moduleType config.ModuleType // Present if the "browser" field is present. This field is intended to be // used by bundlers and lets you redirect the paths of certain 3rd-party @@ -110,6 +111,24 @@ func (r *resolver) parsePackageJSON(path string) *packageJSON { packageJSON := &packageJSON{source: jsonSource} + // Read the "type" field + if typeJSON, _, ok := getProperty(json, "type"); ok { + if typeValue, ok := getString(typeJSON); ok { + switch typeValue { + case "commonjs": + packageJSON.moduleType = config.ModuleCommonJS + case "module": + packageJSON.moduleType = config.ModuleESM + default: + r.log.AddRangeWarning(&jsonSource, jsonSource.RangeOfString(typeJSON.Loc), + fmt.Sprintf("%q is not a valid value for the \"type\" field (must be either \"commonjs\" or \"module\")", typeValue)) + } + } else { + r.log.AddWarning(&jsonSource, typeJSON.Loc, + "The value for \"type\" must be a string") + } + } + // Read the "main" fields mainFields := r.options.MainFields if mainFields == nil { diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go index acbbd263c11..9a548648aa1 100644 --- a/internal/resolver/resolver.go +++ b/internal/resolver/resolver.go @@ -94,13 +94,14 @@ type ResolveResult struct { JSXFactory []string // Default if empty: "React.createElement" JSXFragment []string // Default if empty: "React.Fragment" - IsExternal bool DifferentCase *fs.DifferentCase // If true, any ES6 imports to this file can be considered to have no side // effects. This means they should be removed if unused. IgnorePrimaryIfUnused *IgnoreIfUnusedData + IsExternal bool + // If true, the class field transform should use Object.defineProperty(). UseDefineForClassFieldsTS bool @@ -108,6 +109,9 @@ type ResolveResult struct { // behavior of the "importsNotUsedAsValues" field in "tsconfig.json" when the // value is not "remove". PreserveUnusedImportsTS bool + + // This is the "type" field from "package.json" + ModuleType config.ModuleType } type AlternativeApproach uint8 @@ -414,6 +418,9 @@ func (r *resolver) finalizeResolve(result *ResolveResult) { result.IgnorePrimaryIfUnused = info.packageJSON.ignoreIfUnusedData } } + + // Also copy over the "type" field + result.ModuleType = info.packageJSON.moduleType break } } diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index bf0782a8ab7..eea5d6436fa 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -780,6 +780,63 @@ }), ) + // Test imports from modules without any imports + tests.push( + test(['in.js', '--outfile=node.js', '--bundle'], { + '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'], { + 'in.js': ` + import * as ns from 'pkg/index.cjs' + if (ns.default === void 0) throw 'fail' + `, + 'node_modules/pkg/index.cjs': ``, + }), + test(['in.js', '--outfile=node.js', '--bundle'], { + 'in.js': ` + import * as ns from 'pkg/index.mjs' + if (ns.default !== void 0) throw 'fail' + `, + 'node_modules/pkg/index.mjs': ``, + }, { + expectedStderr: ` > in.js:3:15: warning: Import "default" will always be undefined because there is no matching export + 3 │ if (ns.default !== void 0) throw 'fail' + ╵ ~~~~~~~ + +`, + }), + test(['in.js', '--outfile=node.js', '--bundle'], { + 'in.js': ` + import * as ns from 'pkg' + if (ns.default === void 0) throw 'fail' + `, + 'node_modules/pkg/package.json': `{ + "type": "commonjs" + }`, + 'node_modules/pkg/index.js': ``, + }), + test(['in.js', '--outfile=node.js', '--bundle'], { + 'in.js': ` + import * as ns from 'pkg' + if (ns.default !== void 0) throw 'fail' + `, + 'node_modules/pkg/package.json': `{ + "type": "module" + }`, + 'node_modules/pkg/index.js': ``, + }, { + expectedStderr: ` > in.js:3:15: warning: Import "default" will always be undefined because there is no matching export + 3 │ if (ns.default !== void 0) throw 'fail' + ╵ ~~~~~~~ + +`, + }), + ) + // Test imports not being able to access the namespace object tests.push( test(['in.js', '--outfile=node.js', '--bundle'], {