fix: minimize production dependency tree by MarshallOfSound · Pull Request #1244 · electron/rebuild

MarshallOfSound · 2026-03-24T22:57:12Z

Summary

Shrinks the dependency tree by replacing several packages with small inline implementations or Node.js builtins:

Removed	Replacement
`debug`	`src/debug.ts` — minimal logger gated on `DEBUG`
`detect-libc`	`src/detect-libc.ts` — `process.report`-based glibc/musl check
`got`	native `fetch`
`graceful-fs`	`node:fs` / `node:fs/promises`
`ora`	`src/spinner.ts` — tiny TTY spinner via `util.styleText`
`semver`, `tar`, `yargs`	node builtins / simpler equivalents

Also bumps node-gyp to ^12.2.0 and drops the now-unused src/promisifiedGracefulFs.ts wrapper along with associated @types/* devDeps.

Test plan

yarn test passes
CLI still parses flags correctly and renders spinner/success/fail states
Rebuild against a native module on Linux (glibc + musl) to validate libc detection
Cache snapshot/apply roundtrip still works with node:fs

socket-security · 2026-03-24T22:59:49Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@types/debug@4.1.7 ⏵ 4.1.13			^-2

View full report

dsanders11

If the goal is just to drop "heavy" dependencies as in the title, I don't like several of these changes.

debug is small and only has one transitive dependency, but we're losing the richer functionality and consistency across our other packages that use debug
detect-libc I could take or leave, but it's zero dependency
ora is heavy and we should drop it, but there's zero dependency alternatives rather than rolling our own. We have multiple packages that currently use spinners (most use ora), we should migrate them all to the same alternative rather than having each package roll its own implementation that will drift. Or even create @electron/spinner, if we're insistent on cutting third-party dependencies.
We specifically kept graceful-fs when we did the Node 22 migration, we should not drop it here.

MarshallOfSound · 2026-03-25T00:15:31Z

Apologies @dsanders11 this was intended to be a draft PR, twas not ready for prime time

If the goal is just to drop "heavy" dependencies as in the title, I don't like several of these changes.

I'll rephrase the title, the objective is to drop as many dependencies as is reasonable regardless of weight

debug is small and only has one transitive dependency, but we're losing the richer functionality and consistency across our other packages that use debug

Agree on this, was chatting with @erickzhao we have a funky thing in here that confused some stuff. debugLog.ts existed in history and was just "brought back" to remove the dependency.

detect-libc I could take or leave, but it's zero dependency

It's a zero-dependency thing that was trivial to re-implement and avoid a supply-chain risk 🤷 Seems like a free win to inline it

ora is heavy and we should drop it, but there's zero dependency alternatives rather than rolling our own. We have multiple packages that currently use spinners (most use ora), we should migrate them all to the same alternative rather than having each package roll its own implementation that will drift. Or even create @electron/spinner, if we're insistent on cutting third-party dependencies.

Yeah this one I'm not sure about, tbh I think we should just remove the spinner. I don't think anyone actually uses the CLI anymore (it's just the hook for use in packager or forge).

We specifically kept graceful-fs when we did the Node 22 migration, we should not drop it here.

I didn't have that context but on discussion with @erickzhao I don't see a reason to keep it in this module? The only heavy operations that could possible trigger EMFILE (as I understand it the only reason to use this module) are in the cache implementation and whilst both are unbounded neither are going to operate on enough files to trip that behavior.

There are zero historical reports (even predating fs-extra) of EMFILE in this repo: ref and I don't think we should keep a dependency for basic fs operations out of FUD 🤔

dsanders11 · 2026-03-25T02:14:46Z

Yeah this one I'm not sure about, tbh I think we should just remove the spinner. I don't think anyone actually uses the CLI anymore (it's just the hook for use in packager or forge).

I'm fine with removing the spinner, and I'd prefer removing it all together over rolling our own here.

I didn't have that context but on discussion with @erickzhao I don't see a reason to keep it in this module? The only heavy operations that could possible trigger EMFILE (as I understand it the only reason to use this module) are in the cache implementation and whilst both are unbounded neither are going to operate on enough files to trip that behavior.

There are zero historical reports (even predating fs-extra) of EMFILE in this repo: ref and I don't think we should keep a dependency for basic fs operations out of FUD 🤔

A bit more context here. For the usage in this particular repo we might be safe to remove it, but I do have a memory (can't find PR at the moment) of us getting bit on one package during the Node 22 transition where we restored it, which was part of the reason we opted to leave it in repos during that transition.

Also, graceful-fs is a dependency of node-gyp so it's not like we're fully getting rid of that dependency by removing our top-level usage of it. I wouldn't describe it as FUD, just being more conservative to prevent unintended breakage.

MarshallOfSound · 2026-03-25T05:01:02Z

Also, graceful-fs is a dependency of node-gyp so it's not like we're fully getting rid of that dependency by removing our top-level usage of it. I wouldn't describe it as FUD, just being more conservative to prevent unintended breakage.

Yeah I saw that too, minimizing the tree is still important too me. If we have strong reason to believe something will break as a result of removing it I'm happy to put it back (it's also trivial to put back) but right now it seems like a branch we can snip off relatively easily

I cleaned up the other removals and such, should be in a better spot now

Replace several third-party dependencies with small inline implementations or Node.js builtins: - debug → minimal logger gated on DEBUG env var - detect-libc → process.report-based glibc/musl detection - got → native fetch - graceful-fs → node:fs (drop promisifiedGracefulFs wrapper) - ora → tiny TTY spinner using util.styleText - semver, tar, yargs → node builtins / simpler equivalents Also bumps node-gyp to ^12.2.0.

- Restore `debug` dependency for consistency with other @electron packages - Remove spinner entirely instead of inlining (CLI is primarily a packager/forge hook, not used directly) - Add `modules-found` lifecycle event so CLI can print all module names in one line before building starts

- Remove src/debug.ts wrapper, inline debug() per-file (restores the pattern on main, allows per-file namespace suffixes later) - Add TSDoc to detectLibcFamily documenting return values - Drop the copyright line from CLI help output

erickzhao · 2026-04-15T23:07:48Z

-    cwd: clangDirPath,
-  });
+  d('extracting clang');
+  await spawn('tar', ['-xf', tarPath, '-C', clangDirPath], { stdio: 'ignore' });


just noting here that this moves us from node-tar to relying on the system tar specifically for this one call, which is specifically used for the advanced useElectronClang option.

dsanders11 · 2026-04-17T01:13:52Z

Not gonna block on it, but we really shouldn't have these silly changes in these Claude PRs. This is literally just moving an import around to a different spot, one that makes less sense. Hopefully as part of the oxc.rs we can add more linting rules that enforce this stuff.

dsanders11 · 2026-04-17T01:25:19Z

Gonna need an explanation for why these changes are in this PR and why there's a new 'modules-found' lifecycle event being emitted.

It looks like it's probably related to dropping ora, but I don't think we should have this kind of output change in this PR, as it's unrelated to the stated purpose of the PR.

Gonna need an explanation for why these changes are in this PR and why there's a new 'modules-found' lifecycle event being emitted.

This isn't new, it was dropped ages ago in a bad refactor and this adds it back

This isn't new, it was dropped ages ago in a bad refactor and this adds it back

I can't find any evidence of that, at least under the name 'modules-found'. Not gonna block on this, but not sure this is true.

dsanders11 · 2026-04-21T17:28:03Z

This isn't new, it was dropped ages ago in a bad refactor and this adds it back

I can't find any evidence of that, at least under the name 'modules-found'. Not gonna block on this, but not sure this is true.

electron-npm-package-publisher · 2026-04-21T18:00:15Z

🎉 This PR is included in version 4.0.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

MarshallOfSound requested a review from a team as a code owner March 24, 2026 22:57

dsanders11 requested changes Mar 25, 2026

View reviewed changes

dsanders11 mentioned this pull request Mar 25, 2026

refactor: replace got with native fetch API #1243

Merged

MarshallOfSound force-pushed the sam/reduce-dependencies branch from b4ebcdb to f43fb14 Compare March 25, 2026 00:20

MarshallOfSound changed the title ~~refactor: drop heavy dependencies in favor of node builtins~~ refactor: minimize production dependency tree Mar 25, 2026

erickzhao mentioned this pull request Mar 30, 2026

fix: require node-gyp@^12.2.0 #1245

Closed

erickzhao reviewed Mar 30, 2026

View reviewed changes

Comment thread src/debug.ts Outdated

Comment thread src/detect-libc.ts

Comment thread src/module-type/node-gyp/node-gyp.ts

Comment thread src/cli.ts Outdated

MarshallOfSound added 3 commits April 15, 2026 12:29

refactor: address review feedback

a07793e

- Remove src/debug.ts wrapper, inline debug() per-file (restores the pattern on main, allows per-file namespace suffixes later) - Add TSDoc to detectLibcFamily documenting return values - Drop the copyright line from CLI help output

MarshallOfSound force-pushed the sam/reduce-dependencies branch from 0ef17c7 to a07793e Compare April 15, 2026 19:40

fix: restore direct debug import in module-setup test helper

9273d1e

erickzhao approved these changes Apr 15, 2026

View reviewed changes

dsanders11 requested changes Apr 17, 2026

View reviewed changes

dsanders11 approved these changes Apr 21, 2026

View reviewed changes

MarshallOfSound changed the title ~~refactor: minimize production dependency tree~~ fix: minimize production dependency tree Apr 21, 2026

MarshallOfSound merged commit 21ceb5b into main Apr 21, 2026
8 checks passed

MarshallOfSound deleted the sam/reduce-dependencies branch April 21, 2026 17:50

electron-npm-package-publisher Bot added the released label Apr 21, 2026

Conversation

MarshallOfSound commented Mar 24, 2026

Summary

Test plan

Uh oh!

socket-security Bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsanders11 left a comment

Choose a reason for hiding this comment

Uh oh!

MarshallOfSound commented Mar 25, 2026

Uh oh!

dsanders11 commented Mar 25, 2026

Uh oh!

MarshallOfSound commented Mar 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erickzhao Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

dsanders11 Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

dsanders11 Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

dsanders11 Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

MarshallOfSound Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

dsanders11 Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

dsanders11 Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

electron-npm-package-publisher Bot commented Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

socket-security Bot commented Mar 24, 2026 •

edited

Loading