fix #1874: node defaults to --packages=external

Author: evanw

CHANGELOG.md

@@ -2,6 +2,14 @@
 
 ## Unreleased
 
+* Omit packages from bundle by default when targeting node ([#1874](https://github.com/evanw/esbuild/issues/1874), [#2830](https://github.com/evanw/esbuild/issues/2830), [#2846](https://github.com/evanw/esbuild/issues/2846), [#2915](https://github.com/evanw/esbuild/issues/2915), [#3145](https://github.com/evanw/esbuild/issues/3145), [#3294](https://github.com/evanw/esbuild/issues/3294), [#3323](https://github.com/evanw/esbuild/issues/3323), [#3582](https://github.com/evanw/esbuild/issues/3582), [#3809](https://github.com/evanw/esbuild/issues/3809), [#3815](https://github.com/evanw/esbuild/issues/3815))
+
+    This breaking change is an experiment. People are commonly confused when using esbuild to bundle code for node (i.e. for `--platform=node`) because some packages may not be intended for bundlers, and may use node-specific features that don't work with a bundler. Even though esbuild's "getting started" instructions say to use `--packages=external` to work around this problem, many people don't read the documentation and don't do this, and are then confused when it doesn't work. So arguably this is a bad default behavior for esbuild to have if people keep tripping over this.
+
+    With this release, esbuild will now omit packages from the bundle by default when the platform is `node` (i.e. the previous behavior of `--packages=external` is now the default in this case). If you don't want this behavior, you can do `--packages=bundle` to allow packages to be included in the bundle (i.e. the previous default behavior). Note that `--packages=bundle` doesn't mean all packages are bundled, just that packages are allowed to be bundled. You can still exclude individual packages from the bundle using `--external:` even when `--packages=bundle` is present.
+
+    The `--packages=` setting considers all import paths that "look like" package imports in the original source code to be packages. Specifically packages that don't start with a path segment of `/` or `.` or `..` are considered to be package imports. The only two exceptions to this rule are [subpath imports](https://nodejs.org/api/packages.html#subpath-imports) (which start with a `#` character) and TypeScript path remappings via `path` and/or `baseUrl` in `tsconfig.json` (which are applied first).
+
 * Drop support for older platforms ([#3802](https://github.com/evanw/esbuild/issues/3802))
 
     This release drops support for the following operating systems:

Makefile

@@ -753,7 +753,7 @@ test-tsc: esbuild | github/tsc
 	cp -r github/tsc/src github/tsc/scripts demo/tsc
 	cp github/tsc/lib/*.d.ts demo/tsc/built/local
 	cd demo/tsc && node scripts/processDiagnosticMessages.mjs src/compiler/diagnosticMessages.json
-	./esbuild --bundle demo/tsc/src/tsc/tsc.ts --outfile=demo/tsc/built/local/tsc.js --platform=node --target=es2018 --packages=external
+	./esbuild --bundle demo/tsc/src/tsc/tsc.ts --outfile=demo/tsc/built/local/tsc.js --platform=node --target=es2018
 	echo '{"dependencies":{"@types/node":"20.2.5","@types/microsoft__typescript-etw":"0.1.1","@types/source-map-support":"0.5.6"}}' > demo/tsc/package.json
 	cd demo/tsc && npm i --silent && echo 'Type checking tsc using tsc...' && time -p node ./built/local/tsc.js -p src/compiler
 
@@ -769,6 +769,7 @@ TEST_ROLLUP_REPLACE += "paths": { "package.json": [".\/package.json"] },
 TEST_ROLLUP_FLAGS += --bundle
 TEST_ROLLUP_FLAGS += --external:fsevents
 TEST_ROLLUP_FLAGS += --outfile=dist/rollup.js
+TEST_ROLLUP_FLAGS += --packages=bundle
 TEST_ROLLUP_FLAGS += --platform=node
 TEST_ROLLUP_FLAGS += --target=es6
 TEST_ROLLUP_FLAGS += src/node-entry.ts

lib/shared/types.ts

@@ -125,7 +125,7 @@ export interface BuildOptions extends CommonOptions {
   /** Documentation: https://esbuild.github.io/api/#external */
   external?: string[]
   /** Documentation: https://esbuild.github.io/api/#packages */
-  packages?: 'external'
+  packages?: 'bundle' | 'external'
   /** Documentation: https://esbuild.github.io/api/#alias */
   alias?: Record<string, string>
   /** Documentation: https://esbuild.github.io/api/#loader */

pkg/api/api.go

@@ -180,6 +180,7 @@ type Packages uint8
 
 const (
 	PackagesDefault Packages = iota
+	PackagesBundle
 	PackagesExternal
 )
 

pkg/api/api_impl.go

@@ -216,6 +216,19 @@ func validateASCIIOnly(value Charset) bool {
 	}
 }
 
+func validateExternalPackages(value Packages, platform Platform) bool {
+	switch value {
+	case PackagesDefault:
+		return platform == PlatformNode
+	case PackagesBundle:
+		return false
+	case PackagesExternal:
+		return true
+	default:
+		panic("Invalid packages")
+	}
+}
+
 func validateTreeShaking(value TreeShaking, bundle bool, format Format) bool {
 	switch value {
 	case TreeShakingDefault:
@@ -1267,7 +1280,7 @@ func validateBuildOptions(
 		ExtensionToLoader:     validateLoaders(log, buildOpts.Loader),
 		ExtensionOrder:        validateResolveExtensions(log, buildOpts.ResolveExtensions),
 		ExternalSettings:      validateExternals(log, realFS, buildOpts.External),
-		ExternalPackages:      buildOpts.Packages == PackagesExternal,
+		ExternalPackages:      validateExternalPackages(buildOpts.Packages, buildOpts.Platform),
 		PackageAliases:        validateAlias(log, realFS, buildOpts.Alias),
 		TSConfigPath:          validatePath(log, realFS, buildOpts.Tsconfig, "tsconfig path"),
 		TSConfigRaw:           buildOpts.TsconfigRaw,

pkg/cli/cli_impl.go

@@ -613,12 +613,15 @@ func parseOptionsImpl(
 		case strings.HasPrefix(arg, "--packages=") && buildOpts != nil:
 			value := arg[len("--packages="):]
 			var packages api.Packages
-			if value == "external" {
+			switch value {
+			case "bundle":
+				packages = api.PackagesBundle
+			case "external":
 				packages = api.PackagesExternal
-			} else {
+			default:
 				return parseOptionsExtras{}, cli_helpers.MakeErrorWithNote(
 					fmt.Sprintf("Invalid value %q in %q", value, arg),
-					"The only valid value is \"external\".",
+					"Valid values are \"bundle\" or \"external\".",
 				)
 			}
 			buildOpts.Packages = packages

scripts/end-to-end-tests.js

@@ -8069,7 +8069,7 @@ for (const flags of [[], ['--bundle']]) {
         }
       }`,
     }),
-    test(['in.js', '--outfile=node.js', '--bundle', '--platform=node'].concat(flags), {
+    test(['in.js', '--outfile=node.js', '--bundle', '--platform=node', '--packages=bundle'].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'`,
@@ -8109,6 +8109,38 @@ for (const flags of [[], ['--bundle']]) {
       }`,
     }),
 
+    // Check the default behavior of "--platform=node"
+    test(['in.js', '--outfile=node.js', '--bundle', '--platform=node', '--format=esm'].concat(flags), {
+      'in.js': `import abc from 'pkg'; if (abc !== 'import') throw 'fail'`,
+      'node_modules/pkg/fail.js': `TEST FAILED`, // This package should not be bundled
+      'node_modules/pkg/require.cjs': `module.exports = 'require'`,
+      'node_modules/pkg/import.mjs': `export default 'import'`,
+      'node_modules/pkg/package.json': `{
+        "exports": {
+          ".": {
+            "module": "./fail.js",
+            "import": "./import.mjs",
+            "require": "./require.cjs"
+          }
+        }
+      }`,
+    }),
+    test(['in.js', '--outfile=node.js', '--bundle', '--platform=node', '--format=cjs'].concat(flags), {
+      'in.js': `import abc from 'pkg'; if (abc !== 'require') throw 'fail'`,
+      'node_modules/pkg/fail.js': `TEST FAILED`, // This package should not be bundled
+      'node_modules/pkg/require.cjs': `module.exports = 'require'`,
+      'node_modules/pkg/import.mjs': `export default 'import'`,
+      'node_modules/pkg/package.json': `{
+        "exports": {
+          ".": {
+            "module": "./fail.js",
+            "import": "./import.mjs",
+            "require": "./require.cjs"
+          }
+        }
+      }`,
+    }),
+
     // This is an edge case for extensionless files. The file should be treated
     // as CommonJS even though package.json says "type": "module" because that
     // only applies to ".js" files in node, not to all JavaScript files.

scripts/test-yarnpnp.js

@@ -66,19 +66,20 @@ function runTests() {
     'in.mjs',
     '--bundle',
     '--log-level=debug',
+    '--packages=bundle',
     '--platform=node',
     '--outfile=out-native.js',
   ], { cwd: rootDir, stdio: 'inherit' })
   run('node out-native.js')
 
   // Test the WebAssembly build
   esbuild.buildWasmLib(ESBUILD_BINARY_PATH)
-  run('node ../../npm/esbuild-wasm/bin/esbuild in.mjs --bundle --log-level=debug --platform=node --outfile=out-wasm.js')
+  run('node ../../npm/esbuild-wasm/bin/esbuild in.mjs --bundle --log-level=debug --packages=bundle --platform=node --outfile=out-wasm.js')
   run('node out-wasm.js')
 
   // Test the WebAssembly build when run through Yarn's file system shim
   esbuild.buildWasmLib(ESBUILD_BINARY_PATH)
-  run('yarn node ../../npm/esbuild-wasm/bin/esbuild in.mjs --bundle --log-level=debug --platform=node --outfile=out-wasm-yarn.js')
+  run('yarn node ../../npm/esbuild-wasm/bin/esbuild in.mjs --bundle --log-level=debug --packages=bundle --platform=node --outfile=out-wasm-yarn.js')
   run('node out-wasm-yarn.js')
 }