fix: remove implicit dependencies handling (#9010)

beyondkmp · web-flow · commit 8bd1a10a2dcd · 2025-04-11T07:44:11.000+08:00
fix #9000 and #9006 **Root Cause** It directly copies `node_modules` into the app directory and removes `devDependencies` from `package.json`. This way, `npm list` can retrieve all dependencies. However, during the lookup process, if there are duplicate dependency packages, some `dependencies` in `npm` may be empty. A previous fix（#8864 ） introduced the concept of `implicit dependencies` to address this issue, but it only handled the first level of `implicit dependencies`. If `implicit dependencies` themselves contain further `implicit dependencies`, the problem described in #9000 will occur. **How to fix** The current approach directly searches the `parsedTree` and updates the global `this.productionGraph`. If there are cases where `dependencies` are empty, it retrieves the same dependency from `allDependencies` and performs a lookup for sub-dependencies. If the dependency already exists in `this.productionGraph`, no further lookup is performed. Additionally, to prevent infinite loops, an empty array is set before searching for sub-dependencies. Now that two tree lookup has been eliminated, there will be a certain improvement in performance.
diff --git a/.changeset/lovely-shrimps-buy.md b/.changeset/lovely-shrimps-buy.md
@@ -0,0 +1,5 @@
+---
+"app-builder-lib": patch
+---
+
+fix: remove implicit dependencies handling
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,28 +1,24 @@
 import { hoist, type HoisterTree, type HoisterResult } from "./hoist"
 import * as path from "path"
 import * as fs from "fs"
-import type { NodeModuleInfo, DependencyTree, DependencyGraph, Dependency } from "./types"
+import type { NodeModuleInfo, DependencyGraph, Dependency } from "./types"
 import { exec, log } from "builder-util"
 import { Lazy } from "lazy-val"
 
 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()
+  protected productionGraph: DependencyGraph = {}
 
   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(realTree)
+    this.extractProductionDependencyGraph(realTree, "." /*root project name*/)
 
-    this.collectAllDependencies(parsedTree)
-
-    const productionTree: DependencyTree = this.extractProductionDependencyTree(parsedTree)
-    const dependencyGraph: DependencyGraph = this.convertToDependencyGraph(productionTree)
-
-    const hoisterResult: HoisterResult = hoist(this.transToHoisterTree(dependencyGraph), { check: true })
+    const hoisterResult: HoisterResult = hoist(this.transToHoisterTree(this.productionGraph), { check: true })
     this._getNodeModules(hoisterResult.dependencies, this.nodeModules)
 
     return this.nodeModules
@@ -36,7 +32,8 @@ export abstract class NodeModulesCollector<T extends Dependency<T, OptionalsType
   protected abstract readonly pmCommand: Lazy<string>
   protected abstract getArgs(): string[]
   protected abstract parseDependenciesTree(jsonBlob: string): T
-  protected abstract extractProductionDependencyTree(tree: Dependency<T, OptionalsType>): DependencyTree
+  protected abstract extractProductionDependencyGraph(tree: Dependency<T, OptionalsType>, dependencyId: string): void
+  protected abstract collectAllDependencies(tree: Dependency<T, OptionalsType>): void
 
   protected async getDependenciesTree(): Promise<T> {
     const command = await this.pmCommand.value
@@ -48,35 +45,6 @@ export abstract class NodeModulesCollector<T extends Dependency<T, OptionalsType
     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 tree
-  }
-
-  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)
@@ -91,60 +59,6 @@ export abstract class NodeModulesCollector<T extends Dependency<T, OptionalsType
     }
   }
 
-  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
-      }
-
-      return { ...acc, ...this.convertToDependencyGraph(dependencies, newKey) }
-    }, {})
-  }
-
-  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.collectAllDependencies(value)
-      }
-    }
-  }
-
   private getTreeFromWorkspaces(tree: T): T {
     if (tree.workspaces && tree.dependencies) {
       const packageJson: Dependency<string, string> = require(path.join(this.rootDir, "package.json"))
@@ -186,15 +100,15 @@ export abstract class NodeModulesCollector<T extends Dependency<T, OptionalsType
 
     for (const d of dependencies.values()) {
       const reference = [...d.references][0]
-      const p = this.dependencyPathMap.get(`${d.name}@${reference}`)
+      const p = this.allDependencies.get(`${d.name}@${reference}`)?.path
       if (p === undefined) {
         log.debug({ name: d.name, reference }, "cannot find path for dependency")
         continue
       }
       const node: NodeModuleInfo = {
         name: d.name,
         version: reference,
-        dir: p,
+        dir: this.resolvePath(p),
       }
       result.push(node)
       if (d.dependencies.size > 0) {
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,7 +1,6 @@
 import { Lazy } from "lazy-val"
 import { NodeModulesCollector } from "./nodeModulesCollector"
-import { DependencyTree, NpmDependency, ParsedDependencyTree } from "./types"
-import { log } from "builder-util"
+import { NpmDependency } from "./types"
 
 export class NpmNodeModulesCollector extends NodeModulesCollector<NpmDependency, string> {
   constructor(rootDir: string) {
@@ -15,53 +14,38 @@ export class NpmNodeModulesCollector extends NodeModulesCollector<NpmDependency,
     return ["list", "-a", "--include", "prod", "--include", "optional", "--omit", "dev", "--json", "--long", "--silent"]
   }
 
-  protected extractRelevantData(npmTree: NpmDependency): NpmDependency {
-    const tree = super.extractRelevantData(npmTree)
-    const { optionalDependencies, _dependencies } = npmTree
-    return { ...tree, optionalDependencies, _dependencies }
+  protected collectAllDependencies(tree: NpmDependency) {
+    for (const [key, value] of Object.entries(tree.dependencies || {})) {
+      const { _dependencies = {}, dependencies = {} } = value
+      const isDuplicateDep = Object.keys(_dependencies).length > 0 && Object.keys(dependencies).length === 0
+      if (isDuplicateDep) {
+        continue
+      }
+      this.allDependencies.set(`${key}@${value.version}`, value)
+      this.collectAllDependencies(value)
+    }
   }
 
-  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
+  protected extractProductionDependencyGraph(tree: NpmDependency, dependencyId: string): void {
+    if (this.productionGraph[dependencyId]) {
+      return
     }
 
-    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 },
-        }
-      }
-      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
+    const { _dependencies: prodDependencies = {}, dependencies = {} } = tree
+    const isDuplicateDep = Object.keys(prodDependencies).length > 0 && Object.keys(dependencies).length === 0
+    const resolvedDeps = isDuplicateDep ? (this.allDependencies.get(dependencyId)?.dependencies ?? {}) : dependencies
+    // Initialize with empty dependencies array first to mark this dependency as "in progress"
+    // After initialization, if there are libraries with the same name+version later, they will not be searched recursively again
+    // This will prevents infinite loops when circular dependencies are encountered.
+    this.productionGraph[dependencyId] = { dependencies: [] }
+    const productionDeps = Object.entries(resolvedDeps)
+      .filter(([packageName]) => prodDependencies[packageName])
+      .map(([packageName, dependency]) => {
+        const childDependencyId = `${packageName}@${dependency.version}`
+        this.extractProductionDependencyGraph(dependency, childDependencyId)
+        return childDependencyId
+      })
+    this.productionGraph[dependencyId] = { dependencies: productionDeps }
   }
 
   protected parseDependenciesTree(jsonBlob: string): NpmDependency {
diff --git a/packages/app-builder-lib/src/node-module-collector/pnpmNodeModulesCollector.ts b/packages/app-builder-lib/src/node-module-collector/pnpmNodeModulesCollector.ts
@@ -1,8 +1,9 @@
 import { Lazy } from "lazy-val"
 import { NodeModulesCollector } from "./nodeModulesCollector"
-import { Dependency, DependencyTree, PnpmDependency } from "./types"
-import * as path from "path"
+import { PnpmDependency, Dependency } from "./types"
 import { exec, log } from "builder-util"
+import * as path from "path"
+import * as fs from "fs"
 
 export class PnpmNodeModulesCollector extends NodeModulesCollector<PnpmDependency, PnpmDependency> {
   constructor(rootDir: string) {
@@ -28,54 +29,53 @@ export class PnpmNodeModulesCollector extends NodeModulesCollector<PnpmDependenc
     return ["list", "--prod", "--json", "--depth", "Infinity"]
   }
 
-  protected extractRelevantData(npmTree: PnpmDependency): PnpmDependency {
-    const tree = super.extractRelevantData(npmTree)
-    return {
-      ...tree,
-      optionalDependencies: this.extractInternal(npmTree.optionalDependencies),
+  extractProductionDependencyGraph(tree: PnpmDependency, dependencyId: string): void {
+    if (this.productionGraph[dependencyId]) {
+      return
     }
-  }
 
-  extractProductionDependencyTree(tree: PnpmDependency): DependencyTree {
     const p = path.normalize(this.resolvePath(tree.path))
     const packageJson: Dependency<string, string> = require(path.join(p, "package.json"))
+    const prodDependencies = { ...packageJson.dependencies, ...packageJson.optionalDependencies }
 
     const deps = { ...(tree.dependencies || {}), ...(tree.optionalDependencies || {}) }
-    const dependencies = Object.entries(deps).reduce<DependencyTree["dependencies"]>((acc, curr) => {
-      const [packageName, dependency] = curr
-
-      let isOptional: boolean
-      if (packageJson.dependencies?.[packageName]) {
-        isOptional = false
-      } else if (packageJson.optionalDependencies?.[packageName]) {
-        isOptional = true
-      } else {
-        return acc
-      }
-
-      try {
-        return {
-          ...acc,
-          [packageName]: this.extractProductionDependencyTree(dependency),
+    this.productionGraph[dependencyId] = { dependencies: [] }
+    const dependencies = Object.entries(deps)
+      .filter(([packageName, dependency]) => {
+        // First check if it's in production dependencies
+        if (!prodDependencies[packageName]) {
+          return false
         }
-      } catch (error) {
-        if (isOptional) {
-          return acc
+
+        // Then check if optional dependency path exists
+        if (packageJson.optionalDependencies && packageJson.optionalDependencies[packageName] && !fs.existsSync(dependency.path)) {
+          log.debug(null, `Optional dependency ${packageName}@${dependency.version} path doesn't exist: ${dependency.path}`)
+          return false
         }
-        throw error
-      }
-    }, {})
 
-    const { name, version, path: packagePath, workspaces } = tree
-    const depTree: DependencyTree = {
-      name,
-      version,
-      path: packagePath,
-      workspaces,
-      dependencies,
-      implicitDependenciesInjected: false,
+        return true
+      })
+      .map(([packageName, dependency]) => {
+        const childDependencyId = `${packageName}@${dependency.version}`
+        this.extractProductionDependencyGraph(dependency, childDependencyId)
+        return childDependencyId
+      })
+
+    this.productionGraph[dependencyId] = { dependencies }
+  }
+
+  protected collectAllDependencies(tree: PnpmDependency) {
+    // Collect regular dependencies
+    for (const [key, value] of Object.entries(tree.dependencies || {})) {
+      this.allDependencies.set(`${key}@${value.version}`, value)
+      this.collectAllDependencies(value)
+    }
+
+    // Collect optional dependencies if they exist
+    for (const [key, value] of Object.entries(tree.optionalDependencies || {})) {
+      this.allDependencies.set(`${key}@${value.version}`, value)
+      this.collectAllDependencies(value)
     }
-    return depTree
   }
 
   protected parseDependenciesTree(jsonBlob: string): PnpmDependency {
diff --git a/packages/app-builder-lib/src/node-module-collector/types.ts b/packages/app-builder-lib/src/node-module-collector/types.ts
@@ -12,15 +12,13 @@ export type ParsedDependencyTree = {
   readonly workspaces?: string[] // we only use this at root level
 }
 
-export interface DependencyTree extends Omit<Dependency<DependencyTree, DependencyTree>, "optionalDependencies"> {
-  readonly implicitDependenciesInjected: boolean
-}
-
 // Note: `PnpmDependency` and `NpmDependency` include the output of `JSON.parse(...)` of `pnpm list` and `npm list` respectively
 // This object has a TON of info - a majority, if not all, of each dependency's package.json
 // We extract only what we need when constructing DependencyTree in `extractProductionDependencyTree`
-// eslint-disable-next-line @typescript-eslint/no-empty-object-type
-export interface PnpmDependency extends Dependency<PnpmDependency, PnpmDependency> {}
+export interface PnpmDependency extends Dependency<PnpmDependency, PnpmDependency> {
+  readonly from: string
+}
+
 export interface NpmDependency extends Dependency<NpmDependency, string> {
   // implicit dependencies
   readonly _dependencies?: {
diff --git a/test/snapshots/HoistedNodeModuleTest.js.snap b/test/snapshots/HoistedNodeModuleTest.js.snap
diff --git a/test/src/HoistedNodeModuleTest.ts b/test/src/HoistedNodeModuleTest.ts

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"app-builder-lib": patch
 +---
++
 +fix: remove implicit dependencies handling