feat(plugin-nm): support configuring link mode

[jeff-wishnie]

What's the problem this PR addresses?

Closes #4979

Currently on Windows yarn uses junctions to link workspaces into node_modules directories. On other platforms yarn uses symlinks.

As a result, workspace links in node_modules on Windows are always absolute paths. On other platforms links may be relative paths, leading to inconsistent behavior when using nodeLinker: node-modules and running yarn install from Windows vs. Linux.

A particularly bad side-effect of absolute paths on Windows is that it breaks Docker builds--when a monorepo is mounted into a container, the absolute paths are invalid.

The PR adds an additional configuration option for the nm-linker to use symbolic links on Windows as well, thereby supporting relative links, and fixing container builds.
...

How did you fix it?
added the option nmFolderLinkMode with two settiings:

• classic (the default) -- nm-linker behaves exactly as today, using junctions on Windows and symlinks on other platforms.
• symlinks -- nm-linker uses symlinks on all platforms

...

Checklist

• I have read the Contributing Guide.

• I have set the packages that need to be released for my changes to be effective.
  [PLEASE CHECK MY WORK HERE!]

• I will check that all automated PR checks pass before the PR gets reviewed.
  [A handful of unit and integration tests are failing when I run against master. This same set fails on my branch]

[jeff-wishnie]

There seems to be an issue with the E2E NM Babel Install pipeline. There hasn't been a passing run since Sept 18.

[merceyz]

Indeed, they have some custom plugins that aren't compatible with Yarn v4 (master).

[merceyz]

We also do this in the pnpm linker, should the same logic be applied there?

berry/packages/plugin-pnpm/sources/PnpmLinker.ts

Line 235 in 63a26fe

await xfs.symlinkPromise(depSrcPaths.packageLocation, depDstPath, `junction`);

[jeff-wishnie]

We also do this in the pnpm linker, should the same logic be applied there?

berry/packages/plugin-pnpm/sources/PnpmLinker.ts

Line 235 in 63a26fe

await xfs.symlinkPromise(depSrcPaths.packageLocation, depDstPath, `junction`);

That probably would make sense, but I'm not familiar with pnpm (never used it) so wouldn't want to make that call.

If the pnpm folks like it's the right thing to do, would it make sense to have a single configuration setting that applies to both nm-linker and pnpm-linker or separate flags? It seems like the convention is for plugins to have their own config extensions (e.g. the configs prefixes with nm or npm) and that it could get messy if plugins tried to share config settings that are not defined in core.

It looks like you are one of the maintainers of pnpm-plugin, what would you like to do?

I'm happy to make a similar PR for that plugin.

[larixer]

The integration test for this feature is missing, we typically have integration tests for all the features in Yarn, that make sure they will not become broken in the future. Please add a test. The nm linker integration tests should be added here:
https://github.com/yarnpkg/berry/blob/master/packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts

[larixer]

Looks good to me, thank you!

[jeff-wishnie]

@larixer @merceyz , thank you for the reviews.

@merceyz I think merge is blocked by the change you requested that I made via a push rather than the GitHub UI. I don't see how to clear the change-request, is that something you can do?

Feedback addressed

[jeff-wishnie]

I think this is ready for merge.

@merceyz would you like me to propose a similar PR for the pnpm linker?

[merceyz]

[...], what would you like to do?
would you like me to propose a similar PR for the pnpm linker?

I was thinking there should be a single setting controlling all symlink/junction creation in Yarn instead of one for each individual case, however I wont block this PR on that.

[jeff-wishnie]

I was thinking there should be a single setting controlling all symlink/junction creation in Yarn

I'd be happy to work on that as a follow-up PR:

• replace current nm-linker specific config with a global setting
• update the pnpm linker to respect that setting as well.

I don't see junctions used elsewhere in the code base, but wonder if there are any contrib plugins that do that would want to update as well?

[larixer]

@jeff-wishnie Sounds good. I'm not aware about any contrib plugins. I think the pnpm linker is the only other place where junctions are used. We would be grateful if you submit a follow-up PR for pnpm linker.

[jeff-wishnie]

Will! As noted above, I've never worked with pnpm, this seems a pretty mechanical change so I'm not concerned but definitely looking for another solid review of that one!