Revert "fix #1874: node defaults to --packages=external" (#3820)

This reverts commit 196dcad.

Author: evanw

CHANGELOG.md

@@ -2,6 +2,14 @@
 
 ## Unreleased
 
+* Revert the recent change to avoid bundling dependencies for node ([#3819](https://github.com/evanw/esbuild/issues/3819))
+
+    This release reverts the recent change in version 0.22.0 that made `--packages=external` the default behavior with `--platform=node`.  The default is now back to `--packages=bundle`.
+
+    I've just been made aware that Amazon doesn't pin their dependencies in their "AWS CDK" product, which means that whenever esbuild publishes a new release, many people (potentially everyone?) using their SDK around the world instantly starts using it without Amazon checking that it works first. This change in version 0.22.0 happened to break their SDK. I'm amazed that things haven't broken before this point. This revert attempts to avoid these problems for Amazon's customers. Hopefully Amazon will pin their dependencies in the future.
+
+    In addition, this is probably a sign that esbuild is used widely enough that it now needs to switch to a more complicated release model. I may have esbuild use a beta channel model for further development.
+
 * Fix preserving collapsed JSX whitespace ([#3818](https://github.com/evanw/esbuild/issues/3818))
 
     When transformed, certain whitespace inside JSX elements is ignored completely if it collapses to an empty string. However, the whitespace should only be ignored if the JSX is being transformed, not if it's being preserved. This release fixes a bug where esbuild was previously incorrectly ignoring collapsed whitespace with `--jsx=preserve`. Here is an example:

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
+	./esbuild --bundle demo/tsc/src/tsc/tsc.ts --outfile=demo/tsc/built/local/tsc.js --platform=node --target=es2018 --packages=external
 	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,7 +769,6 @@ 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

pkg/api/api_impl.go

@@ -216,11 +216,9 @@ func validateASCIIOnly(value Charset) bool {
 	}
 }
 
-func validateExternalPackages(value Packages, platform Platform) bool {
+func validateExternalPackages(value Packages) bool {
 	switch value {
-	case PackagesDefault:
-		return platform == PlatformNode
-	case PackagesBundle:
+	case PackagesDefault, PackagesBundle:
 		return false
 	case PackagesExternal:
 		return true
@@ -1280,7 +1278,7 @@ func validateBuildOptions(
 		ExtensionToLoader:     validateLoaders(log, buildOpts.Loader),
 		ExtensionOrder:        validateResolveExtensions(log, buildOpts.ResolveExtensions),
 		ExternalSettings:      validateExternals(log, realFS, buildOpts.External),
-		ExternalPackages:      validateExternalPackages(buildOpts.Packages, buildOpts.Platform),
+		ExternalPackages:      validateExternalPackages(buildOpts.Packages),
 		PackageAliases:        validateAlias(log, realFS, buildOpts.Alias),
 		TSConfigPath:          validatePath(log, realFS, buildOpts.Tsconfig, "tsconfig path"),
 		TSConfigRaw:           buildOpts.TsconfigRaw,

scripts/end-to-end-tests.js

@@ -8108,9 +8108,7 @@ 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), {
+    test(['in.js', '--outfile=node.js', '--bundle', '--platform=node', '--packages=external', '--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'`,
@@ -8125,7 +8123,7 @@ for (const flags of [[], ['--bundle']]) {
         }
       }`,
     }),
-    test(['in.js', '--outfile=node.js', '--bundle', '--platform=node', '--format=cjs'].concat(flags), {
+    test(['in.js', '--outfile=node.js', '--bundle', '--platform=node', '--packages=external', '--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'`,
@@ -8141,6 +8139,23 @@ 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 !== 'module') throw 'fail'`,
+      'node_modules/pkg/module.js': `export default 'module'`,
+      'node_modules/pkg/require.cjs': `module.exports = 'require'`,
+      'node_modules/pkg/import.mjs': `export default 'import'`,
+      'node_modules/pkg/package.json': `{
+        "exports": {
+          ".": {
+            "module": "./module.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

@@ -7,7 +7,7 @@ const ESBUILD_BINARY_PATH = esbuild.buildBinary()
 const rootDir = path.join(__dirname, '..', 'require', 'yarnpnp')
 
 function run(command) {
-  console.log('\n\033[37m' + '$ ' + command + '\033[0m')
+  console.log('\n\x1B[37m' + '$ ' + command + '\x1B[0m')
   child_process.execSync(command, { cwd: rootDir, stdio: 'inherit' })
 }
 
@@ -66,20 +66,19 @@ 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 --packages=bundle --platform=node --outfile=out-wasm.js')
+  run('node ../../npm/esbuild-wasm/bin/esbuild in.mjs --bundle --log-level=debug --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 --packages=bundle --platform=node --outfile=out-wasm-yarn.js')
+  run('yarn node ../../npm/esbuild-wasm/bin/esbuild in.mjs --bundle --log-level=debug --platform=node --outfile=out-wasm-yarn.js')
   run('node out-wasm-yarn.js')
 }