Content-Length: 1117875 | pFad | http://github.com/npm/cli/commit/a96d8f6295886c219076178460718837d2fe45d6

65 fix(arborist): omit failed optional dependencies from installed deps … · npm/cli@a96d8f6 · GitHub
Skip to content

Commit a96d8f6

Browse files
owlstronautzkat
owlstronaut
and
zkat
authored
fix(arborist): omit failed optional dependencies from installed deps (#8184)
… but keep them 'in the tree' This PR was authored by @zkat 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). PS: It's Friday, this is what I managed to get done together. I'm making this a draft PR for now so folks can look at it, but I want to make sure both reifiers work well, fix some messaging issues, and clean stuff up (I'm pretty sure I'm guarding `ideallyInert` in more places than I need to, because I was trying to find the right spot). Still, feel free to talk about the approach. I'll get back to this on Monday. PPS: also hi Co-authored-by: Kat Marchán <kzm@zkat.tech>
1 parent 04f53ce commit a96d8f6

File tree

20 files changed

+1939
-39
lines changed

20 files changed

+1939
-39
lines changed

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

Lines changed: 4 additions & 2 deletions
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

Lines changed: 3 additions & 3 deletions
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

Lines changed: 1 addition & 0 deletions
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

Lines changed: 25 additions & 2 deletions
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

Lines changed: 3 additions & 0 deletions
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

Lines changed: 16 additions & 1 deletion
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

Lines changed: 144 additions & 4 deletions
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

Lines changed: 1 addition & 1 deletion
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)








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/npm/cli/commit/a96d8f6295886c219076178460718837d2fe45d6

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy