Skip to content

fix: handle pnpm deduped dependencies correctly in node module collector - #9618

Merged
mmaietta merged 7 commits into
electron-userland:masterfrom
beyondkmp:fix/pnpm-deduped-dependencies
Feb 28, 2026
Merged

mmaietta merged 7 commits into
electron-userland:masterfrom
beyondkmp:fix/pnpm-deduped-dependencies

Conversation

@beyondkmp

Copy link
Copy Markdown
Contributor

fix #9603
Newer pnpm versions mark deduped dependencies with dedupedDependenciesCount instead of including nested dependency subtrees. Fix several bugs in the dedup handling:

  • Use tree.from instead of tree.name for dedup lookup key, since child dependency objects don't have a name field in pnpm's JSON output
  • Skip deduped entries before calling locatePackageVersion to avoid wasted filesystem lookups
  • Add dedup skip check to optionalDependencies loop (was only in dependencies)
  • Remove dead dedupedDependenciesCount: pkg?.dedupedDependenciesCount that always evaluated to undefined and overwrote the correct spread value
  • Make dedupedDependenciesCount optional in PnpmDependency type since older pnpm versions and non-deduped entries don't have this field

Newer pnpm versions mark deduped dependencies with `dedupedDependenciesCount`
instead of including nested dependency subtrees. Fix several bugs in the
dedup handling:

- Use `tree.from` instead of `tree.name` for dedup lookup key, since child
  dependency objects don't have a `name` field in pnpm's JSON output
- Skip deduped entries before calling `locatePackageVersion` to avoid wasted
  filesystem lookups
- Add dedup skip check to optionalDependencies loop (was only in dependencies)
- Remove dead `dedupedDependenciesCount: pkg?.dedupedDependenciesCount` that
  always evaluated to undefined and overwrote the correct spread value
- Make `dedupedDependenciesCount` optional in PnpmDependency type since older
  pnpm versions and non-deduped entries don't have this field

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Copilot AI review requested due to automatic review settings February 24, 2026 10:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes pnpm dependency tree parsing for newer pnpm versions where deduped dependencies are represented via dedupedDependenciesCount instead of nested subtrees, ensuring the node module collector can still build a correct production dependency graph.

Changes:

  • Add dedupedDependenciesCount?: number to PnpmDependency to support newer pnpm JSON output.
  • Resolve deduped pnpm dependency nodes by looking up the “real” dependency in allDependencies using from/name as the key.
  • Skip deduped dependency entries during dependency collection (both dependencies and optionalDependencies) to avoid wasted filesystem lookups.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/app-builder-lib/src/node-module-collector/types.ts Updates pnpm dependency typing to include optional dedupedDependenciesCount.
packages/app-builder-lib/src/node-module-collector/pnpmNodeModulesCollector.ts Adds dedup handling + avoids unnecessary locatePackageVersion calls for deduped entries in both dependency loops.
Comments suppressed due to low confidence (1)

packages/app-builder-lib/src/node-module-collector/pnpmNodeModulesCollector.ts:30

  • log.error here is using a templated string, whereas other errors in this subsystem use structured fields (e.g., nodeModulesCollector.ts:142, traversalNodeModulesCollector.ts:116). Consider logging an object with dependencyId, depKey, and version to make build logs easier to filter/diagnose, and keep the error message constant.
    if (tree.dedupedDependenciesCount && tree.dedupedDependenciesCount > 0) {
      const depKey = tree.from || tree.name
      const realDep = this.allDependencies.get(`${depKey}@${tree.version}`)
      if (realDep) {
        tree = realDep
      } else {
        log.error(`Could not find real dependency for ${depKey}@${tree.version}`)
        return
      }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mmaietta mmaietta left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @beyondkmp for putting this together!

@changeset-bot

changeset-bot Bot commented Feb 25, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0aec8dd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@beyondkmp
beyondkmp requested a review from mmaietta February 25, 2026 02:24

@diogotcorreia diogotcorreia left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick, feel free to ignore

@t3chguy

t3chguy commented Feb 27, 2026

Copy link
Copy Markdown
Contributor

@mmaietta mmaietta left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Builds broken after upgrading pnpm 10.28.2->10.29.3/10.30.0

5 participants