Repository navigation
Remove two large commented-out blocks from CommonJS.ts and child_process.ts - #39916
Conversation
…ess.ts Both were dead: a ~130-line legacy loadEsmIntoCjs implementation kept inside a block comment after the module-loader rewrite, and a ~130-line commented-out copy of Node's internal/errors registration helpers in child_process.ts. They are stripped by the builtin bundler anyway; this just removes them from the source. No-Verification-Needed: comment-only deletion
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Limit details: You’ve used the included review currently available. Your 62 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 2:50 AM PT - Aug 21st, 2026
✅ @dylan-conway, your commit 68069bf81d8d5604b603aae82649fcfa984692fa passed in 🧪 To try this PR locally: bunx bun-pr 39916That installs a local version of the PR into your bun-39916 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure comment deletion, no live code touched.
What was reviewed:
- Confirmed every removed line in
CommonJS.tsis inside the/* ... */block comment and every removed line inchild_process.tsis a//line comment. - Verified the surrounding live lines (
$esmLoadSyncreturn /$visibility = "Private"andgenericNodeError/ERR_UNKNOWN_SIGNAL) are unchanged in the diff context. - Neither file is under CODEOWNERS.
Extended reasoning...
Overview
This PR deletes two large commented-out blocks from builtin JS sources: a ~130-line /* ... */ block in src/js/builtins/CommonJS.ts (a legacy loadEsmIntoCjs implementation that referenced the old JS-side JSModuleLoader machinery — Loader.registry, $setStateToMax, etc. — which no longer exists) and a ~130-line run of // comments in src/js/node/child_process.ts (a dead copy of Node's internal/errors E() / makeNodeErrorWithCode registration helpers). No executable line is added, removed, or modified.
Security risks
None. Comment-only removal; the builtin bundler already stripped these comments, so the emitted module bytes are unchanged.
Level of scrutiny
Minimal. I walked the diff line-by-line to confirm every - line is either inside the block comment (CommonJS.ts) or begins with // (child_process.ts), and that the diff context lines on both sides of each removed hunk are intact — the live loadEsmIntoCjs wrapper and the $visibility = "Private" directive above requireESM in CommonJS.ts, and genericNodeError / ERR_UNKNOWN_SIGNAL in child_process.ts. There is no risk of an accidentally-uncommented line or a deleted live statement.
Other factors
Neither file is CODEOWNERS-gated. This is exactly the "delete dead code in the same PR that makes it dead" cleanup the repo review guidelines encourage; the removed CommonJS block explicitly documented itself as a dead legacy implementation. No tests are needed because there is no behavioral change to test.
|
Adopted in #39926. The commit is unchanged, rebased onto current main. I checked that the generated output (WebCoreJSBuiltins.h, InternalModuleRegistryConstants.bin) is identical with and without the change, in both debug and release codegen modes. |
What does this PR do?
Deletes two large commented-out blocks from the builtin JS sources:
src/js/builtins/CommonJS.ts: a ~130-line legacyloadEsmIntoCjsimplementation kept inside a block comment since the module-loader rewrite (it referencedLoader.registry,$setStateToMax, etc., which no longer exist).src/js/node/child_process.ts: a ~130-line commented-out copy of Node'sinternal/errorsregistration helpers.Comment-only; the builtin bundler already stripped both, so there is no change to the emitted modules.
How did you verify your code works?
Comment-only deletion; diff context checked to confirm no live line was touched.