From 5abe0715f9be662b182989d2f38a44c7c8b28a2d Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Tue, 6 Dec 2022 19:05:35 -0500 Subject: [PATCH] fix #2417: add the `module` condition by default --- CHANGELOG.md | 10 +++++++ pkg/api/api_impl.go | 9 ++++++- scripts/end-to-end-tests.js | 52 +++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea331be5d1a..34061d9c6b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -113,6 +113,16 @@ The legal comments feature automatically gathers comments containing `@license` or `@preserve` and puts the comments somewhere (either in the generated code or in a separate file). People sometimes want this to happen so that the their dependencies' software licenses are retained in the generated output code. By default esbuild puts these comments at the end of the file when bundling. However, people sometimes find this confusing because these comments can be very generic and may not mention which library they come from. So with this release, esbuild will now discard legal comments by default. You now have to opt-in to preserving them if you want this behavior. +* Enable the `module` condition by default ([#2417](https://github.com/evanw/esbuild/issues/2417)) + + Package authors want to be able to use the new [`exports`](https://nodejs.org/api/packages.html#conditional-exports) field in `package.json` to provide tree-shakable ESM code for ESM-aware bundlers while simultaneously providing fallback CommonJS code for other cases. + + Node's proposed way to do this involves using the `import` and `require` export conditions so that you get the ESM code if you use an import statement and the CommonJS code if you use a require call. However, this has a major drawback: if some code in the bundle uses an import statement and other code in the bundle uses a require call, then you'll get two copies of the same package in the bundle. This is known as the [dual package hazard](https://nodejs.org/api/packages.html#dual-package-hazard) and can lead to bloated bundles or even worse to subtle logic bugs. + + Webpack supports an alternate solution: an export condition called `module` that takes effect regardless of whether the package was imported using an import statement or a require call. This works because bundlers such as Webpack support importing a ESM using a require call (something node doesn't support). You could already do this with esbuild using `--conditions=module` but you previously had to explicitly enable this. Package authors are concerned that esbuild users won't know to do this and will get suboptimal output with their package, so they have requested for esbuild to do this automatically. + + So with this release, esbuild will now automatically add the `module` condition when there aren't any custom `conditions` already configured. You can disable this with `--conditions=` or `conditions: []` (i.e. explicitly clearing all custom conditions). + * Rename the `master` branch to `main` The primary branch for this repository was previously called `master` but is now called `main`. This change mirrors a similar change in many other projects. diff --git a/pkg/api/api_impl.go b/pkg/api/api_impl.go index 26de86cfb77..1998cb303a9 100644 --- a/pkg/api/api_impl.go +++ b/pkg/api/api_impl.go @@ -1005,7 +1005,6 @@ func rebuildImpl( PackageAliases: validateAlias(log, realFS, buildOpts.Alias), TsConfigOverride: validatePath(log, realFS, buildOpts.Tsconfig, "tsconfig path"), MainFields: buildOpts.MainFields, - Conditions: append([]string{}, buildOpts.Conditions...), PublicPath: buildOpts.PublicPath, KeepNames: buildOpts.KeepNames, InjectAbsPaths: make([]string, len(buildOpts.Inject)), @@ -1018,6 +1017,9 @@ func rebuildImpl( WatchMode: buildOpts.Watch != nil, Plugins: plugins, } + if buildOpts.Conditions != nil { + options.Conditions = append([]string{}, buildOpts.Conditions...) + } if options.MainFields != nil { options.MainFields = append([]string{}, options.MainFields...) } @@ -1109,6 +1111,11 @@ func rebuildImpl( options.Mode = config.ModeConvertFormat } + // Automatically enable the "module" condition for better tree shaking + if options.Conditions == nil && options.Platform != config.PlatformNeutral { + options.Conditions = []string{"module"} + } + // Code splitting is experimental and currently only enabled for ES6 modules if options.CodeSplitting && options.OutputFormat != config.FormatESModule { log.AddError(nil, logger.Range{}, "Splitting currently only works with the \"esm\" format") diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 8521604b9e3..462f07718c4 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -6116,6 +6116,58 @@ for (const flags of [[], ['--bundle']]) { } }`, }), + test(['in.js', '--outfile=node.js', '--bundle', '--platform=browser'].concat(flags), { + 'in.js': `import abc from 'pkg'; if (abc !== 'module') throw 'fail'`, + 'node_modules/pkg/default.js': `module.exports = 'default'`, + 'node_modules/pkg/module.js': `export default 'module'`, + 'node_modules/pkg/package.json': `{ + "exports": { + ".": { + "module": "./module.js", + "default": "./default.js" + } + } + }`, + }), + test(['in.js', '--outfile=node.js', '--bundle', '--platform=node'].concat(flags), { + 'in.js': `import abc from 'pkg'; if (abc !== 'module') throw 'fail'`, + 'node_modules/pkg/default.js': `module.exports = 'default'`, + 'node_modules/pkg/module.js': `export default 'module'`, + 'node_modules/pkg/package.json': `{ + "exports": { + ".": { + "module": "./module.js", + "default": "./default.js" + } + } + }`, + }), + test(['in.js', '--outfile=node.js', '--bundle', '--platform=neutral'].concat(flags), { + 'in.js': `import abc from 'pkg'; if (abc !== 'default') throw 'fail'`, + 'node_modules/pkg/default.js': `module.exports = 'default'`, + 'node_modules/pkg/module.js': `export default 'module'`, + 'node_modules/pkg/package.json': `{ + "exports": { + ".": { + "module": "./module.js", + "default": "./default.js" + } + } + }`, + }), + test(['in.js', '--outfile=node.js', '--bundle', '--conditions='].concat(flags), { + 'in.js': `import abc from 'pkg'; if (abc !== 'default') throw 'fail'`, + 'node_modules/pkg/default.js': `module.exports = 'default'`, + 'node_modules/pkg/module.js': `export default 'module'`, + 'node_modules/pkg/package.json': `{ + "exports": { + ".": { + "module": "./module.js", + "default": "./default.js" + } + } + }`, + }), ) // Node 17+ deliberately broke backward compatibility with packages using mappings