Skip to content

refactor: simplify YarnNodeModulesCollector by extending NpmNodeModulesCollector#9506

Merged
mmaietta merged 16 commits into
electron-userland:masterfrom
beyondkmp:refactor/simplify-yarn-collector
Jan 31, 2026
Merged

refactor: simplify YarnNodeModulesCollector by extending NpmNodeModulesCollector#9506
mmaietta merged 16 commits into
electron-userland:masterfrom
beyondkmp:refactor/simplify-yarn-collector

Conversation

@beyondkmp
Copy link
Copy Markdown
Contributor

Yarn Classic (v1) produces a hoisted node_modules structure similar to npm. Instead of implementing custom NDJSON parsing logic, we now leverage NpmNodeModulesCollector's existing implementation using npm list command.

This follows the same pattern as YarnBerryNodeModulesCollector.

Changes:

…esCollector

Yarn Classic (v1) produces a hoisted node_modules structure similar to npm.
Instead of implementing custom NDJSON parsing logic, we now leverage
NpmNodeModulesCollector's existing implementation using npm list command.

This follows the same pattern as YarnBerryNodeModulesCollector.

Changes:
- Reduce yarnNodeModulesCollector.ts from ~268 lines to ~25 lines
- Remove unused YarnDependency and YarnBerryDependency types

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 16, 2026 05:56
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jan 16, 2026

🦋 Changeset detected

Latest commit: 06ec44c

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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors YarnNodeModulesCollector to extend NpmNodeModulesCollector instead of implementing custom NDJSON parsing logic, recognizing that Yarn Classic (v1) produces a hoisted node_modules structure similar to npm. This follows the pattern established by YarnBerryNodeModulesCollector and is a follow-up to PR #9443 which enabled npm list to work on yarn berry and bun project setups.

Changes:

  • Simplified YarnNodeModulesCollector by extending NpmNodeModulesCollector and delegating to npm list command
  • Removed unused YarnDependency and YarnBerryDependency type definitions from types.ts
  • Reduced yarnNodeModulesCollector.ts from ~268 lines to ~25 lines by removing custom parsing logic

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/app-builder-lib/src/node-module-collector/yarnNodeModulesCollector.ts Refactored to extend NpmNodeModulesCollector with minimal overrides for manager, lockfile, isHoisted, getDependenciesTree, and isProdDependency
packages/app-builder-lib/src/node-module-collector/types.ts Removed unused YarnDependency and YarnBerryDependency type definitions

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

Comment thread packages/app-builder-lib/src/node-module-collector/yarnNodeModulesCollector.ts Outdated
@mmaietta
Copy link
Copy Markdown
Collaborator

If this works consistently, then I'm going to be so dam sad and happy at the same time. Sad because that was a ton of dev days put into it; happy because of the new implementation's simplicity haha.

@mmaietta
Copy link
Copy Markdown
Collaborator

Big snapshot failures. Some could be false positives, hard to say since the snapshot diff is so big. This does stick out to me as a red flag, though, as it'll break the app at runtime

-                 "bindings": {
-                   "files": {
-                     "LICENSE.md": {
-                       "size": "<size>",
-                       "unpacked": true,
-                     },
-                     "bindings.js": {
-                       "size": "<size>",
-                       "unpacked": true,
-                     },
-                     "package.json": {
-                       "size": "<size>",
-                       "unpacked": true,
-                     },
-                   },
-                 },

@beyondkmp
Copy link
Copy Markdown
Contributor Author

If this works consistently, then I'm going to be so dam sad and happy at the same time. Sad because that was a ton of dev days put into it; happy because of the new implementation's simplicity haha.

This has always been working fine. The node modules for yarn 1 are the same as npm, and electron-builder always been using npm list when doing this before. I'm not sure why you change it to yarn list.

@beyondkmp
Copy link
Copy Markdown
Contributor Author

beyondkmp commented Jan 17, 2026

Big snapshot failures. Some could be false positives, hard to say since the snapshot diff is so big. This does stick out to me as a red flag, though, as it'll break the app at runtime

-                 "bindings": {
-                   "files": {
-                     "LICENSE.md": {
-                       "size": "<size>",
-                       "unpacked": true,
-                     },
-                     "bindings.js": {
-                       "size": "<size>",
-                       "unpacked": true,
-                     },
-                     "package.json": {
-                       "size": "<size>",
-                       "unpacked": true,
-                     },
-                   },
-                 },
 "node-mac-permissions": {
          "files": {
            ".prettierrc": {
              "size": "<size>",
              "unpacked": true,
            },
            "LICENSE": {
              "size": "<size>",
              "unpacked": true,
            },
            "build": {
              "files": {
                "Release": {
                  "files": {
                    "permissions.node": {
                      "size": "<size>",
                      "unpacked": true,
                    },
                  },
                },
              },
            },
            "index.js": {
              "size": "<size>",
              "unpacked": true,
            },
            "node_modules": {
              "files": {
                "bindings": {
                  "files": {
                    "LICENSE.md": {
                      "size": "<size>",
                      "unpacked": true,
                    },
                    "bindings.js": {
                      "size": "<size>",
                      "unpacked": true,
                    },
                    "package.json": {
                      "size": "<size>",
                      "unpacked": true,
                    },
                  },
                },
                "node-addon-api": {
                  "files": {z

The snapshot inside node-mac-permissions already has snapshots of these two packages

@beyondkmp
Copy link
Copy Markdown
Contributor Author

I manually checked it, and the previous snapshot had duplicate bindings and node-addon-api. Using npm list and then hoisting eliminates these duplicate packages.

@mmaietta mmaietta merged commit 1b113b7 into electron-userland:master Jan 31, 2026
30 checks passed
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.

3 participants