From 7e0f2d10582b64c0f27aef5436abfb20fa92a71a Mon Sep 17 00:00:00 2001 From: Victor Vlasenko Date: Tue, 27 Jul 2021 12:35:13 +0300 Subject: [PATCH 1/4] Gives hoisting priority to direct portal dependencies --- .yarn/versions/614d282a.yml | 25 +++++++++++++++++++ .../sources/buildNodeModulesTree.ts | 5 +++- packages/yarnpkg-nm/sources/hoist.ts | 23 +++++++++++------ 3 files changed, 44 insertions(+), 9 deletions(-) create mode 100644 .yarn/versions/614d282a.yml diff --git a/.yarn/versions/614d282a.yml b/.yarn/versions/614d282a.yml new file mode 100644 index 000000000000..a4379ae0468c --- /dev/null +++ b/.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" diff --git a/packages/yarnpkg-nm/sources/buildNodeModulesTree.ts b/packages/yarnpkg-nm/sources/buildNodeModulesTree.ts index 1595d3affcb9..aa8c6ef3b768 100644 --- a/packages/yarnpkg-nm/sources/buildNodeModulesTree.ts +++ b/packages/yarnpkg-nm/sources/buildNodeModulesTree.ts @@ -248,6 +248,8 @@ const buildPackageTree = (pnp: PnpApi, options: NodeModulesTreeOptions): { packa nodes.set(nodeKey, packageTree); } + const isExternalSoftLinkPackage = isExternalSoftLink(pkg, locator); + if (!node) { node = { name, @@ -255,12 +257,13 @@ const buildPackageTree = (pnp: PnpApi, options: NodeModulesTreeOptions): { packa reference: locator.reference, dependencies: new Set(), peerNames: pkg.packagePeers, + isExternalSoftLink: isExternalSoftLinkPackage, }; 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); diff --git a/packages/yarnpkg-nm/sources/hoist.ts b/packages/yarnpkg-nm/sources/hoist.ts index 1228058c18c6..1c8f537d3c89 100644 --- a/packages/yarnpkg-nm/sources/hoist.ts +++ b/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, peerNames: Set}; +export type HoisterTree = {name: PackageName, identName: PackageName, reference: string, dependencies: Set, peerNames: Set, isExternalSoftLink?: boolean}; export type HoisterResult = {name: PackageName, identName: PackageName, references: Set, dependencies: Set}; type Locator = string; type Ident = string; -type HoisterWorkTree = {name: PackageName, references: Set, ident: Ident, locator: Locator, dependencies: Map, originalDependencies: Map, hoistedDependencies: Map, peerNames: ReadonlySet, decoupled: boolean, reasons: Map, isHoistBorder: boolean, hoistedFrom: Array}; +type HoisterWorkTree = {name: PackageName, references: Set, ident: Ident, locator: Locator, dependencies: Map, originalDependencies: Map, hoistedDependencies: Map, peerNames: ReadonlySet, decoupled: boolean, reasons: Map, isHoistBorder: boolean, hoistedFrom: Array, isExternalSoftLink: boolean}; /** * 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, dependents: Set }>; +type PreferenceMap = Map, dependents: Set, 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, isExternalSoftLink} = 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, + isExternalSoftLink, 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, + isExternalSoftLink: false, 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, isExternalSoftLink} = 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, + isExternalSoftLink: !!isExternalSoftLink, 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(), peerDependents: new Set()}; + entry = {dependents: new Set(), peerDependents: new Set(), hoistPriority: 0}; preferenceMap.set(key, entry); } return entry; @@ -869,8 +874,10 @@ const buildPreferenceMap = (rootNode: HoisterWorkTree): PreferenceMap => { if (!isSeen) { seenNodes.add(node); for (const dep of node.dependencies.values()) { + const entry = getOrCreatePreferenceEntry(dep); + if (node.isExternalSoftLink) + entry.hoistPriority = 1; if (node.peerNames.has(dep.name)) { - const entry = getOrCreatePreferenceEntry(dep); entry.peerDependents.add(node.ident); } else { addDependent(node, dep); From 9dc487e681c9ae88fcae317421361bf7e07d51e0 Mon Sep 17 00:00:00 2001 From: Victor Vlasenko Date: Tue, 27 Jul 2021 13:08:17 +0300 Subject: [PATCH 2/4] Adds integration test --- .../sources/node-modules.test.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts b/packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts index 9c3b8a367681..44674f419418 100644 --- a/packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts +++ b/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( From 04964544a54b588365ba2cc2f3515e48617ba440 Mon Sep 17 00:00:00 2001 From: Victor Vlasenko Date: Tue, 27 Jul 2021 14:36:31 +0300 Subject: [PATCH 3/4] Adds changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a8685ce7eac..c05b1a67b11e 100644 --- a/CHANGELOG.md +++ b/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 `). +### 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. + ## 3.0.0 ### **Breaking Changes** From 6381d0028a1a39688aaac78c5ede851e0349b2ff Mon Sep 17 00:00:00 2001 From: Victor Vlasenko Date: Thu, 29 Jul 2021 15:25:46 +0300 Subject: [PATCH 4/4] Split responsibility better between hoister and tree builder --- .../yarnpkg-nm/sources/buildNodeModulesTree.ts | 2 +- packages/yarnpkg-nm/sources/hoist.ts | 17 ++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/yarnpkg-nm/sources/buildNodeModulesTree.ts b/packages/yarnpkg-nm/sources/buildNodeModulesTree.ts index aa8c6ef3b768..74cf0541c9fb 100644 --- a/packages/yarnpkg-nm/sources/buildNodeModulesTree.ts +++ b/packages/yarnpkg-nm/sources/buildNodeModulesTree.ts @@ -257,7 +257,7 @@ const buildPackageTree = (pnp: PnpApi, options: NodeModulesTreeOptions): { packa reference: locator.reference, dependencies: new Set(), peerNames: pkg.packagePeers, - isExternalSoftLink: isExternalSoftLinkPackage, + hoistPriority: isExternalSoftLinkPackage ? 1 : 0, }; nodes.set(nodeKey, node); diff --git a/packages/yarnpkg-nm/sources/hoist.ts b/packages/yarnpkg-nm/sources/hoist.ts index 1c8f537d3c89..cc92ed3b9475 100644 --- a/packages/yarnpkg-nm/sources/hoist.ts +++ b/packages/yarnpkg-nm/sources/hoist.ts @@ -46,11 +46,11 @@ * until you run out of candidates for current tree root. */ type PackageName = string; -export type HoisterTree = {name: PackageName, identName: PackageName, reference: string, dependencies: Set, peerNames: Set, isExternalSoftLink?: boolean}; +export type HoisterTree = {name: PackageName, identName: PackageName, reference: string, dependencies: Set, peerNames: Set, hoistPriority?: number}; export type HoisterResult = {name: PackageName, identName: PackageName, references: Set, dependencies: Set}; type Locator = string; type Ident = string; -type HoisterWorkTree = {name: PackageName, references: Set, ident: Ident, locator: Locator, dependencies: Map, originalDependencies: Map, hoistedDependencies: Map, peerNames: ReadonlySet, decoupled: boolean, reasons: Map, isHoistBorder: boolean, hoistedFrom: Array, isExternalSoftLink: boolean}; +type HoisterWorkTree = {name: PackageName, references: Set, ident: Ident, locator: Locator, dependencies: Map, originalDependencies: Map, hoistedDependencies: Map, peerNames: ReadonlySet, decoupled: boolean, reasons: Map, isHoistBorder: boolean, hoistedFrom: Array, hoistPriority: number}; /** * Mapping which packages depend on a given package alias + ident. It is used to determine hoisting weight, @@ -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, isExternalSoftLink} = 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,7 +255,7 @@ const decoupleGraphNode = (parent: HoisterWorkTree, node: HoisterWorkTree): Hois reasons: new Map(reasons), decoupled: true, isHoistBorder, - isExternalSoftLink, + hoistPriority, hoistedFrom: [], }; const selfDep = clone.dependencies.get(name); @@ -727,7 +727,7 @@ const cloneTree = (tree: HoisterTree, options: InternalHoistOptions): HoisterWor reasons: new Map(), decoupled: true, isHoistBorder: true, - isExternalSoftLink: false, + hoistPriority: 0, hoistedFrom: [], }; @@ -737,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, isExternalSoftLink} = node; + const {name, identName, reference, peerNames, hoistPriority} = node; const dependenciesNmHoistingLimits = options.hoistingLimits.get(parentNode.locator); workNode = { name, @@ -751,7 +751,7 @@ const cloneTree = (tree: HoisterTree, options: InternalHoistOptions): HoisterWor reasons: new Map(), decoupled: true, isHoistBorder: dependenciesNmHoistingLimits ? dependenciesNmHoistingLimits.has(name) : false, - isExternalSoftLink: !!isExternalSoftLink, + hoistPriority: hoistPriority || 0, hoistedFrom: [], }; seenNodes.set(node, workNode); @@ -875,8 +875,7 @@ const buildPreferenceMap = (rootNode: HoisterWorkTree): PreferenceMap => { seenNodes.add(node); for (const dep of node.dependencies.values()) { const entry = getOrCreatePreferenceEntry(dep); - if (node.isExternalSoftLink) - entry.hoistPriority = 1; + entry.hoistPriority = Math.max(entry.hoistPriority, node.hoistPriority); if (node.peerNames.has(dep.name)) { entry.peerDependents.add(node.ident); } else {