Conversation
|
Status: adopts #39916 at the request of @dylan-conway. The diff is the same commit (author unchanged), based on main at 4448a2e. Verified by a byte for byte compare of the bundle-modules.ts output on main and on this branch, in both codegen modes. No review threads are open. CI: build 102542 passed 180 of 181 jobs. The one red test, test/js/workerd/html-rewriter-leak.test.ts on x64-asan (a UBSan null JSCell member call inside HTMLRewriter), is not reachable from a comment deletion that leaves the generated output identical. It is reported for main triage separately. The head commit was re-pushed once with an identical tree (59a90bc) to run CI again. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesThe PR removes obsolete commented-out implementations and scaffolding. Active runtime behavior and exported APIs remain unchanged. Obsolete code removal
Merge Risk: ⚪ Minimal · up to This change only removes obsolete comments and does not alter generated output or runtime behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure comment/blank-line deletion with no behavioral change.
What was reviewed:
- Confirmed every removed line is inside a
/* ... */block (CommonJS.ts) or a//line/blank (child_process.ts); zero lines added. - The live
loadEsmIntoCjsand$ERR_INVALID_ARG_TYPEpaths that supersede the commented code remain intact in the post-change files. - Codegen strips comments before emitting
WebCoreJSBuiltins.*/InternalModuleRegistryConstants.bin, so the built binary is unchanged (author verified withdiff -ron both debug modes).
Extended reasoning...
Overview
This PR deletes two large commented-out code blocks: a ~130-line /* ... */ block in src/js/builtins/CommonJS.ts containing the legacy loadEsmIntoCjs implementation (dead since the module loader moved to C++), and a ~130-line run of // comments in src/js/node/child_process.ts containing a never-used copy of Node's E()/makeNodeErrorWithCode() helpers. The diff is -263/+0. Every removed line is a comment or blank; no executable code, exports, or declarations are touched.
Security risks
None. Comments are stripped by bundle-functions.ts and bundle-modules.ts before reaching the compiled binary, so this change cannot alter runtime behavior in any way. No auth, crypto, permissions, or user-input handling is involved.
Level of scrutiny
Minimal. This is the canonical low-risk change: mechanical dead-comment removal with a verifiable claim that the generated artifacts are byte-identical. The author ran the codegen on both branches with --debug=ON and --debug=OFF and confirmed diff -r is empty. Per the repo's own review guidance ("Delete dead code in the same PR that makes it dead"), this is exactly the kind of cleanup that should land without friction.
Other factors
No prior reviews or outstanding comments on the PR. No test is added, which is correct — a comment-only deletion has no behavior to test, and the repo guidance's "every behavioral change ships a test" rule does not apply to non-behavioral changes. The bug hunting system found no issues. The two hunks are extracted from a larger dead-code PR (#39618) to land independently, which reduces review surface.
|
Updated 3:22 AM PT - Aug 21st, 2026
✅ @dylan-conway, your commit 59a90bce5a9e2ff66f10d7e37404cbe7d1f1a537 passed in 🧪 To try this PR locally: bunx bun-pr 39926That installs a local version of the PR into your bun-39926 --bun |
…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 (cherry picked from commit 68069bf)
d785447 to
59a90bc
Compare
There was a problem hiding this comment.
LGTM — pure comment deletion, no code changed.
What was reviewed:
- Confirmed every removed line in both hunks is inside a
/* */block comment, a//line comment, or blank; zero lines added. - The live
loadEsmIntoCjs($esmLoadSync) andERR_UNKNOWN_SIGNALthat follow the deleted blocks are intact in the post-diff files. - Checked that no identifier inside the deleted comments (e.g.
loadEsmIntoCjs__dead,makeNodeErrorWithCode) is referenced elsewhere — it was all inert.
Extended reasoning...
Overview
This PR deletes two large commented-out code blocks: a ~130-line /* ... */ block in src/js/builtins/CommonJS.ts containing the obsolete loadEsmIntoCjs__dead implementation (which relied on the removed JS-side Loader.registry/$setStateToMax/parseModule machinery), and a ~130-line run of // comments in src/js/node/child_process.ts containing a dead copy of Node's E()/makeNodeErrorWithCode() error-registration scaffolding. The diff is -263/+0; no executable line is touched.
Security risks
None. Comments are stripped by bundle-functions.ts and bundle-modules.ts before reaching the generated WebCoreJSBuiltins.h / InternalModuleRegistryConstants.bin, so the shipped binary is byte-identical (the PR description confirms this via diff -r on codegen output in both debug modes). There is no attack surface change.
Level of scrutiny
Minimal. This is the canonical "delete dead code" case the repo's review guidelines explicitly require (dead comments narrating superseded implementations). Every removed line was verified to be a comment or blank — there is no possibility of behavioral change, and no test is expected for a comment-only deletion.
Other factors
The commit is authored by a maintainer (dylan-conway) and adopted from #39916. CI build 102542 passed 180/181 jobs; the single failure (html-rewriter-leak.test.ts UBSan on x64-asan) is in unrelated native HTMLRewriter code and cannot be reached by a change that leaves generated output identical. No prior review threads are open and I have not reviewed this PR before.
2 participants
Adopts #39916 by @dylan-conway. The commit is his, rebased onto current main.
Problem
src/js/builtins/CommonJS.ts:184holds a 130 line block comment with the oldloadEsmIntoCjsimplementation. It usedLoader.registry,$setStateToMaxandloader.parseModule. None of these exist since the module loader moved to C++. The liveloadEsmIntoCjsabove it is one call to$esmLoadSync.src/js/node/child_process.ts:1955holds a 130 line run of//comments. It is a copy of theE()andmakeNodeErrorWithCode()helpers from Node'sinternal/errors. The file uses the native$ERR_INVALID_ARG_TYPEinstead.Fix
child_process.tseach removed line starts with//or is blank. InCommonJS.tsthe removed lines are one/* ... */block and the blank line after it.src/codegen/bundle-modules.tson main and on this branch, with--debug=OFFand with--debug=ON, into a directory with the same name.diff -rof thecodegen/andjs/output trees is empty in both modes. This coversWebCoreJSBuiltins.{h,cpp}andInternalModuleRegistryConstants.bin.Background
src/js/builtins/*.tsare JSC builtin functions.src/codegen/bundle-functions.tsturns each exported function into a string table entry inWebCoreJSBuiltins.h.src/js/node/*.tsare thenode:*modules.src/codegen/bundle-modules.tsbundles them intoInternalModuleRegistryConstants.bin. The bundler drops comments, so a comment only change cannot reach the binary.