Skip to content

Commit 68a8d05

Browse files
zkatowlstronaut
authored andcommitted
fix(arborist): omit failed optional dependencies from installed deps, but keep them 'in the tree'
Fixes: #4828 Fixes: #7961 Replaces: #8127 Replaces: #8077 When optional dependencies fail, we don't want to install them, for whatever reason. The way this "uninstallation" is done right now is by simply removing the dependencies from the ideal tree during reification. Unfortunately, this means that what gets saved to the "hidden lockfile" is the edited ideal tree as well, and when NPM runs next, it'll use that when regenerating the "real" package-lock. This PR tags failed optional deps such that they're still added to the "trash list" (and thus have their directories cleaned up by the reifier), but prevents Arborist from removing them altogether from the ideal tree (which is usually done by setting their parent to `null`, making them unreachable, and letting them get GC'd).
1 parent 04f53ce commit 68a8d05

20 files changed

+1939
-39
lines changed

workspaces/arborist/lib/arborist/build-ideal-tree.js

+4-2
Original file line numberDiff line numberDiff line change
@@ -809,7 +809,8 @@ This is a one-time fix-up, please be patient...
809809
const crackOpen = this.#complete &&
810810
node !== this.idealTree &&
811811
node.resolved &&
812-
(hasBundle || hasShrinkwrap)
812+
(hasBundle || hasShrinkwrap) &&
813+
!node.ideallyInert
813814
if (crackOpen) {
814815
const Arborist = this.constructor
815816
const opt = { ...this.options }
@@ -1527,7 +1528,7 @@ This is a one-time fix-up, please be patient...
15271528

15281529
const set = optionalSet(node)
15291530
for (const node of set) {
1530-
node.parent = null
1531+
node.ideallyInert = true
15311532
}
15321533
}
15331534
}
@@ -1548,6 +1549,7 @@ This is a one-time fix-up, please be patient...
15481549
node.parent !== null
15491550
&& !node.isProjectRoot
15501551
&& !excludeNodes.has(node)
1552+
&& !node.ideallyInert
15511553
) {
15521554
this[_addNodeToTrashList](node)
15531555
}

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ module.exports = cls => class IsolatedReifier extends cls {
8181
}
8282
queue.push(e.to)
8383
})
84-
if (!next.isProjectRoot && !next.isWorkspace) {
84+
if (!next.isProjectRoot && !next.isWorkspace && !next.ideallyInert) {
8585
root.external.push(await this.externalProxyMemo(next))
8686
}
8787
}
@@ -147,8 +147,8 @@ module.exports = cls => class IsolatedReifier extends cls {
147147
const nonOptionalDeps = edges.filter(e => !e.optional).map(e => e.to.target)
148148

149149
result.localDependencies = await Promise.all(nonOptionalDeps.filter(n => n.isWorkspace).map(this.workspaceProxyMemo))
150-
result.externalDependencies = await Promise.all(nonOptionalDeps.filter(n => !n.isWorkspace).map(this.externalProxyMemo))
151-
result.externalOptionalDependencies = await Promise.all(optionalDeps.map(this.externalProxyMemo))
150+
result.externalDependencies = await Promise.all(nonOptionalDeps.filter(n => !n.isWorkspace && !n.ideallyInert).map(this.externalProxyMemo))
151+
result.externalOptionalDependencies = await Promise.all(optionalDeps.filter(n => !n.ideallyInert).map(this.externalProxyMemo))
152152
result.dependencies = [
153153
...result.externalDependencies,
154154
...result.localDependencies,

workspaces/arborist/lib/arborist/load-virtual.js

+1
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,7 @@ module.exports = cls => class VirtualLoader extends cls {
267267
integrity: sw.integrity,
268268
resolved: consistentResolve(sw.resolved, this.path, path),
269269
pkg: sw,
270+
ideallyInert: sw.ideallyInert,
270271
hasShrinkwrap: sw.hasShrinkwrap,
271272
dev,
272273
optional,

workspaces/arborist/lib/arborist/reify.js

+25-2
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ module.exports = cls => class Reifier extends cls {
149149
for (const path of this[_trashList]) {
150150
const loc = relpath(this.idealTree.realpath, path)
151151
const node = this.idealTree.inventory.get(loc)
152-
if (node && node.root === this.idealTree) {
152+
if (node && node.root === this.idealTree && !node.ideallyInert) {
153153
node.parent = null
154154
}
155155
}
@@ -237,6 +237,18 @@ module.exports = cls => class Reifier extends cls {
237237
this.idealTree.meta.hiddenLockfile = true
238238
this.idealTree.meta.lockfileVersion = defaultLockfileVersion
239239

240+
// Preserve inertness for failed stuff.
241+
if (this.actualTree) {
242+
for (const [loc, actual] of this.actualTree.inventory.entries()) {
243+
if (actual.ideallyInert) {
244+
const ideal = this.idealTree.inventory.get(loc)
245+
if (ideal) {
246+
ideal.ideallyInert = true
247+
}
248+
}
249+
}
250+
}
251+
240252
this.actualTree = this.idealTree
241253
this.idealTree = null
242254

@@ -599,6 +611,9 @@ module.exports = cls => class Reifier extends cls {
599611
// retire the same path at the same time.
600612
const dirsChecked = new Set()
601613
return promiseAllRejectLate(leaves.map(async node => {
614+
if (node.ideallyInert) {
615+
return
616+
}
602617
for (const d of walkUp(node.path)) {
603618
if (d === node.top.path) {
604619
break
@@ -743,6 +758,10 @@ module.exports = cls => class Reifier extends cls {
743758
}
744759

745760
async #extractOrLink (node) {
761+
if (node.ideallyInert) {
762+
return
763+
}
764+
746765
const nm = resolve(node.parent.path, 'node_modules')
747766
await this.#validateNodeModules(nm)
748767

@@ -818,6 +837,7 @@ module.exports = cls => class Reifier extends cls {
818837
const set = optionalSet(node)
819838
for (node of set) {
820839
log.verbose('reify', 'failed optional dependency', node.path)
840+
node.ideallyInert = true
821841
this[_addNodeToTrashList](node)
822842
}
823843
}) : p).then(() => node)
@@ -1152,6 +1172,9 @@ module.exports = cls => class Reifier extends cls {
11521172

11531173
this.#retiredUnchanged[retireFolder] = []
11541174
return promiseAllRejectLate(diff.unchanged.map(node => {
1175+
if (node.ideallyInert) {
1176+
return
1177+
}
11551178
// no need to roll back links, since we'll just delete them anyway
11561179
if (node.isLink) {
11571180
return mkdir(dirname(node.path), { recursive: true, force: true })
@@ -1231,7 +1254,7 @@ module.exports = cls => class Reifier extends cls {
12311254
// skip links that only live within node_modules as they are most
12321255
// likely managed by packages we installed, we only want to rebuild
12331256
// unchanged links we directly manage
1234-
const linkedFromRoot = node.parent === tree || node.target.fsTop === tree
1257+
const linkedFromRoot = (node.parent === tree && !node.ideallyInert) || node.target.fsTop === tree
12351258
if (node.isLink && linkedFromRoot) {
12361259
nodes.push(node)
12371260
}

workspaces/arborist/lib/node.js

+3
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ class Node {
103103
global = false,
104104
dummy = false,
105105
sourceReference = null,
106+
ideallyInert = false,
106107
} = options
107108
// this object gives querySelectorAll somewhere to stash context about a node
108109
// while processing a query
@@ -197,6 +198,8 @@ class Node {
197198
this.extraneous = false
198199
}
199200

201+
this.ideallyInert = ideallyInert
202+
200203
this.edgesIn = new Set()
201204
this.edgesOut = new CaseInsensitiveMap()
202205

workspaces/arborist/lib/shrinkwrap.js

+16-1
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ const nodeMetaKeys = [
109109
'inBundle',
110110
'hasShrinkwrap',
111111
'hasInstallScript',
112+
'ideallyInert',
112113
]
113114

114115
const metaFieldFromPkg = (pkg, key) => {
@@ -135,6 +136,10 @@ const assertNoNewer = async (path, data, lockTime, dir, seen) => {
135136

136137
const parent = isParent ? dir : resolve(dir, 'node_modules')
137138
const rel = relpath(path, dir)
139+
const inert = data.packages[rel]?.ideallyInert
140+
if (inert) {
141+
return
142+
}
138143
seen.add(rel)
139144
let entries
140145
if (dir === path) {
@@ -173,7 +178,7 @@ const assertNoNewer = async (path, data, lockTime, dir, seen) => {
173178

174179
// assert that all the entries in the lockfile were seen
175180
for (const loc in data.packages) {
176-
if (!seen.has(loc)) {
181+
if (!seen.has(loc) && !data.packages[loc].ideallyInert) {
177182
throw new Error(`missing from node_modules: ${loc}`)
178183
}
179184
}
@@ -783,6 +788,10 @@ class Shrinkwrap {
783788
// ok, I did my best! good luck!
784789
}
785790

791+
if (lock.ideallyInert) {
792+
meta.ideallyInert = true
793+
}
794+
786795
if (lock.bundled) {
787796
meta.inBundle = true
788797
}
@@ -953,6 +962,12 @@ class Shrinkwrap {
953962
this.#buildLegacyLockfile(this.tree, this.data)
954963
}
955964

965+
if (!this.hiddenLockfile) {
966+
for (const node of Object.values(this.data.packages)) {
967+
delete node.ideallyInert
968+
}
969+
}
970+
956971
// lf version 1 = dependencies only
957972
// lf version 2 = dependencies and packages
958973
// lf version 3 = packages only

workspaces/arborist/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs

+144-4
Original file line numberDiff line numberDiff line change
@@ -77868,11 +77868,34 @@ ArboristNode {
7786877868

7786977869
exports[`test/arborist/build-ideal-tree.js TAP optional dependency failures > optional-dep-enotarget 1`] = `
7787077870
ArboristNode {
77871+
"children": Map {
77872+
"tap" => ArboristNode {
77873+
"edgesIn": Set {
77874+
EdgeIn {
77875+
"error": "INVALID",
77876+
"from": "",
77877+
"name": "tap",
77878+
"spec": "9999.0000.9999",
77879+
"type": "optional",
77880+
},
77881+
},
77882+
"errors": Array [
77883+
Object {
77884+
"code": "ETARGET",
77885+
},
77886+
],
77887+
"location": "node_modules/tap",
77888+
"name": "tap",
77889+
"optional": true,
77890+
"path": "{CWD}/test/fixtures/optional-dep-enotarget/node_modules/tap",
77891+
},
77892+
},
7787177893
"edgesOut": Map {
7787277894
"tap" => EdgeOut {
77895+
"error": "INVALID",
7787377896
"name": "tap",
7787477897
"spec": "9999.0000.9999",
77875-
"to": null,
77898+
"to": "node_modules/tap",
7787677899
"type": "optional",
7787777900
},
7787877901
},
@@ -77887,11 +77910,32 @@ ArboristNode {
7788777910

7788877911
exports[`test/arborist/build-ideal-tree.js TAP optional dependency failures > optional-dep-missing 1`] = `
7788977912
ArboristNode {
77913+
"children": Map {
77914+
"@isaacs/this-does-not-exist-at-all" => ArboristNode {
77915+
"edgesIn": Set {
77916+
EdgeIn {
77917+
"from": "",
77918+
"name": "@isaacs/this-does-not-exist-at-all",
77919+
"spec": "*",
77920+
"type": "optional",
77921+
},
77922+
},
77923+
"errors": Array [
77924+
Object {
77925+
"code": "E404",
77926+
},
77927+
],
77928+
"location": "node_modules/@isaacs/this-does-not-exist-at-all",
77929+
"name": "@isaacs/this-does-not-exist-at-all",
77930+
"optional": true,
77931+
"path": "{CWD}/test/fixtures/optional-dep-missing/node_modules/@isaacs/this-does-not-exist-at-all",
77932+
},
77933+
},
7789077934
"edgesOut": Map {
7789177935
"@isaacs/this-does-not-exist-at-all" => EdgeOut {
7789277936
"name": "@isaacs/this-does-not-exist-at-all",
7789377937
"spec": "*",
77894-
"to": null,
77938+
"to": "node_modules/@isaacs/this-does-not-exist-at-all",
7789577939
"type": "optional",
7789677940
},
7789777941
},
@@ -77906,11 +77950,60 @@ ArboristNode {
7790677950

7790777951
exports[`test/arborist/build-ideal-tree.js TAP optional dependency failures > optional-metadep-enotarget 1`] = `
7790877952
ArboristNode {
77953+
"children": Map {
77954+
"@isaacs/prod-dep-enotarget" => ArboristNode {
77955+
"children": Map {
77956+
"tap" => ArboristNode {
77957+
"edgesIn": Set {
77958+
EdgeIn {
77959+
"error": "INVALID",
77960+
"from": "node_modules/@isaacs/prod-dep-enotarget",
77961+
"name": "tap",
77962+
"spec": "9999.0000.9999",
77963+
"type": "prod",
77964+
},
77965+
},
77966+
"errors": Array [
77967+
Object {
77968+
"code": "ETARGET",
77969+
},
77970+
],
77971+
"location": "node_modules/@isaacs/prod-dep-enotarget/node_modules/tap",
77972+
"name": "tap",
77973+
"optional": true,
77974+
"path": "{CWD}/test/fixtures/optional-metadep-enotarget/node_modules/@isaacs/prod-dep-enotarget/node_modules/tap",
77975+
},
77976+
},
77977+
"edgesIn": Set {
77978+
EdgeIn {
77979+
"from": "",
77980+
"name": "@isaacs/prod-dep-enotarget",
77981+
"spec": "*",
77982+
"type": "optional",
77983+
},
77984+
},
77985+
"edgesOut": Map {
77986+
"tap" => EdgeOut {
77987+
"error": "INVALID",
77988+
"name": "tap",
77989+
"spec": "9999.0000.9999",
77990+
"to": "node_modules/@isaacs/prod-dep-enotarget/node_modules/tap",
77991+
"type": "prod",
77992+
},
77993+
},
77994+
"location": "node_modules/@isaacs/prod-dep-enotarget",
77995+
"name": "@isaacs/prod-dep-enotarget",
77996+
"optional": true,
77997+
"path": "{CWD}/test/fixtures/optional-metadep-enotarget/node_modules/@isaacs/prod-dep-enotarget",
77998+
"resolved": "https://registry.npmjs.org/@isaacs/prod-dep-enotarget/-/prod-dep-enotarget-1.0.0.tgz",
77999+
"version": "1.0.0",
78000+
},
78001+
},
7790978002
"edgesOut": Map {
7791078003
"@isaacs/prod-dep-enotarget" => EdgeOut {
7791178004
"name": "@isaacs/prod-dep-enotarget",
7791278005
"spec": "*",
77913-
"to": null,
78006+
"to": "node_modules/@isaacs/prod-dep-enotarget",
7791478007
"type": "optional",
7791578008
},
7791678009
},
@@ -77924,11 +78017,58 @@ ArboristNode {
7792478017

7792578018
exports[`test/arborist/build-ideal-tree.js TAP optional dependency failures > optional-metadep-missing 1`] = `
7792678019
ArboristNode {
78020+
"children": Map {
78021+
"@isaacs/testing-prod-dep-metadata-missing" => ArboristNode {
78022+
"children": Map {
78023+
"@isaacs/this-does-not-exist-at-all" => ArboristNode {
78024+
"edgesIn": Set {
78025+
EdgeIn {
78026+
"from": "node_modules/@isaacs/testing-prod-dep-metadata-missing",
78027+
"name": "@isaacs/this-does-not-exist-at-all",
78028+
"spec": "*",
78029+
"type": "prod",
78030+
},
78031+
},
78032+
"errors": Array [
78033+
Object {
78034+
"code": "E404",
78035+
},
78036+
],
78037+
"location": "node_modules/@isaacs/testing-prod-dep-metadata-missing/node_modules/@isaacs/this-does-not-exist-at-all",
78038+
"name": "@isaacs/this-does-not-exist-at-all",
78039+
"optional": true,
78040+
"path": "{CWD}/test/fixtures/optional-metadep-missing/node_modules/@isaacs/testing-prod-dep-metadata-missing/node_modules/@isaacs/this-does-not-exist-at-all",
78041+
},
78042+
},
78043+
"edgesIn": Set {
78044+
EdgeIn {
78045+
"from": "",
78046+
"name": "@isaacs/testing-prod-dep-metadata-missing",
78047+
"spec": "*",
78048+
"type": "optional",
78049+
},
78050+
},
78051+
"edgesOut": Map {
78052+
"@isaacs/this-does-not-exist-at-all" => EdgeOut {
78053+
"name": "@isaacs/this-does-not-exist-at-all",
78054+
"spec": "*",
78055+
"to": "node_modules/@isaacs/testing-prod-dep-metadata-missing/node_modules/@isaacs/this-does-not-exist-at-all",
78056+
"type": "prod",
78057+
},
78058+
},
78059+
"location": "node_modules/@isaacs/testing-prod-dep-metadata-missing",
78060+
"name": "@isaacs/testing-prod-dep-metadata-missing",
78061+
"optional": true,
78062+
"path": "{CWD}/test/fixtures/optional-metadep-missing/node_modules/@isaacs/testing-prod-dep-metadata-missing",
78063+
"resolved": "https://registry.npmjs.org/@isaacs/testing-prod-dep-metadata-missing/-/testing-prod-dep-metadata-missing-1.0.0.tgz",
78064+
"version": "1.0.0",
78065+
},
78066+
},
7792778067
"edgesOut": Map {
7792878068
"@isaacs/testing-prod-dep-metadata-missing" => EdgeOut {
7792978069
"name": "@isaacs/testing-prod-dep-metadata-missing",
7793078070
"spec": "*",
77931-
"to": null,
78071+
"to": "node_modules/@isaacs/testing-prod-dep-metadata-missing",
7793278072
"type": "optional",
7793378073
},
7793478074
},

workspaces/arborist/tap-snapshots/test/arborist/load-actual.js.test.cjs

+1-1
Original file line numberDiff line numberDiff line change
@@ -7784,4 +7784,4 @@ ArboristNode {
77847784
"name": "yarn-lock-mkdirp-file-dep",
77857785
"path": "yarn-lock-mkdirp-file-dep",
77867786
}
7787-
`
7787+
`

0 commit comments

Comments
 (0)