fix: code cleanup (#8677)

- constructor logic was consolidated. It takes place in the main
Arborist constructor when possible, allowing us to see all of the
constructor at once and find any duplications or problems. It's evident
that our approach to options/this.options needs some attention.
- Some small single-use methods were inlined into the code that called
them. In many cases this prevented re-pulling variables from `this`.
- remove unused param from call to `#linkFromSpec`. The function is not
expecting a fourth parameter.
- remove unused private attributes, `#dryRun` and `#savePrefix` are not
used anymore

Author: wraithgar

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

@@ -42,7 +42,6 @@ const _flagsSuspect = Symbol.for('flagsSuspect')
 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')
 
@@ -101,39 +100,28 @@ module.exports = cls => class IdealTreeBuilder extends cls {
   constructor (options) {
     super(options)
 
-    // normalize trailing slash
-    const registry = options.registry || 'https://registry.npmjs.org'
-    options.registry = this.registry = registry.replace(/(?<!\/)\/+$/, '') + '/'
-
     const {
       follow = false,
       installStrategy = 'hoisted',
-      idealTree = null,
-      installLinks = false,
-      legacyPeerDeps = false,
-      packageLock = true,
       strictPeerDeps = false,
-      workspaces,
       global,
     } = options
 
     this.#strictPeerDeps = !!strictPeerDeps
 
-    this.idealTree = idealTree
-    this.installLinks = installLinks
-    this.legacyPeerDeps = legacyPeerDeps
-
-    this[_usePackageLock] = packageLock
     this.#installStrategy = global ? 'shallow' : installStrategy
     this.#follow = !!follow
 
-    if (workspaces?.length && global) {
-      throw new Error('Cannot operate on workspaces in global mode')
-    }
-
     this[_updateAll] = false
     this[_updateNames] = []
     this[_resolvedAdd] = []
+
+    // caches for cached realpath calls
+    const cwd = process.cwd()
+    // assume that the cwd is real enough for our purposes
+    this[_rpcache] = new Map([[cwd, cwd]])
+    this[_stcache] = new Map()
+    this[_flagsSuspect] = false
   }
 
   get explicitRequests () {
@@ -298,7 +286,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
       .then(root => {
         if (this.options.global) {
           return root
-        } else if (!this[_usePackageLock] || this[_updateAll]) {
+        } else if (!this.options.usePackageLock || this[_updateAll]) {
           return Shrinkwrap.reset({
             path: this.path,
             lockfileVersion: this.options.lockfileVersion,
@@ -1231,7 +1219,7 @@ This is a one-time fix-up, please be patient...
     }
   }
 
-  #nodeFromSpec (name, spec, parent, edge) {
+  async #nodeFromSpec (name, spec, parent, edge) {
     // pacote will slap integrity on its options, so we have to clone
     // the object so it doesn't get mutated.
     // Don't bother to load the manifest for link deps, because the target
@@ -1260,7 +1248,13 @@ This is a one-time fix-up, please be patient...
     // Decide whether to link or copy the dependency
     const shouldLink = (isWorkspace || isProjectInternalFileSpec || !installLinks) && !isTransitiveFileDep
     if (spec.type === 'directory' && shouldLink) {
-      return this.#linkFromSpec(name, spec, parent, edge)
+      const realpath = spec.fetchSpec
+      const { content: pkg } = await PackageJson.normalize(realpath).catch(() => {
+        return { content: {} }
+      })
+      const link = new Link({ name, parent, realpath, pkg, installLinks, legacyPeerDeps })
+      this.#linkNodes.add(link)
+      return link
     }
 
     // if the spec matches a workspace name, then see if the workspace node will satisfy the edge. if it does, we return the workspace node to make sure it takes priority.
@@ -1301,17 +1295,6 @@ This is a one-time fix-up, please be patient...
       })
   }
 
-  async #linkFromSpec (name, spec, parent) {
-    const realpath = spec.fetchSpec
-    const { installLinks, legacyPeerDeps } = this
-    const { content: pkg } = await PackageJson.normalize(realpath).catch(() => {
-      return { content: {} }
-    })
-    const link = new Link({ name, parent, realpath, pkg, installLinks, legacyPeerDeps })
-    this.#linkNodes.add(link)
-    return link
-  }
-
   // load all peer deps and meta-peer deps into the node's parent
   // At the end of this, the node's peer-type outward edges are all
   // resolved, and so are all of theirs, but other dep types are not.
@@ -1446,6 +1429,7 @@ This is a one-time fix-up, please be patient...
   //   and add it to the _depsQueue
   //
   // call buildDepStep if anything was added to the queue; otherwise, we're done
+  // XXX load-virtual also has a #resolveLinks, is there overlap?
   #resolveLinks () {
     for (const link of this.#linkNodes) {
       this.#linkNodes.delete(link)

workspaces/arborist/lib/arborist/index.js

@@ -68,6 +68,34 @@ class Arborist extends Base {
   constructor (options = {}) {
     const timeEnd = time.start('arborist:ctor')
     super(options)
+
+    // normalize trailing slash
+    const registry = options.registry || 'https://registry.npmjs.org'
+    options.registry = this.registry = registry.replace(/(?<!\/)\/+$/, '') + '/'
+
+    // TODO as we consolidate constructors it's more apparent that we are not parsing options and using this.options consistently
+    const {
+      actualTree,
+      global,
+      idealTree = null,
+      installLinks = false,
+      legacyPeerDeps = false,
+      virtualTree,
+      workspaces,
+    } = options
+
+    if (workspaces?.length && global) {
+      throw new Error('Cannot operate on workspaces in global mode')
+    }
+
+    // the tree of nodes on disk
+    this.actualTree = actualTree
+    this.idealTree = idealTree
+    this.installLinks = installLinks
+    this.legacyPeerDeps = legacyPeerDeps
+    // the virtual tree we load from a shrinkwrap
+    this.virtualTree = virtualTree
+
     this.options = {
       nodeVersion: process.version,
       ...options,
@@ -88,6 +116,7 @@ class Arborist extends Base {
       replaceRegistryHost: options.replaceRegistryHost,
       savePrefix: 'savePrefix' in options ? options.savePrefix : '^',
       scriptShell: options.scriptShell,
+      usePackageLock: 'packageLock' in options ? options.packageLock : true,
       workspaces: options.workspaces || [],
       workspacesEnabled: options.workspacesEnabled !== false,
     }
@@ -104,6 +133,7 @@ class Arborist extends Base {
     this.cache = resolve(this.options.cache)
     this.diff = null
     this.path = resolve(this.options.path)
+    this.scriptsRun = new Set()
     timeEnd()
   }
 

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

@@ -1,6 +1,5 @@
 const _makeIdealGraph = Symbol('makeIdealGraph')
 const _createIsolatedTree = Symbol.for('createIsolatedTree')
-const _createBundledTree = Symbol('createBundledTree')
 const { mkdirSync } = require('node:fs')
 const pacote = require('pacote')
 const { join } = require('node:path')
@@ -162,7 +161,7 @@ module.exports = cls => class IsolatedReifier extends cls {
     result.hasInstallScript = node.hasInstallScript
   }
 
-  async [_createBundledTree] () {
+  async #createBundledTree () {
     // TODO: make sure that idealTree object exists
     const idealTree = this.idealTree
     // TODO: test workspaces having bundled deps
@@ -217,7 +216,7 @@ module.exports = cls => class IsolatedReifier extends cls {
 
     const proxiedIdealTree = this.idealGraph
 
-    const bundledTree = await this[_createBundledTree]()
+    const bundledTree = await this.#createBundledTree()
 
     const treeHash = (startNode) => {
       // generate short hash based on the dependency tree

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

@@ -41,19 +41,6 @@ module.exports = cls => class ActualLoader extends cls {
   #topNodes = new Set()
   #transplantFilter
 
-  constructor (options) {
-    super(options)
-
-    // the tree of nodes on disk
-    this.actualTree = options.actualTree
-
-    // caches for cached realpath calls
-    const cwd = process.cwd()
-    // assume that the cwd is real enough for our purposes
-    this[_rpcache] = new Map([[cwd, cwd]])
-    this[_stcache] = new Map()
-  }
-
   // public method
   // TODO remove options param in next semver major
   async loadActual (options = {}) {

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

@@ -18,14 +18,6 @@ const setWorkspaces = Symbol.for('setWorkspaces')
 module.exports = cls => class VirtualLoader extends cls {
   #rootOptionProvided
 
-  constructor (options) {
-    super(options)
-
-    // the virtual tree we load from a shrinkwrap
-    this.virtualTree = options.virtualTree
-    this[flagsSuspect] = false
-  }
-
   // public method
   async loadVirtual (options = {}) {
     if (this.virtualTree) {
@@ -77,7 +69,21 @@ module.exports = cls => class VirtualLoader extends cls {
     this.#checkRootEdges(s, root)
     root.meta = s
     this.virtualTree = root
-    const { links, nodes } = this.#resolveNodes(s, root)
+    // separate out link metadata, and create Node objects for nodes
+    const links = new Map()
+    const nodes = new Map([['', root]])
+    for (const [location, meta] of Object.entries(s.data.packages)) {
+      // skip the root because we already got it
+      if (!location) {
+        continue
+      }
+
+      if (meta.link) {
+        links.set(location, meta)
+      } else {
+        nodes.set(location, this.#loadNode(location, meta))
+      }
+    }
     await this.#resolveLinks(links, nodes)
     if (!(s.originalLockfileVersion >= 2)) {
       this.#assignBundles(nodes)
@@ -160,27 +166,9 @@ module.exports = cls => class VirtualLoader extends cls {
     }
   }
 
-  // separate out link metadata, and create Node objects for nodes
-  #resolveNodes (s, root) {
-    const links = new Map()
-    const nodes = new Map([['', root]])
-    for (const [location, meta] of Object.entries(s.data.packages)) {
-      // skip the root because we already got it
-      if (!location) {
-        continue
-      }
-
-      if (meta.link) {
-        links.set(location, meta)
-      } else {
-        nodes.set(location, this.#loadNode(location, meta))
-      }
-    }
-    return { links, nodes }
-  }
-
   // links is the set of metadata, and nodes is the map of non-Link nodes
   // Set the targets to nodes in the set, if we have them (we might not)
+  // XXX build-ideal-tree also has a #resolveLinks, is there overlap?
   async #resolveLinks (links, nodes) {
     for (const [location, meta] of links.entries()) {
       const targetPath = resolve(this.path, meta.resolved)

workspaces/arborist/lib/arborist/rebuild.js

@@ -24,13 +24,12 @@ const _trashList = Symbol.for('trashList')
 module.exports = cls => class Builder extends cls {
   #doHandleOptionalFailure
   #oldMeta = null
-  #queues
-
-  constructor (options) {
-    super(options)
-
-    this.scriptsRun = new Set()
-    this.#resetQueues()
+  #queues = {
+    preinstall: [],
+    install: [],
+    postinstall: [],
+    prepare: [],
+    bin: [],
   }
 
   async rebuild ({ nodes, handleOptionalFailure = false } = {}) {
@@ -62,7 +61,13 @@ module.exports = cls => class Builder extends cls {
 
     // build link deps
     if (linkNodes.size) {
-      this.#resetQueues()
+      this.#queues = {
+        preinstall: [],
+        install: [],
+        postinstall: [],
+        prepare: [],
+        bin: [],
+      }
       await this.#build(linkNodes, { type: 'links' })
     }
 
@@ -132,16 +137,6 @@ module.exports = cls => class Builder extends cls {
     }
   }
 
-  #resetQueues () {
-    this.#queues = {
-      preinstall: [],
-      install: [],
-      postinstall: [],
-      prepare: [],
-      bin: [],
-    }
-  }
-
   async #build (nodes, { type = 'deps' }) {
     const timeEnd = time.start(`build:${type}`)
 

workspaces/arborist/lib/arborist/reify.js

@@ -57,11 +57,9 @@ const _rollbackRetireShallowNodes = Symbol.for('rollbackRetireShallowNodes')
 const _rollbackCreateSparseTree = Symbol.for('rollbackCreateSparseTree')
 const _rollbackMoveBackRetiredUnchanged = Symbol.for('rollbackMoveBackRetiredUnchanged')
 const _saveIdealTree = Symbol.for('saveIdealTree')
-const _reifyPackages = Symbol.for('reifyPackages')
 
 // defined by build-ideal-tree mixin
 const _resolvedAdd = Symbol.for('resolvedAdd')
-const _usePackageLock = Symbol.for('usePackageLock')
 // used by build-ideal-tree mixin
 const _addNodeToTrashList = Symbol.for('addNodeToTrashList')
 
@@ -70,12 +68,10 @@ const _createIsolatedTree = Symbol.for('createIsolatedTree')
 module.exports = cls => class Reifier extends cls {
   #bundleMissing = new Set() // child nodes we'd EXPECT to be included in a bundle, but aren't
   #bundleUnpacked = new Set() // the nodes we unpack to read their bundles
-  #dryRun
   #nmValidated = new Set()
   #omit
   #retiredPaths = {}
   #retiredUnchanged = {}
-  #savePrefix
   #shrinkwrapInflated = new Set()
   #sparseTreeDirs = new Set()
   #sparseTreeRoots = new Set()
@@ -122,7 +118,7 @@ module.exports = cls => class Reifier extends cls {
       this.idealTree = await this[_createIsolatedTree]()
     }
     await this[_diffTrees]()
-    await this[_reifyPackages]()
+    await this.#reifyPackages()
     if (linked) {
       // swap back in the idealTree
       // so that the lockfile is preserved
@@ -261,7 +257,7 @@ module.exports = cls => class Reifier extends cls {
     return treeCheck(this.actualTree)
   }
 
-  async [_reifyPackages] () {
+  async #reifyPackages () {
     // we don't submit the audit report or write to disk on dry runs
     if (this.options.dryRun) {
       return
@@ -1496,7 +1492,7 @@ module.exports = cls => class Reifier extends cls {
 
     // before now edge specs could be changing, affecting the `requires` field
     // in the package lock, so we hold off saving to the very last action
-    if (this[_usePackageLock]) {
+    if (this.options.usePackageLock) {
       // preserve indentation, if possible
       let format = this.idealTree.package[Symbol.for('indent')]
       if (format === undefined) {

workspaces/arborist/lib/optional-set.js

@@ -10,10 +10,6 @@
 
 const gatherDepSet = require('./gather-dep-set.js')
 const optionalSet = node => {
-  if (!node.optional) {
-    return new Set()
-  }
-
   // start with the node, then walk up the dependency graph until we
   // get to the boundaries that define the optional set.  since the
   // node is optional, we know that all paths INTO this area of the