fix #2963: a non-determinism bug with alias

Author: evanw

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* Fix the `alias` feature to always prefer the longest match ([#2963](https://github.com/evanw/esbuild/issues/2963))
+
+    It's possible to configure conflicting aliases such as `--alias:a=b` and `--alias:a/c=d`, which is ambiguous for the import path `a/c/x` (since it could map to either `b/c/x` or `d/x`). Previously esbuild would pick the first matching `alias`, which would non-deterministically pick between one of the possible matches. This release fixes esbuild to always deterministically pick the longest possible match.
+
 * Minify calls to some global primitive constructors ([#2962](https://github.com/evanw/esbuild/issues/2962))
 
     With this release, esbuild's minifier now replaces calls to `Boolean`/`Number`/`String`/`BigInt` with equivalent shorter code when relevant:

internal/bundler_tests/bundler_default_test.go

@@ -7551,6 +7551,36 @@ func TestPackageAlias(t *testing.T) {
 	})
 }
 
+func TestPackageAliasMatchLongest(t *testing.T) {
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				import "pkg"
+				import "pkg/foo"
+				import "pkg/foo/bar"
+				import "pkg/foo/bar/baz"
+				import "pkg/bar/baz"
+				import "pkg/baz"
+			`,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+			PackageAliases: map[string]string{
+				"pkg":         "alias/pkg",
+				"pkg/foo":     "alias/pkg_foo",
+				"pkg/foo/bar": "alias/pkg_foo_bar",
+			},
+			ExternalSettings: config.ExternalSettings{
+				PreResolve: config.ExternalMatchers{
+					Patterns: []config.WildcardPattern{{Prefix: "alias/"}},
+				},
+			},
+		},
+	})
+}
+
 func TestErrorsForAssertTypeJSON(t *testing.T) {
 	default_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler_tests/snapshots/snapshots_default.txt

@@ -4697,6 +4697,17 @@ console.log(10);
 // node_modules/@scope/prefix-foo/index.js
 console.log(11);
 
+================================================================================
+TestPackageAliasMatchLongest
+---------- /out.js ----------
+// entry.js
+import "alias/pkg";
+import "alias/pkg_foo";
+import "alias/pkg_foo_bar";
+import "alias/pkg_foo_bar/baz";
+import "alias/pkg/bar/baz";
+import "alias/pkg/baz";
+
 ================================================================================
 TestQuotedProperty
 ---------- /out/entry.js ----------

internal/resolver/resolver.go

@@ -293,42 +293,49 @@ func (res *Resolver) Resolve(sourceDir string, importPath string, kind ast.Impor
 		if r.debugLogs != nil {
 			r.debugLogs.addNote("Checking for package alias matches")
 		}
-		foundMatch := false
+		longestKey := ""
+		longestValue := ""
+
 		for key, value := range r.options.PackageAliases {
-			if strings.HasPrefix(importPath, key) && (len(importPath) == len(key) || importPath[len(key)] == '/') {
-				// Resolve the package using the current path instead of the original
-				// path. This is trying to resolve the substitute in the top-level
-				// package instead of the nested package, which lets the top-level
-				// package control the version of the substitution. It's also critical
-				// when using Yarn PnP because Yarn PnP doesn't allow nested packages
-				// to "reach outside" of their normal dependency lists.
-				sourceDir = r.fs.Cwd()
-				if tail := importPath[len(key):]; tail != "/" {
-					// Don't include the trailing characters if they are equal to a
-					// single slash. This comes up because you can abuse this quirk of
-					// node's path resolution to force node to load the package from the
-					// file system instead of as a built-in module. For example, "util"
-					// is node's built-in module while "util/" is one on the file system.
-					// Leaving the trailing slash in place causes problems for people:
-					// https://github.com/evanw/esbuild/issues/2730. It should be ok to
-					// always strip the trailing slash even when using the alias feature
-					// to swap one package for another (except when you swap a reference
-					// to one built-in node module with another but really why would you
-					// do that).
-					value += tail
-				}
-				debugMeta.ModifiedImportPath = value
-				if r.debugLogs != nil {
-					r.debugLogs.addNote(fmt.Sprintf("  Matched with alias from %q to %q", key, value))
-					r.debugLogs.addNote(fmt.Sprintf("  Modified import path from %q to %q", importPath, debugMeta.ModifiedImportPath))
-					r.debugLogs.addNote(fmt.Sprintf("  Changed resolve directory to %q", sourceDir))
-				}
-				importPath = debugMeta.ModifiedImportPath
-				foundMatch = true
-				break
+			if len(key) > len(longestKey) && strings.HasPrefix(importPath, key) && (len(importPath) == len(key) || importPath[len(key)] == '/') {
+				longestKey = key
+				longestValue = value
 			}
 		}
-		if r.debugLogs != nil && !foundMatch {
+
+		if longestKey != "" {
+			debugMeta.ModifiedImportPath = longestValue
+			if tail := importPath[len(longestKey):]; tail != "/" {
+				// Don't include the trailing characters if they are equal to a
+				// single slash. This comes up because you can abuse this quirk of
+				// node's path resolution to force node to load the package from the
+				// file system instead of as a built-in module. For example, "util"
+				// is node's built-in module while "util/" is one on the file system.
+				// Leaving the trailing slash in place causes problems for people:
+				// https://github.com/evanw/esbuild/issues/2730. It should be ok to
+				// always strip the trailing slash even when using the alias feature
+				// to swap one package for another (except when you swap a reference
+				// to one built-in node module with another but really why would you
+				// do that).
+				debugMeta.ModifiedImportPath += tail
+			}
+			if r.debugLogs != nil {
+				r.debugLogs.addNote(fmt.Sprintf("  Matched with alias from %q to %q", longestKey, longestValue))
+				r.debugLogs.addNote(fmt.Sprintf("  Modified import path from %q to %q", importPath, debugMeta.ModifiedImportPath))
+			}
+			importPath = debugMeta.ModifiedImportPath
+
+			// Resolve the package using the current path instead of the original
+			// path. This is trying to resolve the substitute in the top-level
+			// package instead of the nested package, which lets the top-level
+			// package control the version of the substitution. It's also critical
+			// when using Yarn PnP because Yarn PnP doesn't allow nested packages
+			// to "reach outside" of their normal dependency lists.
+			sourceDir = r.fs.Cwd()
+			if r.debugLogs != nil {
+				r.debugLogs.addNote(fmt.Sprintf("  Changed resolve directory to %q", sourceDir))
+			}
+		} else if r.debugLogs != nil {
 			r.debugLogs.addNote("  Failed to find any package alias matches")
 		}
 	}