chore: refactor node module collector, extract explicit DependencyTree, update types to be generic and respective to npm list vs pnpm list dependency trees (electron-userland#8872)

mmaietta · web-flow · commit 7f6c3fea6fea · 2025-02-19T17:00:04.000-08:00
- Moves functions around so that they're topologically ordered.
- Updates `types` for explicit usage of what is the result `JSON.parse`
and what is used for the production tree
- Removes `delete array[key]` logic by refactoring to `reduce` (no more
modifying the original `JSON.parse` tree)
- Extracts `JSON.parse` functionality to be specific to each collector
- Extracts `DependencyTree` with initial set
`implicitDependenciesInjected = false` so that the boolean isn't
optional by using DFS approach
- Consolidates implicit dependency injection to npm-specific logic by
extracting simplified dependency tree w/o `dependencies`
- Converted the abstract collector class to be generic of type:
`NodeModulesCollector&lt;T extends Dependency&lt;T, OptionalsType&gt;,
OptionalsType&gt;`
- This way, all abstracted and overridden methods are easily typed per
their respective `JSON.parse` objects. Then uses a common method to
construct the dependencies into a `DependencyTree` and then internally
(as before) hoisting and converting to `NodeModuleInfo[]`
- This accommodates the differences of `optionalDependencies` within
`pnpm list` (`PnpmDependency`) and `npm list` (`string`)
diff --git a/.changeset/funny-gorillas-fold.md b/.changeset/funny-gorillas-fold.md
@@ -0,0 +1,5 @@
+---
+"app-builder-lib": patch
+---
+
+chore: refactor node module collector, extract explicit `DependencyTree`, update types to be generic and respective to `npm list` vs `pnpm list` dependency trees
diff --git a/.changeset/lazy-boats-love.md b/.changeset/lazy-boats-love.md
@@ -0,0 +1,5 @@
+---
+"app-builder-lib": patch
+---
+
+chore: refactor node module collector to reduce recursion, extract explicit DependencyTree, update types
diff --git a/packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts b/packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts
@@ -1,40 +1,77 @@
 import { hoist, type HoisterTree, type HoisterResult } from "./hoist"
 import * as path from "path"
 import * as fs from "fs"
-import { NodeModuleInfo, DependencyTree, DependencyGraph } from "./types"
+import type { NodeModuleInfo, DependencyTree, DependencyGraph, Dependency } from "./types"
 import { exec, log } from "builder-util"
 
-export abstract class NodeModulesCollector {
-  private nodeModules: NodeModuleInfo[]
-  protected dependencyPathMap: Map<string, string>
-  protected allDependencies: Map<string, DependencyTree> = new Map()
+export abstract class NodeModulesCollector<T extends Dependency<T, OptionalsType>, OptionalsType> {
+  private nodeModules: NodeModuleInfo[] = []
+  protected dependencyPathMap: Map<string, string> = new Map()
+  protected allDependencies: Map<string, T> = new Map()
 
-  constructor(private readonly rootDir: string) {
-    this.dependencyPathMap = new Map()
-    this.nodeModules = []
+  constructor(private readonly rootDir: string) {}
+
+  public async getNodeModules(): Promise<NodeModuleInfo[]> {
+    const tree: T = await this.getDependenciesTree()
+    const realTree: T = this.getTreeFromWorkspaces(tree)
+    const parsedTree: Dependency<T, OptionalsType> = this.extractRelevantData(realTree)
+
+    this.collectAllDependencies(parsedTree)
+
+    const productionTree: DependencyTree = this.extractProductionDependencyTree(parsedTree)
+    const dependencyGraph: DependencyGraph = this.convertToDependencyGraph(productionTree)
+
+    const hoisterResult: HoisterResult = hoist(this.transToHoisterTree(dependencyGraph), { check: true })
+    this._getNodeModules(hoisterResult.dependencies, this.nodeModules)
+
+    return this.nodeModules
   }
 
-  private transToHoisterTree(obj: DependencyGraph, key: string = `.`, nodes: Map<string, HoisterTree> = new Map()): HoisterTree {
-    let node = nodes.get(key)
-    const name = key.match(/@?[^@]+/)![0]
-    if (!node) {
-      node = {
-        name,
-        identName: name,
-        reference: key.match(/@?[^@]+@?(.+)?/)![1] || ``,
-        dependencies: new Set<HoisterTree>(),
-        peerNames: new Set<string>([]),
-      }
-      nodes.set(key, node)
+  protected abstract getCommand(): string
+  protected abstract getArgs(): string[]
+  protected abstract parseDependenciesTree(jsonBlob: string): T
+  protected abstract extractProductionDependencyTree(tree: Dependency<T, OptionalsType>): DependencyTree
 
-      for (const dep of (obj[key] || {}).dependencies || []) {
-        node.dependencies.add(this.transToHoisterTree(obj, dep, nodes))
-      }
+  protected async getDependenciesTree(): Promise<T> {
+    const command = this.getCommand()
+    const args = this.getArgs()
+    const dependencies = await exec(command, args, {
+      cwd: this.rootDir,
+      shell: true,
+    })
+    return this.parseDependenciesTree(dependencies)
+  }
+
+  protected extractRelevantData(npmTree: T): Dependency<T, OptionalsType> {
+    // Do not use `...npmTree` as we are explicitly extracting the data we need
+    const { name, version, path, workspaces, dependencies } = npmTree
+    const tree: Dependency<T, OptionalsType> = {
+      name,
+      version,
+      path,
+      workspaces,
+      // DFS extract subtree
+      dependencies: this.extractInternal(dependencies),
     }
-    return node
+
+    return tree
   }
 
-  protected resolvePath(filePath: string) {
+  protected extractInternal(deps: T["dependencies"]): T["dependencies"] {
+    return deps && Object.keys(deps).length > 0
+      ? Object.entries(deps).reduce((accum, [packageName, depObjectOrVersionString]) => {
+          return {
+            ...accum,
+            [packageName]:
+              typeof depObjectOrVersionString === "object" && Object.keys(depObjectOrVersionString).length > 0
+                ? this.extractRelevantData(depObjectOrVersionString)
+                : depObjectOrVersionString,
+          }
+        }, {})
+      : undefined
+  }
+
+  protected resolvePath(filePath: string): string {
     try {
       const stats = fs.lstatSync(filePath)
       if (stats.isSymbolicLink()) {
@@ -48,70 +85,89 @@ export abstract class NodeModulesCollector {
     }
   }
 
-  private convertToDependencyGraph(tree: DependencyTree): DependencyGraph {
-    const result: DependencyGraph = { ".": {} }
-
-    const flatten = (node: DependencyTree, parentKey = ".") => {
-      const dependencies = node.dependencies || {}
-
-      for (const [key, value] of Object.entries(dependencies)) {
-        // Skip empty dependencies(like some optionalDependencies)
-        if (Object.keys(value).length === 0) {
-          continue
-        }
-        const version = value.version || ""
-        const newKey = `${key}@${version}`
-        this.dependencyPathMap.set(newKey, path.normalize(this.resolvePath(value.path)))
-        if (!result[parentKey]?.dependencies) {
-          result[parentKey] = { dependencies: [] }
-        }
-        result[parentKey].dependencies!.push(newKey)
-
-        if (node.__circularDependencyDetected) {
-          continue
-        }
-        flatten(value, newKey)
+  private convertToDependencyGraph(tree: DependencyTree, parentKey = "."): DependencyGraph {
+    return Object.entries(tree.dependencies || {}).reduce<DependencyGraph>((acc, curr) => {
+      const [packageName, dependencies] = curr
+      // Skip empty dependencies (like some optionalDependencies)
+      if (Object.keys(dependencies).length === 0) {
+        return acc
+      }
+      const version = dependencies.version || ""
+      const newKey = `${packageName}@${version}`
+      if (!dependencies.path) {
+        log.error(
+          {
+            packageName,
+            data: dependencies,
+            parentModule: tree.name,
+            parentVersion: tree.version,
+          },
+          "dependency path is undefined"
+        )
+        throw new Error("unable to parse `path` during `tree.dependencies` reduce")
+      }
+      // Map dependency details: name, version and path to the dependency tree
+      this.dependencyPathMap.set(newKey, path.normalize(this.resolvePath(dependencies.path)))
+      if (!acc[parentKey]) {
+        acc[parentKey] = { dependencies: [] }
+      }
+      acc[parentKey].dependencies.push(newKey)
+      if (tree.implicitDependenciesInjected) {
+        log.debug(
+          {
+            dependency: packageName,
+            version,
+            path: dependencies.path,
+            parentModule: tree.name,
+            parentVersion: tree.version,
+          },
+          "converted implicit dependency"
+        )
+        return acc
       }
-    }
 
-    flatten(tree)
-    return result
+      return { ...acc, ...this.convertToDependencyGraph(dependencies, newKey) }
+    }, {})
   }
 
-  getAllDependencies(tree: DependencyTree) {
-    const dependencies = tree.dependencies || {}
-    for (const [key, value] of Object.entries(dependencies)) {
-      if (value.dependencies && Object.keys(value.dependencies).length > 0) {
+  private collectAllDependencies(tree: Dependency<T, OptionalsType>) {
+    for (const [key, value] of Object.entries(tree.dependencies || {})) {
+      if (Object.keys(value.dependencies ?? {}).length > 0) {
         this.allDependencies.set(`${key}@${value.version}`, value)
-        this.getAllDependencies(value)
+        this.collectAllDependencies(value)
       }
     }
   }
 
-  abstract getCommand(): string
-  abstract getArgs(): string[]
-  abstract removeNonProductionDependencie(tree: DependencyTree): void
+  private getTreeFromWorkspaces(tree: T): T {
+    if (tree.workspaces && tree.dependencies) {
+      for (const [key, value] of Object.entries(tree.dependencies)) {
+        if (this.rootDir.endsWith(path.normalize(key))) {
+          return value
+        }
+      }
+    }
+    return tree
+  }
 
-  protected async getDependenciesTree(): Promise<DependencyTree> {
-    const command = this.getCommand()
-    const args = this.getArgs()
-    const dependencies = await exec(command, args, {
-      cwd: this.rootDir,
-      shell: true,
-    })
-    const dependencyTree: DependencyTree | DependencyTree[] = JSON.parse(dependencies)
+  private transToHoisterTree(obj: DependencyGraph, key: string = `.`, nodes: Map<string, HoisterTree> = new Map()): HoisterTree {
+    let node = nodes.get(key)
+    const name = key.match(/@?[^@]+/)![0]
+    if (!node) {
+      node = {
+        name,
+        identName: name,
+        reference: key.match(/@?[^@]+@?(.+)?/)![1] || ``,
+        dependencies: new Set<HoisterTree>(),
+        peerNames: new Set<string>([]),
+      }
+      nodes.set(key, node)
 
-    // pnpm returns an array of dependency trees
-    if (Array.isArray(dependencyTree)) {
-      const tree = dependencyTree[0]
-      if (tree.optionalDependencies) {
-        tree.dependencies = { ...tree.dependencies, ...tree.optionalDependencies }
+      for (const dep of (obj[key] || {}).dependencies || []) {
+        node.dependencies.add(this.transToHoisterTree(obj, dep, nodes))
       }
-      return tree
     }
-
-    // yarn and npm return a single dependency tree
-    return dependencyTree
+    return node
   }
 
   private _getNodeModules(dependencies: Set<HoisterResult>, result: NodeModuleInfo[]) {
@@ -133,32 +189,10 @@ export abstract class NodeModulesCollector {
       }
       result.push(node)
       if (d.dependencies.size > 0) {
-        node["dependencies"] = []
-        this._getNodeModules(d.dependencies, node["dependencies"])
+        node.dependencies = []
+        this._getNodeModules(d.dependencies, node.dependencies)
       }
     }
     result.sort((a, b) => a.name.localeCompare(b.name))
   }
-
-  private getTreeFromWorkspaces(tree: DependencyTree): DependencyTree {
-    if (tree.workspaces && tree.dependencies) {
-      for (const [key, value] of Object.entries(tree.dependencies)) {
-        if (this.rootDir.endsWith(path.normalize(key))) {
-          return value
-        }
-      }
-    }
-    return tree
-  }
-
-  public async getNodeModules(): Promise<NodeModuleInfo[]> {
-    const tree = await this.getDependenciesTree()
-    const realTree = this.getTreeFromWorkspaces(tree)
-    this.getAllDependencies(realTree)
-    this.removeNonProductionDependencie(realTree)
-    const dependencyGraph = this.convertToDependencyGraph(realTree)
-    const hoisterResult = hoist(this.transToHoisterTree(dependencyGraph), { check: true })
-    this._getNodeModules(hoisterResult.dependencies, this.nodeModules)
-    return this.nodeModules
-  }
 }
diff --git a/packages/app-builder-lib/src/node-module-collector/npmNodeModulesCollector.ts b/packages/app-builder-lib/src/node-module-collector/npmNodeModulesCollector.ts
@@ -1,8 +1,8 @@
 import { NodeModulesCollector } from "./nodeModulesCollector"
-import { DependencyTree } from "./types"
+import { DependencyTree, NpmDependency, ParsedDependencyTree } from "./types"
 import { log } from "builder-util"
 
-export class NpmNodeModulesCollector extends NodeModulesCollector {
+export class NpmNodeModulesCollector extends NodeModulesCollector<NpmDependency, string> {
   constructor(rootDir: string) {
     super(rootDir)
   }
@@ -15,22 +15,56 @@ export class NpmNodeModulesCollector extends NodeModulesCollector {
     return ["list", "-a", "--include", "prod", "--include", "optional", "--omit", "dev", "--json", "--long", "--silent"]
   }
 
-  removeNonProductionDependencie(tree: DependencyTree) {
-    const dependencies = tree.dependencies || {}
-    const _dependencies = tree._dependencies || {}
-    if (Object.keys(_dependencies).length > 0 && Object.keys(dependencies).length === 0) {
-      tree.dependencies = this.allDependencies.get(`${tree.name}@${tree.version}`)?.dependencies || {}
-      tree.__circularDependencyDetected = true
-      log.debug({ name: tree.name, version: tree.version }, "circular dependency detected")
-      return
+  protected extractRelevantData(npmTree: NpmDependency): NpmDependency {
+    const tree = super.extractRelevantData(npmTree)
+    const { optionalDependencies, _dependencies } = npmTree
+    return { ...tree, optionalDependencies, _dependencies }
+  }
+
+  protected extractProductionDependencyTree(tree: NpmDependency): DependencyTree {
+    const _deps = tree._dependencies ?? {}
+
+    let deps = tree.dependencies ?? {}
+    let implicitDependenciesInjected = false
+
+    if (Object.keys(_deps).length > 0 && Object.keys(deps).length === 0) {
+      log.debug({ name: tree.name, version: tree.version }, "injecting implicit _dependencies")
+      deps = this.allDependencies.get(`${tree.name}@${tree.version}`)?.dependencies ?? {}
+      implicitDependenciesInjected = true
     }
 
-    for (const [key, value] of Object.entries(dependencies)) {
-      if (!_dependencies[key] || Object.keys(value).length === 0) {
-        delete dependencies[key]
-        continue
+    const dependencies = Object.entries(deps).reduce<DependencyTree["dependencies"]>((acc, curr) => {
+      const [packageName, dependency] = curr
+      if (!_deps[packageName] || Object.keys(dependency).length === 0) {
+        return acc
+      }
+      if (implicitDependenciesInjected) {
+        const { name, version, path, workspaces } = dependency
+        const simplifiedTree: ParsedDependencyTree = { name, version, path, workspaces }
+        return {
+          ...acc,
+          [packageName]: { ...simplifiedTree, implicitDependenciesInjected },
+        }
       }
-      this.removeNonProductionDependencie(value)
+      return {
+        ...acc,
+        [packageName]: this.extractProductionDependencyTree(dependency),
+      }
+    }, {})
+
+    const { name, version, path: packagePath, workspaces } = tree
+    const depTree: DependencyTree = {
+      name,
+      version,
+      path: packagePath,
+      workspaces,
+      dependencies,
+      implicitDependenciesInjected,
     }
+    return depTree
+  }
+
+  protected parseDependenciesTree(jsonBlob: string): NpmDependency {
+    return JSON.parse(jsonBlob)
   }
 }
diff --git a/packages/app-builder-lib/src/node-module-collector/pnpmNodeModulesCollector.ts b/packages/app-builder-lib/src/node-module-collector/pnpmNodeModulesCollector.ts
diff --git a/packages/app-builder-lib/src/node-module-collector/types.ts b/packages/app-builder-lib/src/node-module-collector/types.ts
