Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: switch app-builder-bin to node-module-collector to get all production node modules #8571

Merged
merged 129 commits into from
Feb 4, 2025

Conversation

beyondkmp
Copy link
Collaborator

@beyondkmp beyondkmp commented Oct 8, 2024

change app-builder-bin to node-module-collector(typescript)

Design

  1. Get node modules tree from "npm list"(npm & yarn) or "pnpm list"
  2. Transform the tree to dependency graph
  3. Transform the graph to hoisted tree
  4. Hoisted tree to node modules array

Support

  • npm
  • pnpm
  • pnpm with hosited
  • yarn1
  • yarn berry(with node-modules)

Performance vs app-builder-bin

This is an IO-intensive task, primarily involving reading files and traversing the entire dependency tree. Node.js is capable of handling such IO-intensive tasks without issues.

Based on my personal testing, there's essentially no difference in performance

Advantages

  1. Perfectly supports pnpm
  2. Optimize packages in the node_modules within the asar file
    unlike the previous app-builder-bin which only searched for all node_modules without optimizing. For example, if a module has one version in the dev node_modules dependencies, another in the root node_modules, and yet another version with multiple dependencies in the production node_modules, it results in multiple duplicate modules in the production node_modules. Now, hoisting is applied in such situations, reducing these duplicate packages.

@beyondkmp
Copy link
Collaborator Author

Note: There's some util functions already existing in builder-util/src/util and builder-util/src/fs that we could leverage to reduce duplicated code and improve debug logging

Excellent suggestions, all changes have been implemented.

Copy link
Collaborator

@mmaietta mmaietta left a comment

Choose a reason for hiding this comment

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

This is awesome, great work!

@mmaietta mmaietta self-requested a review October 25, 2024 16:16
@beyondkmp beyondkmp merged commit 04be569 into electron-userland:master Feb 4, 2025
15 checks passed
@beyondkmp beyondkmp deleted the nodeModulesCollectorNew branch February 4, 2025 03:29
mmaietta added a commit that referenced this pull request Feb 4, 2025
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.

2 participants