From e65ce49815acf64af89801fbd3fb820a736f3020 Mon Sep 17 00:00:00 2001 From: Michael Garvin Date: Thu, 5 Mar 2026 10:44:25 -0800 Subject: [PATCH 1/3] fix: consolidate isolated node/link attributes We are making fake versions of nodes and links in isolated mode. With this we are at least being consistent w/ the default attributes. For example `isStoreLink` was not on workspace links, but it is now. --- .../arborist/lib/arborist/isolated-reifier.js | 176 +++++------------- workspaces/arborist/lib/arborist/reify.js | 75 ++------ workspaces/arborist/lib/inventory.js | 3 +- workspaces/arborist/lib/isolated-classes.js | 138 ++++++++++++++ workspaces/arborist/lib/node.js | 39 ++-- 5 files changed, 226 insertions(+), 205 deletions(-) create mode 100644 workspaces/arborist/lib/isolated-classes.js diff --git a/workspaces/arborist/lib/arborist/isolated-reifier.js b/workspaces/arborist/lib/arborist/isolated-reifier.js index 2b70a831f0205..79d1e24e4fbc6 100644 --- a/workspaces/arborist/lib/arborist/isolated-reifier.js +++ b/workspaces/arborist/lib/arborist/isolated-reifier.js @@ -3,6 +3,7 @@ const pacote = require('pacote') const { join } = require('node:path') const { depth } = require('treeverse') const crypto = require('node:crypto') +const { IsolatedNode, IsolatedLink } = require('../isolated-classes.js') // generate short hash key based on the dependency tree starting at this node const getKey = (startNode) => { @@ -36,40 +37,20 @@ module.exports = cls => class IsolatedReifier extends cls { #processedEdges = new Set() #workspaceProxies = new Map() - #generateChild (node, location, pkg, inStore, root) { - const newChild = { - binPaths: [], - children: new Map(), - edgesIn: new Set(), - edgesOut: new Map(), - fsChildren: new Set(), - /* istanbul ignore next -- emulate Node */ - getBundler () { - return null - }, - global: false, - globalTop: false, - hasShrinkwrap: false, - inDepBundle: false, - integrity: null, - isInStore: inStore, - isLink: false, - isProjectRoot: false, - isRoot: false, - isTop: false, + #generateChild (node, location, pkg, isInStore, root) { + const newChild = new IsolatedNode({ + isInStore, location, name: node.packageName || node.name, optional: node.optional, package: pkg, parent: root, - path: join(this.idealGraph.root.localPath, location), - realpath: join(this.idealGraph.root.localPath, location), + path: join(this.idealGraph.localPath, location), resolved: node.resolved, - root, - top: { path: this.idealGraph.root.localPath }, - version: pkg.version, - } - newChild.target = newChild + root: location, + }) + // XXX top is from place-dep not lib/node.js + newChild.top = { path: this.idealGraph.localPath } root.children.set(newChild.location, newChild) root.inventory.set(newChild.location, newChild) } @@ -92,15 +73,17 @@ module.exports = cls => class IsolatedReifier extends cls { const idealTree = this.idealTree const omit = new Set(this.options.omit) + // XXX this sometimes acts like a node too this.idealGraph = { external: [], isProjectRoot: true, localLocation: idealTree.location, localPath: idealTree.path, + path: idealTree.path, } this.counter = 0 - this.idealGraph.workspaces = await Promise.all(Array.from(idealTree.fsChildren.values(), w => this.workspaceProxy(w))) + this.idealGraph.workspaces = await Promise.all(Array.from(idealTree.fsChildren.values(), w => this.#workspaceProxy(w))) const processed = new Set() const queue = [idealTree, ...idealTree.fsChildren] while (queue.length !== 0) { @@ -110,45 +93,43 @@ module.exports = cls => class IsolatedReifier extends cls { } processed.add(next.location) next.edgesOut.forEach(edge => { - if (edge.to && !(next.package.bundleDependencies || next.package.bundledDependencies || []).includes(edge.to.name)) { - if (!edge.to.shouldOmit?.(omit)) { - queue.push(edge.to) - } + if (edge.to && !(next.package.bundleDependencies || next.package.bundledDependencies || []).includes(edge.to.name) && !edge.to.shouldOmit?.(omit)) { + queue.push(edge.to) } }) // local `file:` deps are in fsChildren but are not workspaces. // they are already handled as workspace-like proxies above and should not go through the external/store extraction path. if (!next.isProjectRoot && !next.isWorkspace && !next.inert && !idealTree.fsChildren.has(next) && !idealTree.fsChildren.has(next.target)) { - this.idealGraph.external.push(await this.externalProxy(next)) + this.idealGraph.external.push(await this.#externalProxy(next)) } } - await this.assignCommonProperties(idealTree, this.idealGraph) + await this.#assignCommonProperties(idealTree, this.idealGraph) } - async workspaceProxy (node) { + async #workspaceProxy (node) { if (this.#workspaceProxies.has(node)) { return this.#workspaceProxies.get(node) } const result = {} - // XXX this goes recursive if we don't set here because assignCommonProperties also calls this.workspaceProxy + // XXX this goes recursive if we don't set here because assignCommonProperties also calls this.#workspaceProxy this.#workspaceProxies.set(node, result) result.localLocation = node.location result.localPath = node.path result.isWorkspace = true result.resolved = node.resolved - await this.assignCommonProperties(node, result) + await this.#assignCommonProperties(node, result) return result } - async externalProxy (node) { + async #externalProxy (node) { if (this.#externalProxies.has(node)) { return this.#externalProxies.get(node) } const result = {} - // XXX this goes recursive if we don't set here because assignCommonProperties also calls this.externalProxy + // XXX this goes recursive if we don't set here because assignCommonProperties also calls this.#externalProxy this.#externalProxies.set(node, result) - await this.assignCommonProperties(node, result, !node.hasShrinkwrap) + await this.#assignCommonProperties(node, result, !node.hasShrinkwrap) if (node.hasShrinkwrap) { const dir = join( node.root.path, @@ -190,15 +171,15 @@ module.exports = cls => class IsolatedReifier extends cls { return result } - async assignCommonProperties (node, result, populateDeps = true) { + async #assignCommonProperties (node, result, populateDeps = true) { result.root = this.idealGraph + // XXX does anything need this? result.id = this.counter++ /* istanbul ignore next - packageName is always set for real packages */ result.name = result.isWorkspace ? (node.packageName || node.name) : node.name result.packageName = node.packageName || node.name result.package = { ...node.package } result.package.bundleDependencies = undefined - result.hasInstallScript = node.hasInstallScript if (!populateDeps) { return @@ -228,9 +209,9 @@ module.exports = cls => class IsolatedReifier extends cls { // local `file:` deps (non-workspace fsChildren) should be treated as local dependencies, not external, so they get symlinked directly instead of being extracted into the store. const isLocal = (n) => n.isWorkspace || node.fsChildren?.has(n) const optionalDeps = edges.filter(edge => edge.optional).map(edge => edge.to.target) - result.localDependencies = await Promise.all(nonOptionalDeps.filter(isLocal).map(n => this.workspaceProxy(n))) - result.externalDependencies = await Promise.all(nonOptionalDeps.filter(n => !isLocal(n) && !n.inert).map(n => this.externalProxy(n))) - result.externalOptionalDependencies = await Promise.all(optionalDeps.filter(n => !n.inert).map(n => this.externalProxy(n))) + result.localDependencies = await Promise.all(nonOptionalDeps.filter(isLocal).map(n => this.#workspaceProxy(n))) + result.externalDependencies = await Promise.all(nonOptionalDeps.filter(n => !isLocal(n) && !n.inert).map(n => this.#externalProxy(n))) + result.externalOptionalDependencies = await Promise.all(optionalDeps.filter(n => !n.inert).map(n => this.#externalProxy(n))) result.dependencies = [ ...result.externalDependencies, ...result.localDependencies, @@ -290,79 +271,27 @@ module.exports = cls => class IsolatedReifier extends cls { async createIsolatedTree () { await this.makeIdealGraph() - const bundledTree = await this.#createBundledTree() - const root = { - binPaths: [], - children: new Map(), - edgesIn: new Set(), - edgesOut: new Map(), - fsChildren: new Set(), - global: false, - hasShrinkwrap: false, - integrity: null, - inventory: new Map(), - isLink: false, - isProjectRoot: true, - isRoot: true, - isTop: true, - linksIn: new Set(), - meta: { loadedFromDisk: false }, - package: this.idealGraph.root.package, - parent: null, - path: this.idealGraph.root.localPath, - realpath: this.idealGraph.root.localPath, - // TODO: we should probably not reference this.idealTree - resolved: this.idealTree.resolved, - tops: new Set(), - workspaces: new Map(), - } - root.inventory.set('', root) + const root = new IsolatedNode(this.idealGraph) root.root = root - root.target = root - // TODO inventory.query is a stub; audit-report needs 'packageName' support - root.inventory.query = () => { - return [] - } + root.inventory.set('', root) const processed = new Set() for (const c of this.idealGraph.workspaces) { const wsName = c.packageName - const workspace = { - binPaths: [], - children: new Map(), - edgesIn: new Set(), - edgesOut: new Map(), - fsChildren: new Set(), - hasInstallScript: c.hasInstallScript, - isLink: false, - isRoot: false, - linksIn: new Set(), + // XXX parent? root? + const workspace = new IsolatedNode({ location: c.localLocation, name: wsName, package: c.package, path: c.localPath, - realpath: c.localPath, resolved: c.resolved, - } - workspace.target = workspace + }) root.fsChildren.add(workspace) root.inventory.set(workspace.location, workspace) // Create workspace Link entry in children for _diffTrees lookup - const wsLink = { - binPaths: [], - children: new Map(), - edgesIn: new Set(), - edgesOut: new Map(), - fsChildren: new Set(), - global: false, - globalTop: false, - isLink: true, - isProjectRoot: false, - isRoot: false, - isTop: false, - linksIn: new Set(), + const wsLink = new IsolatedLink({ location: join('node_modules', wsName), name: wsName, package: workspace.package, @@ -371,7 +300,7 @@ module.exports = cls => class IsolatedReifier extends cls { realpath: workspace.path, root, target: workspace, - } + }) root.children.set(wsLink.name, wsLink) root.inventory.set(wsLink.location, wsLink) root.workspaces.set(wsName, workspace.path) @@ -468,35 +397,28 @@ module.exports = cls => class IsolatedReifier extends cls { } } - const link = { - binPaths: [], - children: new Map(), - edgesIn: new Set(), - edgesOut: new Map(), - fsChildren: new Set(), - global: false, - globalTop: false, - isLink: true, - isProjectRoot: false, - isRoot: false, - isStoreLink: true, - isTop: false, + const pkg = { + _id: dep.package._id, + bin: target.package.bin, + bundleDependencies: undefined, + deprecated: undefined, + scripts: dep.package.scripts, + version: dep.package.version, + optional, + } + const link = new IsolatedLink({ location: join(nmFolder, dep.name), name: toKey, - optional, - // TODO _id: 'abc' ? - package: { _id: 'abc', bundleDependencies: undefined, deprecated: undefined, bin: target.package.bin, scripts: dep.package.scripts }, parent: root, + package: pkg, path: join(dep.root.localPath, nmFolder, dep.name), realpath: target.path, - resolved: external - ? `file:.store/${toKey}/node_modules/${dep.packageName}` - : dep.resolved, + resolved: external ? `file:.store/${toKey}/node_modules/${dep.packageName}` : dep.resolved, root, target, - version: dep.version, - top: { path: dep.root.localPath }, - } + }) + // XXX top is from place-dep not lib/link.js + link.top = { path: dep.root.localPath } const newEdge1 = { optional, from, to: link } from.edgesOut.set(dep.name, newEdge1) link.edgesIn.add(newEdge1) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 9b380b38faa08..44f879b5aea22 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -27,6 +27,7 @@ const retirePath = require('../retire-path.js') const treeCheck = require('../tree-check.js') const { defaultLockfileVersion } = require('../shrinkwrap.js') const { saveTypeMap, hasSubKey } = require('../add-rm-pkg-deps.js') +const { IsolatedNode, IsolatedLink } = require('../isolated-classes.js') // Part of steps (steps need refactoring before we can do anything about these) const _retireShallowNodes = Symbol.for('retireShallowNodes') @@ -818,37 +819,12 @@ module.exports = cls => class Reifier extends cls { for (const child of idealTree.children.values()) { if (!combined.has(child.path) && (child.isInStore || child.isStoreLink) && existsSync(child.path)) { - const entry = { - global: false, - globalTop: false, - isProjectRoot: false, - isTop: false, - location: child.location, - name: child.name, - optional: child.optional, - top: child.top, - children: [], - edgesIn: new Set(), - edgesOut: new Map(), - binPaths: [], - fsChildren: [], - /* istanbul ignore next -- emulate Node */ - getBundler () { - return null - }, - hasShrinkwrap: false, - inDepBundle: false, - integrity: null, - isLink: Boolean(child.isLink), - isRoot: false, - isInStore: Boolean(child.isInStore), - path: child.path, - realpath: child.realpath, - resolved: child.resolved, - version: child.version, - package: child.package, + let entry + if (child.isLink) { + entry = new IsolatedLink(child) + } else { + entry = new IsolatedNode(child) } - entry.target = entry if (child.isLink && combined.has(child.realpath)) { entry.target = combined.get(child.realpath) } @@ -863,33 +839,20 @@ module.exports = cls => class Reifier extends cls { /* istanbul ignore next -- only reached during scoped workspace installs */ combined.get = (key) => combinedGet(key) || origGet(key) - const wrapper = { - isRoot: true, - isLink: actualTree.isLink, - target: actualTree.target, - fsChildren: actualTree.fsChildren, - path: actualTree.path, - realpath: actualTree.realpath, - edgesOut: actualTree.edgesOut, - inventory: actualTree.inventory, - package: actualTree.package, - resolved: actualTree.resolved, - version: actualTree.version, - integrity: actualTree.integrity, - binPaths: actualTree.binPaths, - hasShrinkwrap: false, - inDepBundle: false, - parent: null, - children: combined, - } - - // Set parent/root on synthetic entries for consistency - for (const child of combined.values()) { - if (!child.parent) { - child.parent = wrapper - child.root = wrapper - } + let wrapper + /* istanbul ignore next - untested! */ + if (actualTree.isLink) { + wrapper = new IsolatedLink(actualTree) + } else { + wrapper = new IsolatedNode(actualTree) } + wrapper.root = wrapper + wrapper.binPaths = actualTree.binPaths + wrapper.children = combined + wrapper.edgesOut = actualTree.edgesOut + wrapper.fsChildren = actualTree.fsChildren + wrapper.integrity = actualTree.integrity + wrapper.inventory = actualTree.inventory return wrapper } diff --git a/workspaces/arborist/lib/inventory.js b/workspaces/arborist/lib/inventory.js index 7b3f294fdab2c..5e4a67eeed103 100644 --- a/workspaces/arborist/lib/inventory.js +++ b/workspaces/arborist/lib/inventory.js @@ -1,5 +1,4 @@ -// a class to manage an inventory and set of indexes of a set of objects based -// on specific fields. +// a class to manage an inventory and set of indexes of a set of objects based on specific fields. const { hasOwnProperty } = Object.prototype const debug = require('./debug.js') diff --git a/workspaces/arborist/lib/isolated-classes.js b/workspaces/arborist/lib/isolated-classes.js new file mode 100644 index 0000000000000..b432e91ab0b0b --- /dev/null +++ b/workspaces/arborist/lib/isolated-classes.js @@ -0,0 +1,138 @@ +// Alternate versions of different classes that we use for isolated mode +const CaseInsensitiveMap = require('./case-insensitive-map.js') +const { resolve } = require('node:path') + +// fake lib/inventory.js +class IsolatedInventory extends Map { + query () { + return [] + } +} + +// fake lib/node.js +class IsolatedNode { + binPaths = [] + children = new CaseInsensitiveMap() + edgesIn = new Set() + edgesOut = new CaseInsensitiveMap() + fsChildren = new Set() + hasShrinkwrap = false + integrity = null + inventory = new IsolatedInventory() + isInStore = false + linksIn = new Set() + meta = { loadedFromDisk: false } + optional = false + parent = null + root = null + tops = new Set() + workspaces = new Map() + + constructor (options) { + this.location = options.location + this.name = options.name + this.package = options.package + this.path = options.path + this.realpath = !this.isLink ? this.path : resolve(options.realpath) + + if (options.parent) { + this.parent = options.parent + } + if (options.resolved) { + this.resolved = options.resolved + } + if (options.root) { + this.root = options.root + } + if (options.isInStore) { + this.isInStore = true + } + if (options.optional) { + this.optional = true + } + } + + get isRoot () { + return this === this.root + } + + // The idealGraph is where this is set to true + get isProjectRoot () { + return false + } + + get inDepBundle () { + return false + } + + get isLink () { + return false + } + + get isTop () { + return !this.parent + } + + /* istanbul ignore next -- emulate lib/node.js */ + get global () { + return false + } + + get globalTop () { + return false + } + + /* istanbul ignore next -- emulate lib/node.js */ + set target (t) { + // nop + // In the real lib/node.js this throws in debug mode + } + + get target () { + return this + } + + /* istanbul ignore next -- emulate lib/node.js */ + getBundler () { + return null + } + + /* istanbul ignore next -- emulate lib/node.js */ + get hasInstallScript () { + const { hasInstallScript, scripts } = this.package + const { install, preinstall, postinstall } = scripts || {} + return !!(hasInstallScript || install || preinstall || postinstall) + } + + get version () { + return this.package.version + } +} + +// fake lib/link.js +class IsolatedLink extends IsolatedNode { + #target = this + + constructor (options) { + super(options) + this.#target = options.target + } + + get isStoreLink () { + return true + } + + get isLink () { + return true + } + + set target (t) { + this.#target = t + } + + get target () { + return this.#target + } +} + +module.exports = { IsolatedNode, IsolatedLink } diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index c0891df4af1e4..451ede1e52cbf 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -73,35 +73,34 @@ class Node { constructor (options) { // NB: path can be null if it's a link target const { - root, - path, - realpath, - parent, - error, - meta, - fsParent, - resolved, - integrity, - // allow setting name explicitly when we haven't set a path yet - name, children, + dev = true, + devOptional = true, + dummy = false, + error, + extraneous = true, fsChildren, + fsParent, + global = false, + hasShrinkwrap, + inert = false, installLinks = false, + integrity, + isInStore = false, legacyPeerDeps = false, linksIn, - isInStore = false, - hasShrinkwrap, - overrides, loadOverrides = false, - extraneous = true, - dev = true, + meta, + name, // allow setting name explicitly when we haven't set a path yet optional = true, - devOptional = true, + overrides, + parent, + path, peer = true, - global = false, - dummy = false, + realpath, + resolved, + root, sourceReference = null, - inert = false, } = options // this object gives querySelectorAll somewhere to stash context about a node // while processing a query From e2f5dfdbd0984cc70a3ad7023cb24898efeb1e1f Mon Sep 17 00:00:00 2001 From: Michael Garvin Date: Fri, 6 Mar 2026 08:52:18 -0800 Subject: [PATCH 2/3] fixup: pr review corrections --- workspaces/arborist/lib/arborist/isolated-reifier.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/arborist/isolated-reifier.js b/workspaces/arborist/lib/arborist/isolated-reifier.js index 79d1e24e4fbc6..24ca9c9c450b6 100644 --- a/workspaces/arborist/lib/arborist/isolated-reifier.js +++ b/workspaces/arborist/lib/arborist/isolated-reifier.js @@ -47,7 +47,7 @@ module.exports = cls => class IsolatedReifier extends cls { parent: root, path: join(this.idealGraph.localPath, location), resolved: node.resolved, - root: location, + root, }) // XXX top is from place-dep not lib/node.js newChild.top = { path: this.idealGraph.localPath } @@ -404,11 +404,11 @@ module.exports = cls => class IsolatedReifier extends cls { deprecated: undefined, scripts: dep.package.scripts, version: dep.package.version, - optional, } const link = new IsolatedLink({ location: join(nmFolder, dep.name), name: toKey, + optional, parent: root, package: pkg, path: join(dep.root.localPath, nmFolder, dep.name), From 7bbda0705e5fb76b39f23f5a0ed4dc5ce699691a Mon Sep 17 00:00:00 2001 From: Michael Garvin Date: Fri, 6 Mar 2026 09:28:46 -0800 Subject: [PATCH 3/3] fixup: pr review corrections --- workspaces/arborist/lib/arborist/isolated-reifier.js | 1 + workspaces/arborist/lib/isolated-classes.js | 10 +++++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/arborist/isolated-reifier.js b/workspaces/arborist/lib/arborist/isolated-reifier.js index 24ca9c9c450b6..7644239b03dec 100644 --- a/workspaces/arborist/lib/arborist/isolated-reifier.js +++ b/workspaces/arborist/lib/arborist/isolated-reifier.js @@ -406,6 +406,7 @@ module.exports = cls => class IsolatedReifier extends cls { version: dep.package.version, } const link = new IsolatedLink({ + isStoreLink: true, location: join(nmFolder, dep.name), name: toKey, optional, diff --git a/workspaces/arborist/lib/isolated-classes.js b/workspaces/arborist/lib/isolated-classes.js index b432e91ab0b0b..d9770a386791f 100644 --- a/workspaces/arborist/lib/isolated-classes.js +++ b/workspaces/arborist/lib/isolated-classes.js @@ -111,15 +111,15 @@ class IsolatedNode { // fake lib/link.js class IsolatedLink extends IsolatedNode { - #target = this + #target + isStoreLink = false constructor (options) { super(options) this.#target = options.target - } - - get isStoreLink () { - return true + if (options.isStoreLink) { + this.isStoreLink = true + } } get isLink () {