fix: code cleanup

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