fix #2455: strip / to fix index.js edge case

Author: evanw

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* Fix Yarn PnP issue with packages containing `index.js` ([#2455](https://github.com/evanw/esbuild/issues/2455), [#2461](https://github.com/evanw/esbuild/issues/2461))
+
+    Yarn PnP's tests require the resolved paths to end in `/`. That's not how the rest of esbuild's internals work, however, and doing this messed up esbuild's node module path resolution regarding automatically-detected `index.js` files. Previously packages that relied on implicit `index.js` resolution rules didn't work with esbuild under Yarn PnP. Removing this slash has fixed esbuild's path resolution behavior regarding `index.js`, which should now the same both with and without Yarn PnP.
+
 * Fix Yarn PnP support for `extends` in `tsconfig.json` ([#2456](https://github.com/evanw/esbuild/issues/2456))
 
     Previously using `extends` in `tsconfig.json` with a path in a Yarn PnP package didn't work. This is because the process of setting up package path resolution rules requires parsing `tsconfig.json` files (due to the `baseUrl` and `paths` features) and resolving `extends` to a package path requires package path resolution rules to already be set up, which is a circular dependency. This cycle is broken by using special rules for `extends` in `tsconfig.json` that bypasses esbuild's normal package path resolution process. This is why using `extends` with a Yarn PnP package didn't automatically work. With this release, these special rules have been modified to check for a Yarn PnP manifest so this case should work now.

internal/resolver/yarnpnp.go

@@ -266,10 +266,6 @@ func (r resolverQuery) resolveToUnqualified(specifier string, parentURL string,
 
 	// Return path.resolve(manifest.dirPath, dependencyPkg.packageLocation, modulePath)
 	result := r.fs.Join(manifest.absDirPath, dependencyPkg.packageLocation, modulePath)
-	if !strings.HasSuffix(result, "/") && ((modulePath != "" && strings.HasSuffix(modulePath, "/")) ||
-		(modulePath == "" && strings.HasSuffix(dependencyPkg.packageLocation, "/"))) {
-		result += "/" // This is important for matching Yarn PnP's expectations in tests
-	}
 	if r.debugLogs != nil {
 		r.debugLogs.addNote(fmt.Sprintf("  Resolved %q via Yarn PnP to %q", specifier, result))
 	}

internal/resolver/yarnpnp_test.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"strings"
 	"testing"
 
 	"github.com/evanw/esbuild/internal/config"
@@ -80,6 +81,13 @@ func TestYarnPnP(t *testing.T) {
 					// incorrect. So we change the expected value of the test instead.
 					if current.It == `shouldn't go through PnP when trying to resolve dependencies from packages covered by ignorePatternData` {
 						expected = current.Imported
+					} else if result != "error!" && !strings.HasSuffix(result, "/") {
+						// This is important for matching Yarn PnP's expectations in tests,
+						// but it's important for esbuild that the slash isn't present.
+						// Otherwise esbuild's implementation of node module resolution
+						// (which runs after Yarn PnP resolution) will fail. Specifically
+						// "foo/" will look for "foo/foo.js" instead of "foo/index.js".
+						result += "/"
 					}
 
 					test.AssertEqualWithDiff(t, result, expected)

scripts/js-api-tests.js

@@ -3116,6 +3116,61 @@ require("/assets/file.png");
   // scripts/.js-api-tests/yarnPnP_tsconfig/entry.js
   x = __pow(x, 2);
 })();
+`)
+  },
+
+  async yarnPnP_indexJs({ esbuild, testDir }) {
+    const entry = path.join(testDir, 'entry.js')
+    const fooIndex = path.join(testDir, 'foo', 'index.js')
+    const fooFoo = path.join(testDir, 'foo', 'foo.js')
+    const manifest = path.join(testDir, '.pnp.data.json')
+
+    await writeFileAsync(entry, `
+      import x from '@some/pkg'
+      x()
+    `)
+
+    await mkdirAsync(path.dirname(fooIndex), { recursive: true })
+    await writeFileAsync(fooIndex, `export default success`)
+
+    await mkdirAsync(path.dirname(fooFoo), { recursive: true })
+    await writeFileAsync(fooFoo, `failure!`)
+
+    await writeFileAsync(manifest, `{
+      "packageRegistryData": [
+        [null, [
+          [null, {
+            "packageLocation": "./",
+            "packageDependencies": [
+              ["@some/pkg", "npm:1.0.0"]
+            ],
+            "linkType": "SOFT"
+          }]
+        ]],
+        ["@some/pkg", [
+          ["npm:1.0.0", {
+            "packageLocation": "./foo/",
+            "packageDependencies": [],
+            "linkType": "HARD"
+          }]
+        ]]
+      ]
+    }`)
+
+    const value = await esbuild.build({
+      entryPoints: [entry],
+      bundle: true,
+      write: false,
+    })
+
+    assert.strictEqual(value.outputFiles.length, 1)
+    assert.strictEqual(value.outputFiles[0].text, `(() => {
+  // scripts/.js-api-tests/yarnPnP_indexJs/foo/index.js
+  var foo_default = success;
+
+  // scripts/.js-api-tests/yarnPnP_indexJs/entry.js
+  foo_default();
+})();
 `)
   },
 }