From 9c77053cc60e542229f53807b9cb34c27faa137f Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Wed, 10 Aug 2022 09:18:55 -0400 Subject: [PATCH 1/3] yarn pnp: `$$virtual` is the same as `__virtual__` --- internal/fs/fs_zip.go | 9 +++++++-- scripts/js-api-tests.js | 38 +++++++++++++++++++++++++++++--------- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/internal/fs/fs_zip.go b/internal/fs/fs_zip.go index 316abf92388..c25f7123841 100644 --- a/internal/fs/fs_zip.go +++ b/internal/fs/fs_zip.go @@ -324,8 +324,13 @@ func parseYarnPnPVirtualPath(path string) (string, string, bool) { } i += slash + 1 - // Replace the segments "__virtual__//" with N times the ".." operation - if path[start:i-1] == "__virtual__" { + // Replace the segments "__virtual__//" with N times the ".." + // operation. Note: The "__virtual__" folder name appeared with Yarn 3.0. + // Earlier releases used "$$virtual", but it was changed after discovering + // that this pattern triggered bugs in software where paths were used as + // either regexps or replacement. For example, "$$" found in the second + // parameter of "String.prototype.replace" silently turned into "$". + if segment := path[start : i-1]; segment == "__virtual__" || segment == "$$virtual" { if slash := strings.IndexAny(path[i:], "/\\"); slash != -1 { var count string var suffix string diff --git a/scripts/js-api-tests.js b/scripts/js-api-tests.js index b9eed43e217..014004ed9fb 100644 --- a/scripts/js-api-tests.js +++ b/scripts/js-api-tests.js @@ -2653,17 +2653,25 @@ require("/assets/file.png"); import foo from './test.zip/foo.js' import bar from './test.zip/bar/bar.js' - import virtual1 from './test.zip/__virtual__/ignored/0/foo.js' - import virtual2 from './test.zip/ignored/__virtual__/ignored/1/foo.js' - import virtual3 from './test.zip/__virtual__/ignored/1/test.zip/foo.js' + import __virtual__1 from './test.zip/__virtual__/ignored/0/foo.js' + import __virtual__2 from './test.zip/ignored/__virtual__/ignored/1/foo.js' + import __virtual__3 from './test.zip/__virtual__/ignored/1/test.zip/foo.js' + + import $$virtual1 from './test.zip/$$virtual/ignored/0/foo.js' + import $$virtual2 from './test.zip/ignored/$$virtual/ignored/1/foo.js' + import $$virtual3 from './test.zip/$$virtual/ignored/1/test.zip/foo.js' console.log({ foo, bar, - virtual1, - virtual2, - virtual3, + __virtual__1, + __virtual__2, + __virtual__3, + + $$virtual1, + $$virtual2, + $$virtual3, }) `) @@ -2702,13 +2710,25 @@ require("/assets/file.png"); // scripts/.js-api-tests/zipFile/test.zip/__virtual__/ignored/1/test.zip/foo.js var foo_default4 = "foo"; + // scripts/.js-api-tests/zipFile/test.zip/$$virtual/ignored/0/foo.js + var foo_default5 = "foo"; + + // scripts/.js-api-tests/zipFile/test.zip/ignored/$$virtual/ignored/1/foo.js + var foo_default6 = "foo"; + + // scripts/.js-api-tests/zipFile/test.zip/$$virtual/ignored/1/test.zip/foo.js + var foo_default7 = "foo"; + // scripts/.js-api-tests/zipFile/entry.js console.log({ foo: foo_default, bar: bar_default, - virtual1: foo_default2, - virtual2: foo_default3, - virtual3: foo_default4 + __virtual__1: foo_default2, + __virtual__2: foo_default3, + __virtual__3: foo_default4, + $$virtual1: foo_default5, + $$virtual2: foo_default6, + $$virtual3: foo_default7 }); })(); `) From b7908b618d174fed33cb7b2b6572676e38e4cfb7 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Wed, 10 Aug 2022 09:28:02 -0400 Subject: [PATCH 2/3] yarn pnp: handle other specification changes --- internal/resolver/testExpectations.json | 2 +- internal/resolver/yarnpnp.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/resolver/testExpectations.json b/internal/resolver/testExpectations.json index 7a44b4339f1..56cd4a106f9 100644 --- a/internal/resolver/testExpectations.json +++ b/internal/resolver/testExpectations.json @@ -13,7 +13,7 @@ [null, [ [null, { "packageLocation": "./", - "packageDependencies": [], + "packageDependencies": [["test", "npm:1.0.0"]], "linkType": "SOFT" }] ]], diff --git a/internal/resolver/yarnpnp.go b/internal/resolver/yarnpnp.go index 53b77419d03..3bb7984e631 100644 --- a/internal/resolver/yarnpnp.go +++ b/internal/resolver/yarnpnp.go @@ -91,8 +91,8 @@ func (r resolverQuery) pnpResolve(specifier string, parentURL string, parentMani return specifier, true } - // Otherwise, if specifier starts with "/", "./", or "../", then - if strings.HasPrefix(specifier, "/") || strings.HasPrefix(specifier, "./") || strings.HasPrefix(specifier, "../") { + // Otherwise, if `specifier` is either an absolute path or a path prefixed with "./" or "../", then + if r.fs.IsAbs(specifier) || strings.HasPrefix(specifier, "./") || strings.HasPrefix(specifier, "../") { // Set resolved to NM_RESOLVE(specifier, parentURL) and return it return specifier, true } @@ -264,10 +264,10 @@ func (r resolverQuery) resolveToUnqualified(specifier string, parentURL string, r.debugLogs.addNote(fmt.Sprintf(" Found package %q at %q", ident, dependencyPkg.packageLocation)) } - // Return dependencyPkg.packageLocation concatenated with modulePath - resolved := dependencyPkg.packageLocation + modulePath - result := r.fs.Join(manifest.absDirPath, resolved) - if strings.HasSuffix(resolved, "/") && !strings.HasSuffix(result, "/") { + // 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 { From b9cae7d0d1370c8e53cb3d1b6ad07702c2f797cd Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Wed, 10 Aug 2022 09:41:34 -0400 Subject: [PATCH 3/3] yarn pnp: ignore manifests in __virtual__ folders --- internal/fs/fs_zip.go | 6 +-- internal/resolver/resolver.go | 54 +++++++++++++++++++------- scripts/js-api-tests.js | 73 +++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 18 deletions(-) diff --git a/internal/fs/fs_zip.go b/internal/fs/fs_zip.go index c25f7123841..346730cc316 100644 --- a/internal/fs/fs_zip.go +++ b/internal/fs/fs_zip.go @@ -279,7 +279,7 @@ func (fs *zipFS) Abs(path string) (string, bool) { } func (fs *zipFS) Dir(path string) string { - if prefix, suffix, ok := parseYarnPnPVirtualPath(path); ok && suffix == "" { + if prefix, suffix, ok := ParseYarnPnPVirtualPath(path); ok && suffix == "" { return prefix } return fs.inner.Dir(path) @@ -313,7 +313,7 @@ func (fs *zipFS) WatchData() WatchData { return fs.inner.WatchData() } -func parseYarnPnPVirtualPath(path string) (string, string, bool) { +func ParseYarnPnPVirtualPath(path string) (string, string, bool) { i := 0 for { @@ -377,7 +377,7 @@ func parseYarnPnPVirtualPath(path string) (string, string, bool) { } func mangleYarnPnPVirtualPath(path string) string { - if prefix, suffix, ok := parseYarnPnPVirtualPath(path); ok { + if prefix, suffix, ok := ParseYarnPnPVirtualPath(path); ok { return prefix + suffix } return path diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go index 3feead3d9a9..b527e78a682 100644 --- a/internal/resolver/resolver.go +++ b/internal/resolver/resolver.go @@ -1189,21 +1189,45 @@ func (r resolverQuery) dirInfoUncached(path string) *dirInfo { } } - // Record if this directory has a Yarn PnP data file - if pnp, _ := entries.Get(".pnp.data.json"); pnp != nil && pnp.Kind(r.fs) == fs.FileEntry { - absPath := r.fs.Join(path, ".pnp.data.json") - if json := r.extractYarnPnPDataFromJSON(absPath, &r.caches.JSONCache); json.Data != nil { - info.pnpData = compileYarnPnPData(absPath, path, json) - } - } else if pnp, _ := entries.Get(".pnp.cjs"); pnp != nil && pnp.Kind(r.fs) == fs.FileEntry { - absPath := r.fs.Join(path, ".pnp.cjs") - if json := r.tryToExtractYarnPnPDataFromJS(absPath, &r.caches.JSONCache); json.Data != nil { - info.pnpData = compileYarnPnPData(absPath, path, json) - } - } else if pnp, _ := entries.Get(".pnp.js"); pnp != nil && pnp.Kind(r.fs) == fs.FileEntry { - absPath := r.fs.Join(path, ".pnp.js") - if json := r.tryToExtractYarnPnPDataFromJS(absPath, &r.caches.JSONCache); json.Data != nil { - info.pnpData = compileYarnPnPData(absPath, path, json) + // Record if this directory has a Yarn PnP manifest. This must not be done + // for Yarn virtual paths because that will result in duplicate copies of + // the same manifest which will result in multiple copies of the same virtual + // directory in the same path, which we don't handle (and which also doesn't + // match Yarn's behavior). + // + // For example, imagine a project with a manifest here: + // + // /project/.pnp.cjs + // + // and a source file with an import of "bar" here: + // + // /project/.yarn/__virtual__/pkg/1/foo.js + // + // If we didn't ignore Yarn PnP manifests in virtual folders, then we would + // pick up on the one here: + // + // /project/.yarn/__virtual__/pkg/1/.pnp.cjs + // + // which means we would potentially resolve the import to something like this: + // + // /project/.yarn/__virtual__/pkg/1/.yarn/__virtual__/pkg/1/bar + // + if _, _, ok := fs.ParseYarnPnPVirtualPath(path); !ok { + if pnp, _ := entries.Get(".pnp.data.json"); pnp != nil && pnp.Kind(r.fs) == fs.FileEntry { + absPath := r.fs.Join(path, ".pnp.data.json") + if json := r.extractYarnPnPDataFromJSON(absPath, &r.caches.JSONCache); json.Data != nil { + info.pnpData = compileYarnPnPData(absPath, path, json) + } + } else if pnp, _ := entries.Get(".pnp.cjs"); pnp != nil && pnp.Kind(r.fs) == fs.FileEntry { + absPath := r.fs.Join(path, ".pnp.cjs") + if json := r.tryToExtractYarnPnPDataFromJS(absPath, &r.caches.JSONCache); json.Data != nil { + info.pnpData = compileYarnPnPData(absPath, path, json) + } + } else if pnp, _ := entries.Get(".pnp.js"); pnp != nil && pnp.Kind(r.fs) == fs.FileEntry { + absPath := r.fs.Join(path, ".pnp.js") + if json := r.tryToExtractYarnPnPDataFromJS(absPath, &r.caches.JSONCache); json.Data != nil { + info.pnpData = compileYarnPnPData(absPath, path, json) + } } } diff --git a/scripts/js-api-tests.js b/scripts/js-api-tests.js index 014004ed9fb..6938a064209 100644 --- a/scripts/js-api-tests.js +++ b/scripts/js-api-tests.js @@ -2985,6 +2985,79 @@ require("/assets/file.png"); // scripts/.js-api-tests/yarnPnP_pnp_cjs_JSON_parse_identifier/entry.js console.log(left_pad_default()); })(); +`) + }, + + async yarnPnP_ignoreNestedManifests({ esbuild, testDir }) { + const entry = path.join(testDir, 'entry.js') + const foo = path.join(testDir, 'foo', 'index.js') + const bar = path.join(testDir, 'bar', 'index.js') + const manifest = path.join(testDir, '.pnp.data.json') + + await writeFileAsync(entry, ` + import foo from 'foo' + console.log(foo) + `) + + await mkdirAsync(path.dirname(foo), { recursive: true }) + await writeFileAsync(foo, ` + import bar from 'bar' + export default 'foo' + bar + `) + + await mkdirAsync(path.dirname(bar), { recursive: true }) + await writeFileAsync(bar, ` + export default 'bar' + `) + + await writeFileAsync(manifest, `{ + "packageRegistryData": [ + [null, [ + [null, { + "packageLocation": "./", + "packageDependencies": [ + ["foo", "npm:1.0.0"], + ["bar", "npm:1.0.0"] + ], + "linkType": "SOFT" + }] + ]], + ["foo", [ + ["npm:1.0.0", { + "packageLocation": "./__virtual__/whatever/0/foo/", + "packageDependencies": [ + ["bar", "npm:1.0.0"] + ], + "linkType": "HARD" + }] + ]], + ["bar", [ + ["npm:1.0.0", { + "packageLocation": "./__virtual__/whatever/0/bar/", + "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_ignoreNestedManifests/__virtual__/whatever/0/bar/index.js + var bar_default = "bar"; + + // scripts/.js-api-tests/yarnPnP_ignoreNestedManifests/__virtual__/whatever/0/foo/index.js + var foo_default = "foo" + bar_default; + + // scripts/.js-api-tests/yarnPnP_ignoreNestedManifests/entry.js + console.log(foo_default); +})(); `) }, }