Skip to content

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 two 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: c52f08b

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
@mmaietta
Copy link
Collaborator

Fantastic work on this PR!

@mmaietta mmaietta mentioned this pull request Apr 10, 2025
@beyondkmp beyondkmp merged commit 8bd1a10 into electron-userland:master Apr 10, 2025
18 checks passed
@beyondkmp beyondkmp deleted the twoPackageJsons branch April 11, 2025 03:06
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