fix(nm): Gives hoisting priority to direct portal dependencies

.yarn/versions/614d282a.yml

@@ -0,0 +1,25 @@
+releases:
+  "@yarnpkg/cli": patch
+  "@yarnpkg/nm": major
+  "@yarnpkg/plugin-nm": 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"

CHANGELOG.md

@@ -8,6 +8,10 @@ Yarn now accepts sponsorships! Please give a look at our [OpenCollective](https:
 
 **Note:** features in `master` can be tried out by running `yarn set version from sources` in your project (existing contrib plugins are updated automatically, while new contrib plugins can be added by running `yarn plugin import from sources <name>`).
 
+### Bugfixes
+
+- Direct portal dependencies for `node_modules` install are given priority during hoisting now, to prevent cases when indirect regular dependencies take place in the install tree first and block the way for direct portal dependencies.
+
 ### Installs
 
 - `hardlinks-global` node modules mode is automatically downgraded to `hardlinks-local` when global cache and install folder are on a different devices and the install continues normally. Warning is produced to the user with mitigation steps provided in documentation.

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

@@ -1070,6 +1070,33 @@ describe(`Node_Modules`, () => {
     })
   );
 
+  test(`should give a priority to direct portal dependencies over indirect regular dependencies`,
+    makeTemporaryEnv({},
+      {
+        nodeLinker: `node-modules`,
+      },
+      async ({path, run}) => {
+        await xfs.mktempPromise(async portalTarget => {
+          await xfs.writeJsonPromise(`${portalTarget}/package.json` as PortablePath, {
+            name: `portal`,
+            dependencies: {
+              'no-deps': `2.0.0`,
+            },
+          });
+
+          await xfs.writeJsonPromise(`${path}/package.json` as PortablePath, {
+            dependencies: {
+              portal: `portal:${portalTarget}`,
+              'one-fixed-dep': `1.0.0`,
+              'one-range-dep': `1.0.0`,
+            },
+          });
+
+          await expect(run(`install`)).resolves.not.toThrow();
+        });
+      })
+  );
+
   test(
     `should not hoist dependencies in nested workspaces when using nmHoistingLimits`,
     makeTemporaryEnv(

packages/yarnpkg-nm/sources/buildNodeModulesTree.ts

@@ -248,19 +248,22 @@ const buildPackageTree = (pnp: PnpApi, options: NodeModulesTreeOptions): { packa
       nodes.set(nodeKey, packageTree);
     }
 
+    const isExternalSoftLinkPackage = isExternalSoftLink(pkg, locator);
+
     if (!node) {
       node = {
         name,
         identName: locator.name,
         reference: locator.reference,
         dependencies: new Set(),
         peerNames: pkg.packagePeers,
+        hoistPriority: isExternalSoftLinkPackage ? 1 : 0,
       };
 
       nodes.set(nodeKey, node);
     }
 
-    if (isHoistBorder && !isExternalSoftLink(pkg, locator)) {
+    if (isHoistBorder && !isExternalSoftLinkPackage) {
       const parentLocatorKey = stringifyLocator({name: parent.identName, reference: parent.reference});
       const dependencyBorders = hoistingLimits.get(parentLocatorKey) || new Set();
       hoistingLimits.set(parentLocatorKey, dependencyBorders);

packages/yarnpkg-nm/sources/hoist.ts

@@ -46,18 +46,18 @@
  * until you run out of candidates for current tree root.
  */
 type PackageName = string;
-export type HoisterTree = {name: PackageName, identName: PackageName, reference: string, dependencies: Set<HoisterTree>, peerNames: Set<PackageName>};
+export type HoisterTree = {name: PackageName, identName: PackageName, reference: string, dependencies: Set<HoisterTree>, peerNames: Set<PackageName>, hoistPriority?: number};
 export type HoisterResult = {name: PackageName, identName: PackageName, references: Set<string>, dependencies: Set<HoisterResult>};
 type Locator = string;
 type Ident = string;
-type HoisterWorkTree = {name: PackageName, references: Set<string>, ident: Ident, locator: Locator, dependencies: Map<PackageName, HoisterWorkTree>, originalDependencies: Map<PackageName, HoisterWorkTree>, hoistedDependencies: Map<PackageName, HoisterWorkTree>, peerNames: ReadonlySet<PackageName>, decoupled: boolean, reasons: Map<PackageName, string>, isHoistBorder: boolean, hoistedFrom: Array<string>};
+type HoisterWorkTree = {name: PackageName, references: Set<string>, ident: Ident, locator: Locator, dependencies: Map<PackageName, HoisterWorkTree>, originalDependencies: Map<PackageName, HoisterWorkTree>, hoistedDependencies: Map<PackageName, HoisterWorkTree>, peerNames: ReadonlySet<PackageName>, decoupled: boolean, reasons: Map<PackageName, string>, isHoistBorder: boolean, hoistedFrom: Array<string>, hoistPriority: number};
 
 /**
  * Mapping which packages depend on a given package alias + ident. It is used to determine hoisting weight,
  * e.g. which one among the group of packages with the same name should be hoisted.
  * The package having the biggest number of parents using this package will be hoisted.
  */
-type PreferenceMap = Map<string, { peerDependents: Set<Ident>, dependents: Set<Ident> }>;
+type PreferenceMap = Map<string, { peerDependents: Set<Ident>, dependents: Set<Ident>, hoistPriority: number }>;
 
 enum Hoistable {
   YES, NO, DEPENDS,
@@ -239,7 +239,7 @@ const decoupleGraphNode = (parent: HoisterWorkTree, node: HoisterWorkTree): Hois
   if (node.decoupled)
     return node;
 
-  const {name, references, ident, locator, dependencies, originalDependencies, hoistedDependencies, peerNames, reasons, isHoistBorder} = node;
+  const {name, references, ident, locator, dependencies, originalDependencies, hoistedDependencies, peerNames, reasons, isHoistBorder, hoistPriority} = node;
   // To perform node hoisting from parent node we must clone parent nodes up to the root node,
   // because some other package in the tree might depend on the parent package where hoisting
   // cannot be performed
@@ -255,6 +255,7 @@ const decoupleGraphNode = (parent: HoisterWorkTree, node: HoisterWorkTree): Hois
     reasons: new Map(reasons),
     decoupled: true,
     isHoistBorder,
+    hoistPriority,
     hoistedFrom: [],
   };
   const selfDep = clone.dependencies.get(name);
@@ -290,7 +291,9 @@ const getHoistIdentMap = (rootNode: HoisterWorkTree, preferenceMap: PreferenceMa
   keyList.sort((key1, key2) => {
     const entry1 = preferenceMap.get(key1)!;
     const entry2 = preferenceMap.get(key2)!;
-    if (entry2.peerDependents.size !== entry1.peerDependents.size) {
+    if (entry2.hoistPriority !== entry1.hoistPriority) {
+      return entry2.hoistPriority - entry1.hoistPriority;
+    } else if (entry2.peerDependents.size !== entry1.peerDependents.size) {
       return entry2.peerDependents.size - entry1.peerDependents.size;
     } else {
       return entry2.dependents.size - entry1.dependents.size;
@@ -724,6 +727,7 @@ const cloneTree = (tree: HoisterTree, options: InternalHoistOptions): HoisterWor
     reasons: new Map(),
     decoupled: true,
     isHoistBorder: true,
+    hoistPriority: 0,
     hoistedFrom: [],
   };
 
@@ -733,7 +737,7 @@ const cloneTree = (tree: HoisterTree, options: InternalHoistOptions): HoisterWor
     let workNode = seenNodes.get(node);
     const isSeen = !!workNode;
     if (!workNode) {
-      const {name, identName, reference, peerNames} = node;
+      const {name, identName, reference, peerNames, hoistPriority} = node;
       const dependenciesNmHoistingLimits = options.hoistingLimits.get(parentNode.locator);
       workNode = {
         name,
@@ -747,6 +751,7 @@ const cloneTree = (tree: HoisterTree, options: InternalHoistOptions): HoisterWor
         reasons: new Map(),
         decoupled: true,
         isHoistBorder: dependenciesNmHoistingLimits ? dependenciesNmHoistingLimits.has(name) : false,
+        hoistPriority: hoistPriority || 0,
         hoistedFrom: [],
       };
       seenNodes.set(node, workNode);
@@ -854,7 +859,7 @@ const buildPreferenceMap = (rootNode: HoisterWorkTree): PreferenceMap => {
     const key = getPreferenceKey(node);
     let entry = preferenceMap.get(key);
     if (!entry) {
-      entry = {dependents: new Set<Ident>(), peerDependents: new Set<Ident>()};
+      entry = {dependents: new Set<Ident>(), peerDependents: new Set<Ident>(), hoistPriority: 0};
       preferenceMap.set(key, entry);
     }
     return entry;
@@ -869,8 +874,9 @@ const buildPreferenceMap = (rootNode: HoisterWorkTree): PreferenceMap => {
     if (!isSeen) {
       seenNodes.add(node);
       for (const dep of node.dependencies.values()) {
+        const entry = getOrCreatePreferenceEntry(dep);
+        entry.hoistPriority = Math.max(entry.hoistPriority, node.hoistPriority);
         if (node.peerNames.has(dep.name)) {
-          const entry = getOrCreatePreferenceEntry(dep);
           entry.peerDependents.add(node.ident);
         } else {
           addDependent(node, dep);