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.

My change now resolves the issue by normalizing the destination path returned by PackageHoister.init. As far as I can tell, this is only ever used as a filesystem path downstream, so this should be safe.

Test plan

Automated:
The included unit test demonstrates the failing case when run on Windows with the change reverted.

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

[dpoindexter]

Note: This is the change that #1862 was supposed to contain. Apologies for adding to the noise with my initial mistake!

[bestander]

Thanks, @dpoindexter.
Can you come up with a unit test for this fix?
Can be an isolated fs unit test.

Is it a regression since release 0.16.1?

[bestander]

Unit test would be much appreciated

[dpoindexter]

Not a regression -- it's been this way since at least 0.15 as far as I can tell. I'll add tests.

[dpoindexter]

Added a unit test, and edited the PR description to reflect the updated approach. I rebased onto most recent master before pushing these updates-- is there something I need to mark to show that I've made the requested review changes?

[wyze]

@dpoindexter, nothing you need to mark, just comment that you made the requested changes, which you did.

[bestander]

@dpoindexter, thanks a lot for the test!

[robgha01]

what is the status on publiching this fix ? im getting the @types problem so i guess its yet not out ?

[dpoindexter]

@robgha01: The PR was merge and released in 0.18.0, which is still marked as a pre-release version.