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

fix: remove implicit dependencies handling #9010

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

Conversation

beyondkmp
Copy link
Collaborator

@beyondkmp beyondkmp commented Apr 4, 2025

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 one tree lookup has been eliminated, there will be a certain improvement in performance.

Copy link

changeset-bot bot commented Apr 4, 2025

🦋 Changeset detected

Latest commit: 1fb9db1

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 marked this pull request as draft April 4, 2025 08:03
@beyondkmp beyondkmp marked this pull request as ready for review April 5, 2025 03:04
@beyondkmp beyondkmp requested a review from mmaietta April 5, 2025 03:05
@beyondkmp
Copy link
Collaborator Author

beyondkmp commented Apr 5, 2025

https://github.com/electron-userland/electron-builder/actions/runs/14277766371/job/40028363385

The HoistedNodeModuleTest did not run, and there are several others under this category that were also skipped. This might be caused by issues with parallel testing. @mmaietta When you have time, could you help take a look at this?

          - ArtifactPublisherTest,BuildTest,ExtraBuildTest,RepoSlugTest,binDownloadTest,configurationValidationTest,filenameUtilTest,filesTest,globTest,ignoreTest,macroExpanderTest,mainEntryTest,urlUtilTest,extraMetadataTest,linuxArchiveTest,linuxPackagerTest,HoistedNodeModuleTest,MemoLazyTest,HoistTest

@beyondkmp beyondkmp marked this pull request as draft April 8, 2025 08:46
@beyondkmp beyondkmp marked this pull request as ready for review April 8, 2025 10:25
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are two cases:

  1. two packages json(newly added)

  2. npm tar

    I manually compared the npm tar, and there are no issues. The previous collection algorithm had a problem, as it included some unnecessary packages.

@@ -7,15 +7,12 @@ export interface NodeModuleInfo {

export type ParsedDependencyTree = {
readonly name: string
readonly from: string // for pnpm
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this be more appropriate in PnpmDependency since that extends ParsedDependencyTree already?

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.

Required dependencies not included in app.asar
2 participants