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

Open
wants to merge 112 commits into
base: master
Choose a base branch
from

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 beyondkmp marked this pull request as draft October 15, 2024 06:54
@beyondkmp beyondkmp marked this pull request as ready for review October 16, 2024 09:49
@beyondkmp
Copy link
Collaborator Author

@mmaietta Please help review again.

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.

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

@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
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