Resolve scoped package paths during linking on Windows

[dpoindexter]

Summary

Resolves #1861.

The above issue is Windows-specific. It's caused by the step that unlinks extraneous packages. This compares a Set possibleExtraneous against a Set of paths files resolved during bulk copy.

The problem is how paths are added to the files Set. yarn attempts to add all path segments to files by splitting the destination path on path.sep. This works as expected on Posix systems, where the path separator is the same as the scoped package name delimiter:

project/node_modules/@scoped/dep/node_modules/subdep

But on Windows, the destination path is constructed naively from the package name:

project\\node_modules\\@scoped/dep\\node_modules\\subdep

possibleExtraneous, having been resolved from reading directories on disk rather than from the package.json contains the correct Windows path:

project\\node_modules\\@scoped\\dep\\node_modules\\subdep

possibleExtraneous then checks to see if its members are present in files. Any that are are not considered extraneous. Because of the above difference in the path string, scoped subdirectories are wrongly seen as extraneous, and deleted.

What changed
It was difficult for me to tell what was intended behavior in other parts of the system, so I constrained this change to the two places that file paths are added to the non-extraneous files collection during the bulk copy operation. I'm calling path.normalize on each destination path before it's split by path.sep. This resolves the separator inconsistency caused by scoped package names.

It may be better to resolve this higher up, e.g where the CopyQueue is generated.

Test plan

Automated:
I'm having trouble running automated tests locally due to #1823. I could also use some help with Jest.

Manual:
The repro steps in the linked issue demonstrate that this is resolved on Windows.

1. yarn add a scoped package (ex. @cycle/http)
2. yarn add any dependency of the scoped package, at an incompatible version (ex. [email protected]) (This prevents the subdependency of the scoped package from hoisting)
3. Run the same yarn add a second time

[wyze]

This same fix was merged in #1840.

[dpoindexter]

I'm not seeing how #1840 addresses the same issue. I can still repro the error after applying that fix locally.

[wyze]

1840.diff

diff --git a/src/cli/commands/global.js b/src/cli/commands/global.js
index 934569a..0716f99 100644
--- a/src/cli/commands/global.js
+++ b/src/cli/commands/global.js
@@ -17,7 +17,7 @@ import * as fs from '../../util/fs.js';

 class GlobalAdd extends Add {
   maybeOutputSaveTree(): Promise<void> {
-    for (const pattern of this.args) {
+    for (const pattern of this.addedPatterns) {
       const manifest = this.resolver.getStrictResolvedPattern(pattern);
       ls(manifest, this.reporter, true);
     }

1862.diff:

diff --git a/src/cli/commands/global.js b/src/cli/commands/global.js
index 934569a..0716f99 100644
--- a/src/cli/commands/global.js
+++ b/src/cli/commands/global.js
@@ -17,7 +17,7 @@ import * as fs from '../../util/fs.js';

 class GlobalAdd extends Add {
   maybeOutputSaveTree(): Promise<void> {
-    for (const pattern of this.args) {
+    for (const pattern of this.addedPatterns) {
       const manifest = this.resolver.getStrictResolvedPattern(pattern);
       ls(manifest, this.reporter, true);
     }

Not sure how it doesn't resolve it for you since the diffs are exactly the same. Also not sure why GitHub didn't report any merge conflicts.

[dpoindexter]

You're totally right. The diff above is not the change I intended to be in this PR. I don't know what happened -- I'll get the right change and make a new PR. So sorry!