fix(node-modules): Followup fixes to node_modules portals support (#2806

)

* Followup fixes to nm portals support

* Amends the name of YN00071 error code

.yarn/versions/0a83996d.yml

@@ -0,0 +1,24 @@
+releases:
+  "@yarnpkg/cli": patch
+  "@yarnpkg/plugin-node-modules": patch
+  "@yarnpkg/pnpify": patch
+
+declined:
+  - "@yarnpkg/plugin-compat"
+  - "@yarnpkg/plugin-constraints"
+  - "@yarnpkg/plugin-dlx"
+  - "@yarnpkg/plugin-essentials"
+  - "@yarnpkg/plugin-init"
+  - "@yarnpkg/plugin-interactive-tools"
+  - "@yarnpkg/plugin-npm-cli"
+  - "@yarnpkg/plugin-pack"
+  - "@yarnpkg/plugin-patch"
+  - "@yarnpkg/plugin-pnp"
+  - "@yarnpkg/plugin-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - vscode-zipfs
+  - "@yarnpkg/builder"
+  - "@yarnpkg/core"
+  - "@yarnpkg/doctor"

packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts

@@ -878,6 +878,38 @@ describe(`Node_Modules`, () => {
       })
   );
 
+  test(`should still hoist direct dependencies from portal target to parent with nmHoistingLimits: dependencies`,
+    makeTemporaryEnv({},
+      {
+        nodeLinker: `node-modules`,
+        nmHoistingLimits: `dependencies`,
+      },
+      async ({path, run, source}) => {
+        await xfs.mktempPromise(async portalTarget => {
+          await xfs.writeJsonPromise(`${portalTarget}/package.json` as PortablePath, {
+            name: `portal`,
+            dependencies: {
+              [`one-fixed-dep`]: `1.0.0`,
+            },
+          });
+          await xfs.writeJsonPromise(`${path}/package.json` as PortablePath, {
+            dependencies: {
+              [`portal`]: `portal:${portalTarget}`,
+            },
+          });
+
+          const {stdout} = await run(`install`);
+
+          await expect(source(`require('one-fixed-dep')`)).resolves.toMatchObject({
+            version: `1.0.0`,
+          });
+          expect(stdout).toMatch(new RegExp(`--preserve-symlinks`));
+          expect(await xfs.existsPromise(`${portalTarget}/node_modules` as PortablePath)).toBeFalsy();
+          expect(await xfs.existsPromise(`${path}/node_modules/no-deps` as PortablePath)).toBeFalsy();
+        });
+      })
+  );
+
   test(`should error out on external portal requiring a dependency that conflicts with parent package`,
     makeTemporaryEnv({},
       {
@@ -888,14 +920,18 @@ describe(`Node_Modules`, () => {
           await xfs.writeJsonPromise(`${portalTarget}/package.json` as PortablePath, {
             name: `portal`,
             dependencies: {
-              [`no-deps`]: `2.0.0`,
+              'no-deps': `2.0.0`,
+            },
+            peerDependencies: {
+              'no-deps-bins': `1.0.0`,
             },
           });
 
           await xfs.writeJsonPromise(`${path}/package.json` as PortablePath, {
             dependencies: {
               portal: `portal:${portalTarget}`,
               'no-deps': `1.0.0`,
+              'no-deps-bins': `1.0.0`,
             },
           });
 
@@ -911,6 +947,47 @@ describe(`Node_Modules`, () => {
       })
   );
 
+  test(`should error out if two same-parent portals have conflict between their direct dependencies`,
+    makeTemporaryEnv({},
+      {
+        nodeLinker: `node-modules`,
+      },
+      async ({path, run}) => {
+        await xfs.mktempPromise(async portalTarget1 => {
+          await xfs.mktempPromise(async portalTarget2 => {
+            await xfs.writeJsonPromise(`${portalTarget1}/package.json` as PortablePath, {
+              name: `portal1`,
+              dependencies: {
+                'no-deps': `2.0.0`,
+              },
+            });
+            await xfs.writeJsonPromise(`${portalTarget2}/package.json` as PortablePath, {
+              name: `portal2`,
+              dependencies: {
+                'no-deps': `1.0.0`,
+              },
+            });
+
+            await xfs.writeJsonPromise(`${path}/package.json` as PortablePath, {
+              dependencies: {
+                portal1: `portal:${portalTarget1}`,
+                portal2: `portal:${portalTarget2}`,
+              },
+            });
+
+            let stdout;
+            try {
+              await run(`install`);
+            } catch (e) {
+              stdout = e.stdout;
+            }
+
+            expect(stdout).toMatch(new RegExp(`dependency no-deps@npm:1.0.0 conflicts with dependency no-deps@npm:2.0.0 from sibling portal portal1`));
+          });
+        });
+      })
+  );
+
   test(
     `should not warn when depending on workspaces with postinstall`,
     makeTemporaryEnv(

packages/gatsby/content/advanced/error-codes.md

@@ -328,9 +328,11 @@ A packageExtension is detected by Yarn as being unused, which means that the sel
 
 A packageExtension is detected by Yarn as being unneeded, which means that the selected packages have the same behavior with and without the extension.
 
-## YN0071 - `NM_CANT_INSTALL_PORTAL`
+## YN0071 - `NM_CANT_INSTALL_EXTERNAL_SOFT_LINK`
 
-A portal dependency cannot be installed, because incompatible version of a dependency exists in the parent package. This prevents portal representation for node_modules installs without a need to write files into portal's target directory, which is forbidden for security reasons.
+An external soft link (portal) cannot be installed, because incompatible version of a dependency exists in the parent package. This prevents portal representation for node_modules installs without a need to write files into portal's target directory, which is forbidden for security reasons.
+
+**Workarounds** If the ranges for conflicting dependencies overlap between portal target and portal parent, the workaround is to use `yarn dedupe foo` (where `foo` is the conflicting dependency name) to upgrade the conflicting dependencies to the highest available versions, if `yarn dedupe` is used without arguments, all the dependencies across the project will be upgraded to the highest versions within their ranges in `package.json`. Another alternative is to use `link:` protocol instead of `portal:` and install dependencies inside the target directory explicitely.
 
 ## YN0072 - `NM_PRESERVE_SYMLINKS_REQUIRED`
 

packages/plugin-node-modules/sources/NodeModulesLinker.ts

@@ -274,7 +274,7 @@ class NodeModulesInstaller implements Installer {
       },
     };
 
-    const {tree, errors, preserveSymlinksRequired} = buildNodeModulesTree(pnpApi, {pnpifyFs: false, hoistingLimitsByCwd, project: this.opts.project});
+    const {tree, errors, preserveSymlinksRequired} = buildNodeModulesTree(pnpApi, {pnpifyFs: false, validateExternalSoftLinks: true, hoistingLimitsByCwd, project: this.opts.project});
     if (!tree) {
       for (const {messageName, text} of errors)
         this.opts.report.reportError(messageName, text);
@@ -555,7 +555,7 @@ type LocationTree = Map<LocationRoot, LocationNode>;
 const parseLocation = (location: PortablePath, {skipPrefix}: {skipPrefix: PortablePath}): {locationRoot: PortablePath, segments: Array<Filename>} => {
   const projectRelativePath = ppath.contains(skipPrefix, location);
   if (projectRelativePath === null)
-    throw new Error(`Assertion failed: Cannot process a path that isn't part of the requested prefix (${location} isn't within ${skipPrefix})`);
+    throw new Error(`Assertion failed: Writing attempt prevented to ${location} which is outside project root: ${skipPrefix}`);
 
   const allSegments = projectRelativePath
     .split(ppath.sep)
@@ -1070,7 +1070,7 @@ async function persistBinSymlinks(previousBinSymlinks: BinSymlinkMap, binSymlink
       } else {
         await xfs.removePromise(symlinkPath);
         await symlinkPromise(target, symlinkPath);
-        if (ppath.contains(target, projectCwd) !== null) {
+        if (ppath.contains(projectCwd, await xfs.realpathPromise(target)) !== null) {
           await xfs.chmodPromise(target, 0o755);
         }
       }

packages/yarnpkg-core/sources/MessageName.ts

@@ -73,7 +73,7 @@ export enum MessageName {
   UNUSED_PACKAGE_EXTENSION = 68,
   REDUNDANT_PACKAGE_EXTENSION = 69,
   AUTO_NM_SUCCESS = 70,
-  NM_CANT_INSTALL_PORTAL = 71,
+  NM_CANT_INSTALL_EXTERNAL_SOFT_LINK = 71,
   NM_PRESERVE_SYMLINKS_REQUIRED = 72,
 }
 

packages/yarnpkg-pnpify/sources/buildNodeModulesTree.ts

@@ -1,4 +1,4 @@
-import {structUtils, Project, MessageName}                                    from '@yarnpkg/core';
+import {structUtils, Project, MessageName, Locator}                           from '@yarnpkg/core';
 import {toFilename, npath, ppath}                                             from '@yarnpkg/fslib';
 import {NativePath, PortablePath, Filename}                                   from '@yarnpkg/fslib';
 import {PnpApi, PhysicalPackageLocator, PackageInformation, DependencyTarget} from '@yarnpkg/pnp';
@@ -51,6 +51,7 @@ export type NodeModulesTreeErrors = Array<{messageName: MessageName, text: strin
 
 export interface NodeModulesTreeOptions {
   pnpifyFs?: boolean;
+  validateExternalSoftLinks?: boolean;
   hoistingLimitsByCwd?: Map<PortablePath, NodeModulesHoistingLimits>;
   project?: Project;
 }
@@ -138,6 +139,12 @@ export const buildLocatorMap = (nodeModulesTree: NodeModulesTree): NodeModulesLo
   return map;
 };
 
+const areRealLocatorsEqual = (a: Locator, b: Locator) => {
+  const realA = structUtils.isVirtualLocator(a) ? structUtils.devirtualizeLocator(a) : a;
+  const realB = structUtils.isVirtualLocator(b) ? structUtils.devirtualizeLocator(b) : b;
+  return structUtils.areLocatorsEqual(realA, realB);
+};
+
 /**
  * Traverses PnP tree and produces input for the `RawHoister`
  *
@@ -224,6 +231,14 @@ const buildPackageTree = (pnp: PnpApi, options: NodeModulesTreeOptions): { packa
   const nodes = new Map<string, HoisterTree>();
   const getNodeKey = (name: string, locator: PhysicalPackageLocator) => `${stringifyLocator(locator)}:${name}`;
 
+  const isExternalSoftLink = (pkg: PackageInformation<NativePath>, locator: PhysicalPackageLocator) => {
+    if (pkg.linkType !== LinkType.SOFT || !options.project)
+      return false;
+
+    const realSoftLinkPath = npath.toPortablePath(pnp.resolveVirtual && locator.reference && locator.reference.startsWith(`virtual:`) ? pnp.resolveVirtual(pkg.packageLocation)! : pkg.packageLocation);
+    return ppath.contains(options.project.cwd, realSoftLinkPath) === null;
+  };
+
   const addPackageToTree = (name: string, pkg: PackageInformation<NativePath>, locator: PhysicalPackageLocator, parent: HoisterTree, parentDependencies: Map<string, DependencyTarget>, parentRelativeCwd: PortablePath, isHoistBorder: boolean) => {
     const nodeKey = getNodeKey(name, locator);
     let node = nodes.get(nodeKey);
@@ -234,22 +249,6 @@ const buildPackageTree = (pnp: PnpApi, options: NodeModulesTreeOptions): { packa
       nodes.set(nodeKey, packageTree);
     }
 
-    if (pkg.linkType === LinkType.SOFT && options.project && ppath.contains(options.project.cwd, npath.toPortablePath(pkg.packageLocation)) === null) {
-      if (pkg.packageDependencies.size > 0)
-        preserveSymlinksRequired = true;
-
-      for (const [name, referencish] of pkg.packageDependencies) {
-        const dependencyLocator = structUtils.parseLocator(Array.isArray(referencish) ? `${referencish[0]}@${referencish[1]}` : `${name}@${referencish}`);
-        const parentReferencish = parentDependencies.get(name);
-        if (parentReferencish) {
-          const parentDependencyLocator = structUtils.parseLocator(Array.isArray(parentReferencish) ? `${parentReferencish[0]}@${parentReferencish[1]}` : `${name}@${parentReferencish}`);
-          if (!structUtils.areLocatorsEqual(parentDependencyLocator, dependencyLocator)) {
-            errors.push({messageName: MessageName.NM_CANT_INSTALL_PORTAL, text: `Cannot link ${structUtils.prettyIdent(options.project.configuration, structUtils.parseIdent(locator.name))} into ${structUtils.prettyLocator(options.project.configuration, structUtils.parseLocator(`${parent.identName}@${parent.reference}`))} dependency ${structUtils.prettyLocator(options.project.configuration, dependencyLocator)} conflicts with parent dependency ${structUtils.prettyLocator(options.project.configuration, parentDependencyLocator)}`});
-          }
-        }
-      }
-    }
-
     if (!node) {
       node = {
         name,
@@ -262,7 +261,7 @@ const buildPackageTree = (pnp: PnpApi, options: NodeModulesTreeOptions): { packa
       nodes.set(nodeKey, node);
     }
 
-    if (isHoistBorder) {
+    if (isHoistBorder && !isExternalSoftLink(pkg, locator)) {
       const parentLocatorKey = stringifyLocator({name: parent.identName, reference: parent.reference});
       const dependencyBorders = hoistingLimits.get(parentLocatorKey) || new Set();
       hoistingLimits.set(parentLocatorKey, dependencyBorders);
@@ -298,6 +297,7 @@ const buildPackageTree = (pnp: PnpApi, options: NodeModulesTreeOptions): { packa
     parent.dependencies.add(node);
 
     if (!isSeen) {
+      const siblingPortalDependencyMap = new Map<string, {target: DependencyTarget, portal: PhysicalPackageLocator}>();
       for (const [depName, referencish] of allDependencies) {
         if (referencish !== null) {
           const depLocator = pnp.getLocator(depName, referencish);
@@ -306,9 +306,53 @@ const buildPackageTree = (pnp: PnpApi, options: NodeModulesTreeOptions): { packa
           const depPkg = pnp.getPackageInformation(pkgLocator);
           if (depPkg === null)
             throw new Error(`Assertion failed: Expected the package to have been registered`);
+          const isExternalSoftLinkDep = isExternalSoftLink(depPkg, depLocator);
+
+          if (options.validateExternalSoftLinks && options.project && isExternalSoftLinkDep) {
+            if (depPkg.packageDependencies.size > 0)
+              preserveSymlinksRequired = true;
+
+            for (const [name, referencish] of depPkg.packageDependencies) {
+              const portalDependencyLocator = structUtils.parseLocator(Array.isArray(referencish) ? `${referencish[0]}@${referencish[1]}` : `${name}@${referencish}`);
+              // Ignore self-references during portal hoistability check
+              if (stringifyLocator(portalDependencyLocator) !== stringifyLocator(depLocator)) {
+                const parentDependencyReferencish = allDependencies.get(name);
+                if (parentDependencyReferencish) {
+                  const parentDependencyLocator = structUtils.parseLocator(Array.isArray(parentDependencyReferencish) ? `${parentDependencyReferencish[0]}@${parentDependencyReferencish[1]}` : `${name}@${parentDependencyReferencish}`);
+                  if (!areRealLocatorsEqual(parentDependencyLocator, portalDependencyLocator)) {
+                    errors.push({
+                      messageName: MessageName.NM_CANT_INSTALL_EXTERNAL_SOFT_LINK,
+                      text: `Cannot link ${structUtils.prettyIdent(options.project.configuration, structUtils.parseIdent(depLocator.name))} ` +
+                        `into ${structUtils.prettyLocator(options.project.configuration, structUtils.parseLocator(`${locator.name}@${locator.reference}`))} ` +
+                        `dependency ${structUtils.prettyLocator(options.project.configuration, portalDependencyLocator)} ` +
+                        `conflicts with parent dependency ${structUtils.prettyLocator(options.project.configuration, parentDependencyLocator)}`,
+                    });
+                  }
+                } else {
+                  const siblingPortalDependency = siblingPortalDependencyMap.get(name);
+                  if (siblingPortalDependency) {
+                    const siblingReferncish = siblingPortalDependency.target;
+                    const siblingPortalDependencyLocator = structUtils.parseLocator(Array.isArray(siblingReferncish) ? `${siblingReferncish[0]}@${siblingReferncish[1]}` : `${name}@${siblingReferncish}`);
+                    if (!areRealLocatorsEqual(siblingPortalDependencyLocator, portalDependencyLocator)) {
+                      errors.push({
+                        messageName: MessageName.NM_CANT_INSTALL_EXTERNAL_SOFT_LINK,
+                        text: `Cannot link ${structUtils.prettyIdent(options.project.configuration, structUtils.parseIdent(depLocator.name))} ` +
+                          `into ${structUtils.prettyLocator(options.project.configuration, structUtils.parseLocator(`${locator.name}@${locator.reference}`))} ` +
+                          `dependency ${structUtils.prettyLocator(options.project.configuration, portalDependencyLocator)} ` +
+                          `conflicts with dependency ${structUtils.prettyLocator(options.project.configuration, siblingPortalDependencyLocator)} ` +
+                          `from sibling portal ${structUtils.prettyIdent(options.project.configuration, structUtils.parseIdent(siblingPortalDependency.portal.name))}`,
+                      });
+                    }
+                  } else {
+                    siblingPortalDependencyMap.set(name, {target: portalDependencyLocator.reference, portal: depLocator});
+                  }
+                }
+              }
+            }
+          }
 
           const parentHoistingLimits = options.hoistingLimitsByCwd?.get(parentRelativeCwd);
-          const relativeDepCwd = ppath.relative(topPkgPortableLocation, npath.toPortablePath(depPkg.packageLocation)) || PortablePath.dot;
+          const relativeDepCwd = isExternalSoftLinkDep ? parentRelativeCwd : ppath.relative(topPkgPortableLocation, npath.toPortablePath(depPkg.packageLocation)) || PortablePath.dot;
           const depHoistingLimits = options.hoistingLimitsByCwd?.get(relativeDepCwd);
           const isHoistBorder = parentHoistingLimits === NodeModulesHoistingLimits.DEPENDENCIES
             || depHoistingLimits === NodeModulesHoistingLimits.DEPENDENCIES

packages/yarnpkg-pnpify/sources/hoist.ts

@@ -196,8 +196,10 @@ const getUsedDependencies = (rootNodePath: Array<HoisterWorkTree>): Map<PackageN
 
     for (const dep of node.hoistedDependencies.values()) {
       if (!hiddenDependencies.has(dep.name)) {
-        const reachableDependency = reachableDependencies.get(dep.name)!;
-        usedDependencies.set(reachableDependency.name, reachableDependency);
+        const reachableDependency = reachableDependencies.get(dep.name);
+        if (reachableDependency) {
+          usedDependencies.set(reachableDependency.name, reachableDependency);
+        }
       }
     }