fix #1680: match node's core module behavior

CHANGELOG.md

@@ -17,6 +17,14 @@
     a{color:rgba(255,128,0,.5)}
     ```
 
+* Match node's behavior for core module detection ([#1680](https://github.com/evanw/esbuild/issues/1680))
+
+    Node has a hard-coded list of core modules (e.g. `fs`) that, when required, short-circuit the module resolution algorithm and instead return the corresponding internal core module object. When you pass `--platform=node` to esbuild, esbuild also implements this short-circuiting behavior and doesn't try to bundle these import paths. This was implemented in esbuild using the existing `external` feature (e.g. essentially `--external:fs`). However, there is an edge case where esbuild's `external` feature behaved differently than node.
+
+    Modules specified via esbuild's `external` feature also cause all sub-paths to be excluded as well, so for example `--external:foo` excludes both `foo` and `foo/bar` from the bundle. However, node's core module check is only an exact equality check, so for example `fs` is a core module and bypasses the module resolution algorithm but `fs/foo` is not a core module and causes the module resolution algorithm to search the file system.
+
+    This behavior can be used to load a module on the file system with the same name as one of node's core modules. For example, `require('fs/')` will load the module `fs` from the file system instead of loading node's core `fs` module. With this release, esbuild will now match node's behavior in this edge case. This means the external modules that are automatically added by `--platform=node` now behave subtly differently than `--external:`, which allows code that relies on this behavior to be bundled correctly.
+
 ## 0.13.6
 
 * Emit decorators for `declare` class fields ([#1675](https://github.com/evanw/esbuild/issues/1675))

internal/bundler/bundler_default_test.go

@@ -4707,3 +4707,38 @@ func TestStrictModeNestedFnDeclKeepNamesVariableInliningIssue1552(t *testing.T)
 		},
 	})
 }
+
+func TestBuiltInNodeModulePrecedence(t *testing.T) {
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				console.log([
+					// These are node core modules
+					require('fs'),
+					require('fs/promises'),
+					require('node:foo'),
+
+					// These are not node core modules
+					require('fs/abc'),
+					require('fs/'),
+				])
+			`,
+			"/node_modules/fs/abc.js": `
+				console.log('include this')
+			`,
+			"/node_modules/fs/index.js": `
+				console.log('include this too')
+			`,
+			"/node_modules/fs/promises.js": `
+				throw 'DO NOT INCLUDE THIS'
+			`,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			Platform:     config.PlatformNode,
+			OutputFormat: config.FormatCommonJS,
+			AbsOutputDir: "/out",
+		},
+	})
+}

internal/bundler/snapshots/snapshots_default.txt

@@ -181,6 +181,32 @@ async function main(name) {
 }
 main("fs");
 
+================================================================================
+TestBuiltInNodeModulePrecedence
+---------- /out/entry.js ----------
+// node_modules/fs/abc.js
+var require_abc = __commonJS({
+  "node_modules/fs/abc.js"() {
+    console.log("include this");
+  }
+});
+
+// node_modules/fs/index.js
+var require_fs = __commonJS({
+  "node_modules/fs/index.js"() {
+    console.log("include this too");
+  }
+});
+
+// entry.js
+console.log([
+  require("fs"),
+  require("fs/promises"),
+  require("node:foo"),
+  require_abc(),
+  require_fs()
+]);
+
 ================================================================================
 TestCallImportNamespaceWarning
 ---------- /out/js.js ----------

internal/resolver/resolver.go

@@ -214,20 +214,6 @@ type resolverQuery struct {
 }
 
 func NewResolver(fs fs.FS, log logger.Log, caches *cache.CacheSet, options config.Options) Resolver {
-	// Bundling for node implies allowing node's builtin modules
-	if options.Platform == config.PlatformNode {
-		externalNodeModules := make(map[string]bool)
-		if options.ExternalModules.NodeModules != nil {
-			for name := range options.ExternalModules.NodeModules {
-				externalNodeModules[name] = true
-			}
-		}
-		for name := range BuiltInNodeModules {
-			externalNodeModules[name] = true
-		}
-		options.ExternalModules.NodeModules = externalNodeModules
-	}
-
 	// Filter out non-CSS extensions for CSS "@import" imports
 	atImportExtensionOrder := make([]string, 0, len(options.ExtensionOrder))
 	for _, ext := range options.ExtensionOrder {
@@ -294,8 +280,9 @@ func (rr *resolver) Resolve(sourceDir string, importPath string, kind ast.Import
 		// "background: url(//example.com/images/image.png);"
 		strings.HasPrefix(importPath, "//") ||
 
+		// "import fs from 'fs'"
 		// "import fs from 'node:fs'"
-		(r.options.Platform == config.PlatformNode && strings.HasPrefix(importPath, "node:")) {
+		(r.options.Platform == config.PlatformNode && (BuiltInNodeModules[importPath] || strings.HasPrefix(importPath, "node:"))) {
 
 		if r.debugLogs != nil {
 			r.debugLogs.addNote("Marking this path as implicitly external")
@@ -1729,6 +1716,11 @@ func IsPackagePath(path string) bool {
 		!strings.HasPrefix(path, "../") && path != "." && path != ".."
 }
 
+// This list can be obtained with the following command:
+//
+//   node --experimental-wasi-unstable-preview1 -p "[...require('module').builtinModules].join('\n')"
+//
+// Be sure to use the *LATEST* version of node when updating this list!
 var BuiltInNodeModules = map[string]bool{
 	"_http_agent":         true,
 	"_http_client":        true,
@@ -1745,6 +1737,7 @@ var BuiltInNodeModules = map[string]bool{
 	"_tls_common":         true,
 	"_tls_wrap":           true,
 	"assert":              true,
+	"assert/strict":       true,
 	"async_hooks":         true,
 	"buffer":              true,
 	"child_process":       true,
@@ -1755,9 +1748,11 @@ var BuiltInNodeModules = map[string]bool{
 	"dgram":               true,
 	"diagnostics_channel": true,
 	"dns":                 true,
+	"dns/promises":        true,
 	"domain":              true,
 	"events":              true,
 	"fs":                  true,
+	"fs/promises":         true,
 	"http":                true,
 	"http2":               true,
 	"https":               true,
@@ -1766,21 +1761,28 @@ var BuiltInNodeModules = map[string]bool{
 	"net":                 true,
 	"os":                  true,
 	"path":                true,
+	"path/posix":          true,
+	"path/win32":          true,
 	"perf_hooks":          true,
 	"process":             true,
 	"punycode":            true,
 	"querystring":         true,
 	"readline":            true,
 	"repl":                true,
 	"stream":              true,
+	"stream/consumers":    true,
+	"stream/promises":     true,
+	"stream/web":          true,
 	"string_decoder":      true,
 	"sys":                 true,
 	"timers":              true,
+	"timers/promises":     true,
 	"tls":                 true,
 	"trace_events":        true,
 	"tty":                 true,
 	"url":                 true,
 	"util":                true,
+	"util/types":          true,
 	"v8":                  true,
 	"vm":                  true,
 	"wasi":                true,