fix #2239: more closely replicate a browserify bug

Author: evanw

CHANGELOG.md

@@ -37,6 +37,41 @@
 
     Note that this behavior is TypeScript-specific. Babel doesn't support the `...` token at all (it gives the error "Spread children are not supported in React").
 
+* Slightly adjust esbuild's handling of the `browser` field in `package.json` ([#2239](https://github.com/evanw/esbuild/issues/2239))
+
+    This release changes esbuild's interpretation of `browser` path remapping to fix a regression that was introduced in esbuild version 0.14.21. Browserify has a bug where it incorrectly matches package paths to relative paths in the `browser` field, and esbuild replicates this bug for compatibility with Browserify. I have a set of tests that I use to verify that esbuild's replication of this Browserify is accurate here: https://github.com/evanw/package-json-browser-tests. However, I was missing a test case and esbuild's behavior diverges from Browserify in this case. This release now handles this edge case as well:
+
+    * `entry.js`:
+
+        ```js
+        require('pkg/sub')
+        ```
+
+    * `node_modules/pkg/package.json`:
+
+        ```json
+        {
+          "browser": {
+            "./sub": "./sub/foo.js",
+            "./sub/sub.js": "./sub/foo.js"
+          }
+        }
+        ```
+
+    * `node_modules/pkg/sub/foo.js`:
+
+        ```js
+        require('sub')
+        ```
+
+    * `node_modules/sub/index.js`:
+
+        ```js
+        console.log('works')
+        ```
+
+    The import path `sub` in `require('sub')` was previously matching the remapping `"./sub/sub.js": "./sub/foo.js"` but with this release it should now no longer match that remapping. Now `require('sub')` will only match the remapping `"./sub/sub": "./sub/foo.js"` (without the trailing `.js`). Browserify apparently only matches without the `.js` suffix here.
+
 ## 0.14.38
 
 * Further fixes to TypeScript 4.7 instantiation expression parsing ([#2201](https://github.com/evanw/esbuild/issues/2201))

internal/bundler/bundler_packagejson_test.go

@@ -848,6 +848,28 @@ func TestPackageJsonBrowserIssue2002B(t *testing.T) {
 	})
 }
 
+// See https://github.com/evanw/esbuild/issues/2239
+func TestPackageJsonBrowserIssue2002C(t *testing.T) {
+	packagejson_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.js": `require('pkg/sub')`,
+			"/Users/user/project/src/node_modules/pkg/package.json": `{
+				"browser": {
+					"./sub": "./sub/foo.js",
+					"./sub/sub.js": "./sub/bar.js"
+				}
+			}`,
+			"/Users/user/project/src/node_modules/pkg/sub/foo.js": `require('sub')`,
+			"/Users/user/project/src/node_modules/sub/index.js":   `works()`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/Users/user/project/out.js",
+		},
+	})
+}
+
 func TestPackageJsonDualPackageHazardImportOnly(t *testing.T) {
 	packagejson_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler/snapshots/snapshots_packagejson.txt

@@ -71,6 +71,26 @@ var require_foo = __commonJS({
 // Users/user/project/src/entry.js
 require_foo();
 
+================================================================================
+TestPackageJsonBrowserIssue2002C
+---------- /Users/user/project/out.js ----------
+// Users/user/project/src/node_modules/sub/index.js
+var require_sub = __commonJS({
+  "Users/user/project/src/node_modules/sub/index.js"() {
+    works();
+  }
+});
+
+// Users/user/project/src/node_modules/pkg/sub/foo.js
+var require_foo = __commonJS({
+  "Users/user/project/src/node_modules/pkg/sub/foo.js"() {
+    require_sub();
+  }
+});
+
+// Users/user/project/src/entry.js
+require_foo();
+
 ================================================================================
 TestPackageJsonBrowserMapAvoidMissing
 ---------- /Users/user/project/out.js ----------

internal/resolver/package_json.go

@@ -99,7 +99,14 @@ func (r resolverQuery) checkBrowserMap(resolveDirInfo *dirInfo, inputPath string
 	packageJSON := resolveDirInfo.enclosingBrowserScope.packageJSON
 	browserMap := packageJSON.browserMap
 
-	checkPath := func(pathToCheck string) bool {
+	type implicitExtensions uint8
+
+	const (
+		includeImplicitExtensions implicitExtensions = iota
+		skipImplicitExtensions
+	)
+
+	checkPath := func(pathToCheck string, implicitExtensions implicitExtensions) bool {
 		if r.debugLogs != nil {
 			r.debugLogs.addNote(fmt.Sprintf("Checking for %q in the \"browser\" map in %q",
 				pathToCheck, packageJSON.source.KeyPath.Text))
@@ -116,15 +123,17 @@ func (r resolverQuery) checkBrowserMap(resolveDirInfo *dirInfo, inputPath string
 		}
 
 		// If that failed, try adding implicit extensions
-		for _, ext := range r.options.ExtensionOrder {
-			extPath := pathToCheck + ext
-			if r.debugLogs != nil {
-				r.debugLogs.addNote(fmt.Sprintf("  Checking for %q", extPath))
-			}
-			remapped, ok = browserMap[extPath]
-			if ok {
-				inputPath = extPath
-				return true
+		if implicitExtensions == includeImplicitExtensions {
+			for _, ext := range r.options.ExtensionOrder {
+				extPath := pathToCheck + ext
+				if r.debugLogs != nil {
+					r.debugLogs.addNote(fmt.Sprintf("  Checking for %q", extPath))
+				}
+				remapped, ok = browserMap[extPath]
+				if ok {
+					inputPath = extPath
+					return true
+				}
 			}
 		}
 
@@ -145,15 +154,17 @@ func (r resolverQuery) checkBrowserMap(resolveDirInfo *dirInfo, inputPath string
 		}
 
 		// If that failed, try adding implicit extensions
-		for _, ext := range r.options.ExtensionOrder {
-			extPath := indexPath + ext
-			if r.debugLogs != nil {
-				r.debugLogs.addNote(fmt.Sprintf("  Checking for %q", extPath))
-			}
-			remapped, ok = browserMap[extPath]
-			if ok {
-				inputPath = extPath
-				return true
+		if implicitExtensions == includeImplicitExtensions {
+			for _, ext := range r.options.ExtensionOrder {
+				extPath := indexPath + ext
+				if r.debugLogs != nil {
+					r.debugLogs.addNote(fmt.Sprintf("  Checking for %q", extPath))
+				}
+				remapped, ok = browserMap[extPath]
+				if ok {
+					inputPath = extPath
+					return true
+				}
 			}
 		}
 
@@ -175,11 +186,11 @@ func (r resolverQuery) checkBrowserMap(resolveDirInfo *dirInfo, inputPath string
 	}
 
 	// First try the import path as a package path
-	if !checkPath(inputPath) && IsPackagePath(inputPath) {
+	if !checkPath(inputPath, includeImplicitExtensions) && IsPackagePath(inputPath) {
 		// If a package path didn't work, try the import path as a relative path
 		switch kind {
 		case absolutePathKind:
-			checkPath("./" + inputPath)
+			checkPath("./"+inputPath, includeImplicitExtensions)
 
 		case packagePathKind:
 			// Browserify allows a browser map entry of "./pkg" to override a package
@@ -206,7 +217,10 @@ func (r resolverQuery) checkBrowserMap(resolveDirInfo *dirInfo, inputPath string
 					relativePathPrefix += strings.ReplaceAll(relPath, "\\", "/") + "/"
 				}
 
-				checkPath(relativePathPrefix + inputPath)
+				// Browserify lets "require('pkg')" match "./pkg" but not "./pkg.js".
+				// So don't add implicit extensions specifically in this place so we
+				// match Browserify's behavior.
+				checkPath(relativePathPrefix+inputPath, skipImplicitExtensions)
 			}
 		}
 	}