Skip to content

Commit 5fd40b6

Browse files
manzoorwanijkmanzoorwanijk
committed
fix(arborist): do not hoist undeclared workspaces in linked strategy (#9076)
1 parent f95f368 commit 5fd40b6

2 files changed

Lines changed: 165 additions & 45 deletions

File tree

‎workspaces/arborist/lib/arborist/isolated-reifier.js‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ const getKey = (startNode) => {
3434

3535
module.exports = cls => class IsolatedReifier extends cls {
3636
#externalProxies = new Map()
37+
#rootDeclaredDeps = new Set()
3738
#processedEdges = new Set()
3839
#workspaceProxies = new Map()
3940

@@ -73,6 +74,15 @@ module.exports = cls => class IsolatedReifier extends cls {
7374
const idealTree = this.idealTree
7475
const omit = new Set(this.options.omit)
7576

77+
// npm auto-creates 'workspace' edges from root to all workspaces.
78+
// For isolated/linked mode, only include workspaces that root explicitly declares as dependencies.
79+
const rootPkg = idealTree.package
80+
this.#rootDeclaredDeps = new Set([
81+
...Object.keys(rootPkg.dependencies || {}),
82+
...Object.keys(rootPkg.devDependencies || {}),
83+
...Object.keys(rootPkg.optionalDependencies || {}),
84+
])
85+
7686
// XXX this sometimes acts like a node too
7787
this.idealGraph = {
7888
external: [],
@@ -190,7 +200,13 @@ module.exports = cls => class IsolatedReifier extends cls {
190200
edge.to?.target &&
191201
!(node.package.bundledDependencies || node.package.bundleDependencies)?.includes(edge.to.name)
192202
)
193-
const nonOptionalDeps = edges.filter(edge => !edge.optional).map(edge => edge.to.target)
203+
let nonOptionalDeps = edges.filter(edge => !edge.optional).map(edge => edge.to.target)
204+
205+
// npm auto-creates 'workspace' edges from root to all workspaces.
206+
// For isolated/linked mode, only include workspaces that root explicitly declares as dependencies.
207+
if (node.isProjectRoot) {
208+
nonOptionalDeps = nonOptionalDeps.filter(n => !n.isWorkspace || this.#rootDeclaredDeps.has(n.packageName))
209+
}
194210

195211
// When legacyPeerDeps is enabled, peer dep edges are not created on the node.
196212
// Resolve them from the tree so they get symlinked in the store.
@@ -293,22 +309,26 @@ module.exports = cls => class IsolatedReifier extends cls {
293309
workspace.isWorkspace = true
294310
root.fsChildren.add(workspace)
295311
root.inventory.set(workspace.location, workspace)
312+
root.workspaces.set(wsName, workspace.path)
296313

297-
// Create workspace Link entry in children for _diffTrees lookup
314+
// Create workspace Link. For root declared deps, link at root node_modules/. For undeclared deps, link at the workspace's own node_modules/ (self-link).
315+
const isDeclared = this.#rootDeclaredDeps.has(wsName)
298316
const wsLink = new IsolatedLink({
299-
location: join('node_modules', wsName),
317+
location: isDeclared ? join('node_modules', wsName) : join(c.localLocation, 'node_modules', wsName),
300318
name: wsName,
301319
package: workspace.package,
302320
parent: root,
303-
path: join(root.path, 'node_modules', wsName),
321+
path: isDeclared ? join(root.path, 'node_modules', wsName) : join(root.path, c.localLocation, 'node_modules', wsName),
304322
realpath: workspace.path,
305323
root,
306324
target: workspace,
307325
})
308326
wsLink.top = root
309-
root.children.set(wsLink.name, wsLink)
327+
if (!isDeclared) {
328+
workspace.children.set(wsName, wsLink)
329+
}
330+
root.children.set(wsName, wsLink)
310331
root.inventory.set(wsLink.location, wsLink)
311-
root.workspaces.set(wsName, workspace.path)
312332
workspace.linksIn.add(wsLink)
313333
}
314334

‎workspaces/arborist/test/isolated-mode.js‎

Lines changed: 139 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -404,10 +404,12 @@ tap.test('idempotent install with legacyPeerDeps and workspace peer deps', async
404404
const arb2 = new Arborist({ ...opts, packumentCache: new Map() })
405405
await arb2.reify({ installStrategy: 'linked' })
406406

407-
// Workspace symlinks should still be symlinks (not directories)
407+
// Workspace peer dep symlinks should still be present after second install
408408
for (let i = 0; i < 20; i++) {
409-
t.ok(fs.lstatSync(path.join(dir, 'node_modules', `ws-${i}`)).isSymbolicLink(),
410-
`ws-${i} is still a symlink after second install`)
409+
const peerTarget = `ws-${(i + 1) % 20}`
410+
const peerLink = path.join(dir, 'packages', `ws-${i}`, 'node_modules', peerTarget)
411+
t.ok(fs.lstatSync(peerLink).isSymbolicLink(),
412+
`ws-${i} peer dep on ${peerTarget} is still a symlink after second install`)
411413
}
412414
})
413415

@@ -465,32 +467,36 @@ tap.test('Basic workspaces setup', async t => {
465467
'[email protected]': {},
466468
},
467469
},
468-
'[email protected] (workspace)': {
469-
'[email protected] (workspace)': {
470-
'[email protected]': {
471-
'[email protected]': {},
472-
},
473-
},
470+
},
471+
'[email protected] (workspace)': {
472+
'[email protected]': {
473+
'[email protected]': {},
474+
},
475+
},
476+
'[email protected] (workspace)': {
477+
'[email protected] (workspace)': {
474478
'[email protected]': {
475479
'[email protected]': {},
476480
},
477481
},
482+
'[email protected]': {
483+
'[email protected]': {},
484+
},
485+
},
486+
'[email protected] (workspace)': {
487+
'[email protected]': {
488+
'[email protected]': {},
489+
},
490+
},
491+
'[email protected] (workspace)': {
478492
'[email protected] (workspace)': {
479493
'[email protected]': {
480494
'[email protected]': {},
481495
},
482496
},
483-
'[email protected] (workspace)': {
484-
'[email protected] (workspace)': {
485-
'[email protected]': {
486-
'[email protected]': {},
487-
},
488-
},
489-
'[email protected]': {
490-
'[email protected]': {},
491-
},
497+
'[email protected]': {
498+
'[email protected]': {},
492499
},
493-
'[email protected]': {},
494500
},
495501
}
496502

@@ -1075,7 +1081,10 @@ tap.test('nested bundled dependencies of workspaces', async t => {
10751081

10761082
const resolved = {
10771083
'[email protected] (root)': {
1078-
'[email protected] (workspace)': {},
1084+
'[email protected]': {},
1085+
'[email protected]': {},
1086+
},
1087+
'[email protected] (workspace)': {
10791088
'[email protected]': {},
10801089
'[email protected]': {},
10811090
},
@@ -1093,12 +1102,14 @@ tap.test('nested bundled dependencies of workspaces', async t => {
10931102
rule1.apply(t, dir, resolved, asserted)
10941103
rule2.apply(t, dir, resolved, asserted)
10951104
rule3.apply(t, dir, resolved, asserted)
1096-
rule4.apply(t, dir, resolved, asserted)
1105+
// Workspace link at packages/bar/node_modules/bar and bundled deps (which, isexe) are co-located in the same node_modules/, making them mutually accessible regardless of declared dependencies.
1106+
// rule4.apply(t, dir, resolved, asserted)
10971107
rule5.apply(t, dir, resolved, asserted)
10981108
// I think that duplicated versions are okay in the case of bundled deps
10991109
// rule6.apply(t, dir, resolved, asserted)
11001110
rule7.apply(t, dir, resolved, asserted)
11011111

1112+
// Bundled deps are hoisted to root node_modules as real directories
11021113
const isexePath = path.join(dir, 'node_modules', 'isexe')
11031114
t.equal(isexePath, fs.realpathSync(isexePath))
11041115
const whichPath = path.join(dir, 'node_modules', 'which')
@@ -1125,16 +1136,16 @@ tap.test('nested bundled dependencies of workspaces with conflicting isolated de
11251136
// the 'which' that is bundled is not hoisted due to a conflict
11261137
const resolved = {
11271138
'[email protected] (root)': {
1128-
'[email protected] (workspace)': {
1129-
'[email protected]': {
1130-
'[email protected]': {},
1131-
},
1132-
'[email protected]': {},
1133-
},
11341139
'[email protected]': {
11351140
'[email protected]': {},
11361141
},
11371142
},
1143+
'[email protected] (workspace)': {
1144+
'[email protected]': {
1145+
'[email protected]': {},
1146+
},
1147+
'[email protected]': {},
1148+
},
11381149
}
11391150

11401151
const { dir, registry } = await getRepo(graph)
@@ -1149,7 +1160,8 @@ tap.test('nested bundled dependencies of workspaces with conflicting isolated de
11491160
rule1.apply(t, dir, resolved, asserted)
11501161
rule2.apply(t, dir, resolved, asserted)
11511162
rule3.apply(t, dir, resolved, asserted)
1152-
rule4.apply(t, dir, resolved, asserted)
1163+
// Workspace link at packages/bar/node_modules/bar and bundled deps (which, isexe) share the same node_modules/, making them mutually accessible regardless of declared dependencies.
1164+
// rule4.apply(t, dir, resolved, asserted)
11531165
rule5.apply(t, dir, resolved, asserted)
11541166
// I think that duplicated versions are okay in the case of bundled deps
11551167
// rule6.apply(t, dir, resolved, asserted)
@@ -1481,10 +1493,10 @@ tap.test('aliased packages in workspace', async t => {
14811493
'[email protected]': {
14821494
'[email protected]': {},
14831495
},
1484-
'[email protected] (workspace)': {
1485-
'prettier@3.0.3': {
1486-
'isexe@1.0.0': {},
1487-
},
1496+
},
1497+
'my-pkg@1.0.0 (workspace)': {
1498+
'prettier@3.0.3': {
1499+
'[email protected]': {},
14881500
},
14891501
},
14901502
}
@@ -1610,7 +1622,7 @@ tap.test('postinstall scripts are run', async t => {
16101622
const postInstallRanWhich = pathExists(`${setupRequire(dir)('which')}/postInstallRanWhich`)
16111623
t.ok(postInstallRanWhich)
16121624

1613-
const postInstallRanBar = pathExists(`${setupRequire(dir)('bar')}/postInstallRanBar`)
1625+
const postInstallRanBar = pathExists(`${path.join(dir, 'packages', 'bar')}/postInstallRanBar`)
16141626
t.ok(postInstallRanBar)
16151627
})
16161628

@@ -1729,10 +1741,8 @@ tap.test('bins are installed', async t => {
17291741
const binFromRootToWhich = pathExists(`${dir}/node_modules/.bin/which`)
17301742
t.ok(binFromRootToWhich)
17311743

1732-
const binFromRootToBar = pathExists(`${dir}/node_modules/.bin/bar`)
1733-
t.ok(binFromRootToBar)
1734-
1735-
const binFromBarToWhich = pathExists(`${setupRequire(dir)('bar')}/node_modules/.bin/which`)
1744+
// bar is not a root dep, so its bin should be in the workspace's own node_modules
1745+
const binFromBarToWhich = pathExists(`${path.join(dir, 'packages', 'bar')}/node_modules/.bin/which`)
17361746
t.ok(binFromBarToWhich)
17371747
})
17381748

@@ -1845,8 +1855,8 @@ tap.test('workspace links are not affected by store resolved fix', async t => {
18451855
const arb2 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache })
18461856
await arb2.reify({ installStrategy: 'linked' })
18471857

1848-
// Verify workspace is still correctly linked
1849-
t.ok(setupRequire(dir)('mypkg'), 'workspace is requireable after second install')
1858+
// Verify workspace is still correctly linked (workspace can resolve itself via self-link)
1859+
t.ok(setupRequire(path.join(dir, 'packages', 'mypkg'))('mypkg'), 'workspace is requireable via self-link after second install')
18501860
t.ok(setupRequire(dir)('abbrev'), 'registry dep is requireable after second install')
18511861

18521862
// Verify the diff has unchanged nodes (store entries are correctly matched)
@@ -2055,6 +2065,96 @@ function parseGraphRecursive (key, deps) {
20552065
return { name, version, workspace, peer, dependencies }
20562066
}
20572067

2068+
tap.test('undeclared workspaces are not hoisted to root node_modules', async t => {
2069+
// Regression test: all workspace packages were unconditionally symlinked into root node_modules/.
2070+
// Only workspaces that root explicitly depends on should appear at root node_modules/.
2071+
const graph = {
2072+
registry: [
2073+
{ name: 'which', version: '1.0.0', dependencies: { isexe: '^1.0.0' } },
2074+
{ name: 'isexe', version: '1.0.0' },
2075+
],
2076+
root: {
2077+
name: 'myapp',
2078+
version: '1.0.0',
2079+
dependencies: { 'ws-a': '*' },
2080+
},
2081+
workspaces: [
2082+
{ name: 'ws-a', version: '1.0.0', dependencies: { 'ws-b': '*', which: '1.0.0' } },
2083+
{ name: 'ws-b', version: '1.0.0' },
2084+
{ name: 'ws-c', version: '1.0.0' },
2085+
],
2086+
}
2087+
2088+
const { dir, registry } = await getRepo(graph)
2089+
const cache = fs.mkdtempSync(`${getTempDir()}/test-`)
2090+
const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache })
2091+
await arborist.reify({ installStrategy: 'linked' })
2092+
2093+
// ws-a is declared as a root dependency — should be at root node_modules
2094+
t.ok(pathExists(path.join(dir, 'node_modules', 'ws-a')),
2095+
'declared workspace ws-a is symlinked at root node_modules')
2096+
t.ok(fs.lstatSync(path.join(dir, 'node_modules', 'ws-a')).isSymbolicLink(),
2097+
'ws-a at root is a symlink')
2098+
2099+
// ws-b is NOT a root dependency — should NOT be at root node_modules
2100+
t.notOk(pathExists(path.join(dir, 'node_modules', 'ws-b')),
2101+
'undeclared workspace ws-b is NOT at root node_modules')
2102+
2103+
// ws-c is NOT a root dependency — should NOT be at root node_modules
2104+
t.notOk(pathExists(path.join(dir, 'node_modules', 'ws-c')),
2105+
'undeclared workspace ws-c is NOT at root node_modules')
2106+
2107+
// ws-b should be resolvable from ws-a (ws-a depends on ws-b)
2108+
t.ok(pathExists(path.join(dir, 'packages', 'ws-a', 'node_modules', 'ws-b')),
2109+
'ws-b is linked in ws-a/node_modules (declared dep)')
2110+
2111+
// ws-c has no dependencies and is not depended on — should not be able to access ws-b
2112+
t.notOk(pathExists(path.join(dir, 'packages', 'ws-c', 'node_modules', 'ws-b')),
2113+
'ws-c cannot access ws-b (no dependency declared)')
2114+
})
2115+
2116+
tap.test('omit dev dependencies with linked strategy', async t => {
2117+
const graph = {
2118+
registry: [
2119+
{ name: 'which', version: '1.0.0', dependencies: { isexe: '^1.0.0' } },
2120+
{ name: 'isexe', version: '1.0.0' },
2121+
{ name: 'eslint', version: '1.0.0' },
2122+
],
2123+
root: {
2124+
name: 'myapp',
2125+
version: '1.0.0',
2126+
dependencies: { which: '1.0.0' },
2127+
devDependencies: { eslint: '1.0.0' },
2128+
},
2129+
workspaces: [
2130+
{
2131+
name: 'mylib',
2132+
version: '1.0.0',
2133+
dependencies: { isexe: '1.0.0' },
2134+
devDependencies: { eslint: '1.0.0' },
2135+
},
2136+
],
2137+
}
2138+
2139+
const { dir, registry } = await getRepo(graph)
2140+
const cache = fs.mkdtempSync(`${getTempDir()}/test-`)
2141+
const arborist = new Arborist({
2142+
path: dir,
2143+
registry,
2144+
packumentCache: new Map(),
2145+
cache,
2146+
omit: ['dev'],
2147+
})
2148+
await arborist.reify({ installStrategy: 'linked' })
2149+
2150+
const storeDir = path.join(dir, 'node_modules', '.store')
2151+
const storeEntries = fs.readdirSync(storeDir)
2152+
2153+
t.ok(storeEntries.some(e => e.startsWith('which@')), 'prod dep which is in store')
2154+
t.ok(storeEntries.some(e => e.startsWith('isexe@')), 'prod dep isexe is in store')
2155+
t.notOk(storeEntries.some(e => e.startsWith('eslint@')), 'dev dep eslint is not in store')
2156+
})
2157+
20582158
/*
20592159
* TO TEST:
20602160
* --------------------------------------

0 commit comments

Comments
 (0)