Skip to content

Commit

Permalink
fix: Arborist code cleanup (#7237)
Browse files Browse the repository at this point in the history 
A bunch of tiny tweaks to Arborist, in service of eventually cleaning it up and removing the mixin approach.

Most of the fixes are like before: removing symbols in favor of private attributes so we can easily find where state is being shared. There is also some shuffling of single-use methods into larger classes.
  • Loading branch information
@wraithgar
wraithgar committed Feb 28, 2024
1 parent 818957c commit 6d1789c
Show file tree
Hide file tree
Showing 19 changed files with 791 additions and 876 deletions.
51 changes: 0 additions & 51 deletions workspaces/arborist/lib/arborist/audit.js
View file

This file was deleted.

102 changes: 56 additions & 46 deletions workspaces/arborist/lib/arborist/build-ideal-tree.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,22 +38,15 @@ const resetDepFlags = require('../reset-dep-flags.js')
// them with unit tests and reuse them across mixins
const _updateAll = Symbol.for('updateAll')
const _flagsSuspect = Symbol.for('flagsSuspect')
const _workspaces = Symbol.for('workspaces')
const _setWorkspaces = Symbol.for('setWorkspaces')
const _updateNames = Symbol.for('updateNames')
const _resolvedAdd = Symbol.for('resolvedAdd')
const _usePackageLock = Symbol.for('usePackageLock')
const _rpcache = Symbol.for('realpathCache')
const _stcache = Symbol.for('statCache')
const _includeWorkspaceRoot = Symbol.for('includeWorkspaceRoot')

// exposed symbol for unit testing the placeDep method directly
const _peerSetSource = Symbol.for('peerSetSource')

// used by Reify mixin
const _force = Symbol.for('force')
const _global = Symbol.for('global')
const _idealTreePrune = Symbol.for('idealTreePrune')
const _addNodeToTrashList = Symbol.for('addNodeToTrashList')

// Push items in, pop them sorted by depth and then path
// Sorts physically shallower deps up to the front of the queue, because
Expand Down Expand Up @@ -117,6 +110,10 @@ module.exports = cls => class IdealTreeBuilder extends cls {
#loadFailures = new Set()
#manifests = new Map()
#mutateTree = false
// a map of each module in a peer set to the thing that depended on
// that set of peers in the first place. Use a WeakMap so that we
// don't hold onto references for nodes that are garbage collected.
#peerSetSource = new WeakMap()
#preferDedupe = false
#prune
#strictPeerDeps
Expand All @@ -131,45 +128,33 @@ module.exports = cls => class IdealTreeBuilder extends cls {

const {
follow = false,
force = false,
global = false,
installStrategy = 'hoisted',
idealTree = null,
includeWorkspaceRoot = false,
installLinks = false,
legacyPeerDeps = false,
packageLock = true,
strictPeerDeps = false,
workspaces = [],
workspaces,
global,
} = options

this[_workspaces] = workspaces || []
this[_force] = !!force
this.#strictPeerDeps = !!strictPeerDeps

this.idealTree = idealTree
this.installLinks = installLinks
this.legacyPeerDeps = legacyPeerDeps

this[_usePackageLock] = packageLock
this[_global] = !!global
this.#installStrategy = global ? 'shallow' : installStrategy
this.#follow = !!follow

if (this[_workspaces].length && this[_global]) {
if (workspaces?.length && global) {
throw new Error('Cannot operate on workspaces in global mode')
}

this[_updateAll] = false
this[_updateNames] = []
this[_resolvedAdd] = []

// a map of each module in a peer set to the thing that depended on
// that set of peers in the first place. Use a WeakMap so that we
// don't hold onto references for nodes that are garbage collected.
this[_peerSetSource] = new WeakMap()

this[_includeWorkspaceRoot] = includeWorkspaceRoot
}

get explicitRequests () {
Expand All @@ -196,7 +181,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {

process.emit('time', 'idealTree')

if (!options.add && !options.rm && !options.update && this[_global]) {
if (!options.add && !options.rm && !options.update && this.options.global) {
throw new Error('global requires add, rm, or update option')
}

Expand Down Expand Up @@ -232,7 +217,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
for (const node of this.idealTree.inventory.values()) {
if (!node.optional) {
try {
checkEngine(node.package, npmVersion, nodeVersion, this[_force])
checkEngine(node.package, npmVersion, nodeVersion, this.options.force)
} catch (err) {
if (engineStrict) {
throw err
Expand All @@ -243,7 +228,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
current: err.current,
})
}
checkPlatform(node.package, this[_force])
checkPlatform(node.package, this.options.force)
}
}
}
Expand Down Expand Up @@ -295,7 +280,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
async #initTree () {
process.emit('time', 'idealTree:init')
let root
if (this[_global]) {
if (this.options.global) {
root = await this.#globalRootNode()
} else {
try {
Expand All @@ -313,7 +298,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
// When updating all, we load the shrinkwrap, but don't bother
// to build out the full virtual tree from it, since we'll be
// reconstructing it anyway.
.then(root => this[_global] ? root
.then(root => this.options.global ? root
: !this[_usePackageLock] || this[_updateAll]
? Shrinkwrap.reset({
path: this.path,
Expand All @@ -329,7 +314,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
// Load on a new Arborist object, so the Nodes aren't the same,
// or else it'll get super confusing when we change them!
.then(async root => {
if ((!this[_updateAll] && !this[_global] && !root.meta.loadedFromDisk) || (this[_global] && this[_updateNames].length)) {
if ((!this[_updateAll] && !this.options.global && !root.meta.loadedFromDisk) || (this.options.global && this[_updateNames].length)) {
await new this.constructor(this.options).loadActual({ root })
const tree = root.target
// even though we didn't load it from a package-lock.json FILE,
Expand Down Expand Up @@ -408,7 +393,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
devOptional: false,
peer: false,
optional: false,
global: this[_global],
global: this.options.global,
installLinks: this.installLinks,
legacyPeerDeps: this.legacyPeerDeps,
loadOverrides: true,
Expand All @@ -423,7 +408,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
devOptional: false,
peer: false,
optional: false,
global: this[_global],
global: this.options.global,
installLinks: this.installLinks,
legacyPeerDeps: this.legacyPeerDeps,
root,
Expand All @@ -438,11 +423,11 @@ module.exports = cls => class IdealTreeBuilder extends cls {
process.emit('time', 'idealTree:userRequests')
const tree = this.idealTree.target

if (!this[_workspaces].length) {
if (!this.options.workspaces.length) {
await this.#applyUserRequestsToNode(tree, options)
} else {
const nodes = this.workspaceNodes(tree, this[_workspaces])
if (this[_includeWorkspaceRoot]) {
const nodes = this.workspaceNodes(tree, this.options.workspaces)
if (this.options.includeWorkspaceRoot) {
nodes.push(tree)
}
const appliedRequests = nodes.map(
Expand All @@ -458,14 +443,14 @@ module.exports = cls => class IdealTreeBuilder extends cls {
// If we have a list of package names to update, and we know it's
// going to update them wherever they are, add any paths into those
// named nodes to the buildIdealTree queue.
if (!this[_global] && this[_updateNames].length) {
if (!this.options.global && this[_updateNames].length) {
this.#queueNamedUpdates()
}

// global updates only update the globalTop nodes, but we need to know
// that they're there, and not reinstall the world unnecessarily.
const globalExplicitUpdateNames = []
if (this[_global] && (this[_updateAll] || this[_updateNames].length)) {
if (this.options.global && (this[_updateAll] || this[_updateNames].length)) {
const nm = resolve(this.path, 'node_modules')
const paths = await readdirScoped(nm).catch(() => [])
for (const p of paths) {
Expand Down Expand Up @@ -510,7 +495,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
// triggers a refresh of all edgesOut. this has to be done BEFORE
// adding the edges to explicitRequests, because the package setter
// resets all edgesOut.
if (add && add.length || rm && rm.length || this[_global]) {
if (add && add.length || rm && rm.length || this.options.global) {
tree.package = tree.package
}

Expand Down Expand Up @@ -616,7 +601,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
//
// XXX: how to handle top nodes that aren't the root? Maybe the report
// just tells the user to cd into that directory and fix it?
if (this[_force] && this.auditReport && this.auditReport.topVulns.size) {
if (this.options.force && this.auditReport && this.auditReport.topVulns.size) {
options.add = options.add || []
options.rm = options.rm || []
const nodesTouched = new Set()
Expand Down Expand Up @@ -900,7 +885,7 @@ This is a one-time fix-up, please be patient...
// dep if allowed.

const tasks = []
const peerSource = this[_peerSetSource].get(node) || node
const peerSource = this.#peerSetSource.get(node) || node
for (const edge of this.#problemEdges(node)) {
if (edge.peerConflicted) {
continue
Expand Down Expand Up @@ -958,7 +943,7 @@ This is a one-time fix-up, please be patient...

auditReport: this.auditReport,
explicitRequest: this.#explicitRequests.has(edge),
force: this[_force],
force: this.options.force,
installLinks: this.installLinks,
installStrategy: this.#installStrategy,
legacyPeerDeps: this.legacyPeerDeps,
Expand Down Expand Up @@ -1099,13 +1084,13 @@ This is a one-time fix-up, please be patient...

// keep track of the thing that caused this node to be included.
const src = parent.sourceReference
this[_peerSetSource].set(node, src)
this.#peerSetSource.set(node, src)

// do not load the peers along with the set if this is a global top pkg
// otherwise we'll be tempted to put peers as other top-level installed
// things, potentially clobbering what's there already, which is not
// what we want. the missing edges will be picked up on the next pass.
if (this[_global] && edge.from.isProjectRoot) {
if (this.options.global && edge.from.isProjectRoot) {
return node
}

Expand Down Expand Up @@ -1328,7 +1313,7 @@ This is a one-time fix-up, please be patient...
const parentEdge = node.parent.edgesOut.get(edge.name)
const { isProjectRoot, isWorkspace } = node.parent.sourceReference
const isMine = isProjectRoot || isWorkspace
const conflictOK = this[_force] || !isMine && !this.#strictPeerDeps
const conflictOK = this.options.force || !isMine && !this.#strictPeerDeps

if (!edge.to) {
if (!parentEdge) {
Expand Down Expand Up @@ -1415,7 +1400,7 @@ This is a one-time fix-up, please be patient...
currentEdge: currentEdge ? currentEdge.explain() : null,
edge: edge.explain(),
strictPeerDeps: this.#strictPeerDeps,
force: this[_force],
force: this.options.force,
}
}

Expand Down Expand Up @@ -1503,7 +1488,7 @@ This is a one-time fix-up, please be patient...
// otherwise, don't bother.
const needPrune = metaFromDisk && (mutateTree || flagsSuspect)
if (this.#prune && needPrune) {
this[_idealTreePrune]()
this.#idealTreePrune()
for (const node of this.idealTree.inventory.values()) {
if (node.extraneous) {
node.parent = null
Expand All @@ -1514,7 +1499,7 @@ This is a one-time fix-up, please be patient...
process.emit('timeEnd', 'idealTree:fixDepFlags')
}

[_idealTreePrune] () {
#idealTreePrune () {
for (const node of this.idealTree.inventory.values()) {
if (node.extraneous) {
node.parent = null
Expand All @@ -1534,4 +1519,29 @@ This is a one-time fix-up, please be patient...
}
}
}

async prune (options = {}) {
// allow the user to set options on the ctor as well.
// XXX: deprecate separate method options objects.
options = { ...this.options, ...options }

await this.buildIdealTree(options)

this.#idealTreePrune()

if (!this.options.workspacesEnabled) {
const excludeNodes = this.excludeWorkspacesDependencySet(this.idealTree)
for (const node of this.idealTree.inventory.values()) {
if (
node.parent !== null
&& !node.isProjectRoot
&& !excludeNodes.has(node)
) {
this[_addNodeToTrashList](node)
}
}
}

return this.reify(options)
}
}
19 changes: 0 additions & 19 deletions workspaces/arborist/lib/arborist/deduper.js
View file

This file was deleted.

0 comments on commit 6d1789c

Please sign in to comment.