From 9b4026768e946482a453a5611ab1347275a1f47a Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Fri, 12 Aug 2022 17:32:50 -0400 Subject: [PATCH] fix #2455: strip `/` to fix `index.js` edge case --- CHANGELOG.md | 4 +++ internal/resolver/yarnpnp.go | 4 --- internal/resolver/yarnpnp_test.go | 8 +++++ scripts/js-api-tests.js | 55 +++++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ce6ec3020c..05f2cb4bcea 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/internal/resolver/yarnpnp.go b/internal/resolver/yarnpnp.go index 8695dc39b4f..c1d138028b1 100644 --- a/internal/resolver/yarnpnp.go +++ b/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)) } diff --git a/internal/resolver/yarnpnp_test.go b/internal/resolver/yarnpnp_test.go index df1c5c4b312..7b0a155f8a8 100644 --- a/internal/resolver/yarnpnp_test.go +++ b/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) diff --git a/scripts/js-api-tests.js b/scripts/js-api-tests.js index 51d3dbb65a9..810586a72cc 100644 --- a/scripts/js-api-tests.js +++ b/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(); +})(); `) }, }