feat(app-builder-lib): exclude default production deps from node_modules instead of erroring (BREAKING) - #9994
Conversation
…les instead of erroring (BREAKING)
🦋 Changeset detectedLatest commit: 185d1ae The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
|
@liamcmitchell thanks for the contribution! Mind if I/claude push some updates to this PR? Some identified gaps I'd like to discuss (or that I can push changes to cover): Blocking --- the exclusion defeats the #9945 collection validation.
Fix: make validation exclusion-aware --- the collector already returns Should-fix:
Nits: exclusion log reports names but diffs name@version, so it can claim Test gap: coverage is all unit-level against the traversal collector with a mocked packager. No pack-level test asserting the final app actually omits |
mmaietta
left a comment
There was a problem hiding this comment.
Marking as Request Changes to discuss the previous comment 🙂
* origin/master: feat(updater): fix manifest sha512 hash-encoding sniffing, add opt-in Linux package-signature verification (electron-userland#9990) fix: don't empty locales dir when electronLanguages uses bare language codes (electron-userland#10007) fix: extract .tar.7z snap template archives correctly (electron-userland#10003)
Thanks for the review! Feel free to push changes however you see fit.
I pushed a change to mark nodes as excluded instead of removing them completely. This means the full tree can be used for collection validation as before. The nodes marked The cpu/os exclusion could use the same mechanism but I've left it as-is.
Changed the algorithm to only exclude root deps, not nested.
When I checked, both those projects seemed abandoned so I thought it would be a good time to clean them up. Feel free to re-add.
That flag is undocumented and the commits/issues I found referencing it are very old. Thought it made sense to remove in a breaking change. I would be surprised if a single user is using this.
Now that only top-level deps are excluded I think it makes sense to log just the name. I don't think logging excluded transitive deps and/or versions are helpful.
Did the previous hard-coded list catch aliases?
Not sure how to better keep that in sync
Disabling is not a common case. Current doc
Let me know if and what coverage you want from me. |
…usion Close the ignoredProductionDependencies test gaps: - pack-level assertPack test: an app declaring electron (default-ignored) and ms in dependencies packs with ms bundled in app.asar and electron omitted, exercising the real npm collector end-to-end - exclusion-aware validation: an app whose every external production dep is ignored still yields a successful collection (no spurious "no node modules returned" warning, exclusion summary still logged), and a monorepo whose only non-workspace dep is ignored still validates - npm collector graph ids: exclusion marking through canned npm list trees, pinning down that npm aliases match by alias key, not by the underlying package name - collectionMatchesAppDependencies counts excluded modules as validation markers (issue electron-userland#9945 interaction) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaKXX4mAESjjtvTYCgZtaa
…null semantics The exclusion summary now records the full name@version graph id — like the other logSummary buckets — so the log stays truthful when another version of the same name still ships via a kept dependency (previously it could claim `debug` was excluded while `debug@3` remained bundled). Also document in the ignoredProductionDependencies jsdoc (source of truth for scheme.json): - matching is by the declared dependency name, so npm aliases are matched by their alias key, never the underlying package name - only app-declared dependencies are eligible for exclusion, plus the MODULE_NOT_FOUND footgun for kept packages that require() an excluded name without declaring it - null (or omitted) applies the default list; [] disables exclusion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaKXX4mAESjjtvTYCgZtaa
Move DEFAULT_IGNORED_PRODUCTION_DEPENDENCIES next to the ignoredProductionDependencies option in configuration.ts so the code constant, the jsdoc @default, and the generated scheme.json live in one place, add a test pinning the generated schema default to the constant, and regenerate scheme.json to pick up the new jsdoc wording. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaKXX4mAESjjtvTYCgZtaa
Cover every breaking change this PR introduces: - ALLOW_ELECTRON_BUILDER_AS_PRODUCTION_DEPENDENCY: no longer presented as impact-free — describe the behavior flip (the var used to make electron-builder bundle; now the package is silently excluded by default) and how to bundle it again via ignoredProductionDependencies - electron-prebuilt / electron-rebuild: document that the v26 hard error is removed WITHOUT a default exclusion (they now ship if declared, electron-prebuilt dragging a full Electron binary along), that electron-nightly was never guarded, why the guard is dropped in a major (both packages long deprecated), and what users should do - exclusion semantics: only app-declared dependencies are eligible, exclusive transitive subtrees go with them, and a kept package that require()s an excluded name without declaring it crashes with MODULE_NOT_FOUND; npm aliases match by alias key - update the at-a-glance table row and the v26-to-v27 walkthrough checklists accordingly Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaKXX4mAESjjtvTYCgZtaa
|
Thanks for the replies! Added a few commits to expand test coverage, log the excluded deps w/ version, regenerated the scheme.json to sync schema/jsdoc/code, and updated breaking-changes doc for the removed items (I'm aligned with your removals of them for v27) |
I want to install packages like
electronandreactas production deps for other tooling (SBOM generation, license/vulnerability checking). I want production dep to mean "shipped with my application".electron-buildercurrently forces production dep to mean "copy this into node_modules in my application".electronas a prod dep fails to build with error⨯ Package "electron" is only allowed in "devDependencies". Please remove it from the "dependencies" section in your package.json.reactas a prod dep and bundled using vite/electron produces an unneeded copy in the ASARThis PR replaces the previous hard-coded error with a configurable ignore list, making the prod/dev definition more flexible.
The ignore list takes precedence over files matching. Ignored modules and their exclusive dependencies are removed before file matching.
I'm hoping this can get into the upcoming v27 so I shortened the default list to just
electronandelectron-builderassuming the others on the prev list are no longer needed.In a bundling world, an
externalswhitelist is probably more useful than an ignore blacklist but I figured this as implemented is a more manageable change for now.Tested on a local project using the
yalcsymlinks as described in CONTRIBUTING.md:Modules in app.asar were as expected.