node:module: implement stripTypeScriptTypes#32206
Conversation
|
Updated 1:48 AM PT - Jun 19th, 2026
❌ @robobun, your commit 524b763 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32206That installs a local version of the PR into your bun-32206 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements node:module.stripTypeScriptTypes with parser/AST runtime-semantics tracking, new TypeScript SyntaxError codes, a Rust FFI + C++ host wrapper that validate options and forward to a one-shot Transpiler, and comprehensive tests including inline sourcemap emission. ChangesTypeScript runtime semantics tracking and node:module stripTypeScriptTypes API
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/NodeModuleModule.rs`:
- Around line 334-363: The current logic only emits the sourceURL trailer in the
else-if branch so when source_map is true the sourceURL is skipped; modify the
block that handles source_map in NodeModuleModule.rs (the conditional using
source_map, json, source_url_utf8, out and map_vlq) to also append the `//#
sourceURL=` comment when source_url_utf8.slice() is non-empty—i.e., after
writing the inline base64 source map to out, check
source_url_utf8.slice().is_empty() and if not, extend out with "\n\n//#
sourceURL=" and the source_url_utf8 bytes (same semantics as the existing else
branch) so both sourceMappingURL and sourceURL are emitted for combined options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb880893-e5b6-4bdf-b8c4-2a649f4dab9c
📒 Files selected for processing (11)
src/ast/ast_result.rssrc/ast/lib.rssrc/js_parser/p.rssrc/js_parser/parse/parse_stmt.rssrc/js_parser/parse/parse_typescript.rssrc/js_parser/visit/mod.rssrc/jsc/ErrorCode.rssrc/jsc/NodeModuleModule.rssrc/jsc/bindings/ErrorCode.tssrc/jsc/modules/NodeModuleModule.cpptest/js/node/module/strip-typescript-types.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/NodeModuleModule.rs (2)
225-231:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve a leading hashbang in the returned source.
code_utf8still contains the original bytes, but the function returns only the printer output plus trailers, so#!/usr/bin/env bunis dropped on every non-empty input. That turns executable TypeScript entrypoints into non-executable output and breaks the Node-compat contract this API is aiming for. Please carry the shebang through and add a regression case for it. Based on PR objectives, this API is intended to match Node'sstripTypeScriptTypesbehavior.Also applies to: 301-331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/NodeModuleModule.rs` around lines 225 - 231, The current code creates code_utf8 and a bun_ast::Source but drops a leading shebang (#!...) when returning printed output; update the logic around code_utf8 and the printer output in NodeModuleModule.rs (the block using code_utf8, source, and the printer) to detect and preserve a leading hashbang from code_utf8.slice() and prepend it to the final returned string (or ensure the bun_ast::Source includes it) so the output remains executable; apply the same fix in the similar block at the other occurrence (around the 301-331 region) and add a regression test that feeds input with a shebang (e.g. "#!/usr/bin/env bun\n...") and asserts the returned string begins with the original shebang.
261-271:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the banned
String::from_utf8_lossycall.Clippy is already failing on Line 271 for this exact method, so the PR cannot merge as-is. Please switch this parser-error formatting path to one of the repo-approved byte/string helpers instead of
alloc::string::String::from_utf8_lossy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/NodeModuleModule.rs` around lines 261 - 271, Replace the banned alloc::string::String::from_utf8_lossy call in the NodeModuleModule.rs error path: instead of String::from_utf8_lossy(text) use the repository's approved byte→string helper (import it) to convert text: &[u8] into a safe String/&str and pass that into the format_args! call; specifically modify the return expression that builds jsc::ErrorCode::ERR_INVALID_TYPESCRIPT_SYNTAX.throw(...) so it uses the approved helper (e.g. crate::util::decode_lossy_bytes(text) or the project's equivalent) in place of String::from_utf8_lossy, and add the necessary use/import for that helper.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/jsc/NodeModuleModule.rs`:
- Around line 225-231: The current code creates code_utf8 and a bun_ast::Source
but drops a leading shebang (#!...) when returning printed output; update the
logic around code_utf8 and the printer output in NodeModuleModule.rs (the block
using code_utf8, source, and the printer) to detect and preserve a leading
hashbang from code_utf8.slice() and prepend it to the final returned string (or
ensure the bun_ast::Source includes it) so the output remains executable; apply
the same fix in the similar block at the other occurrence (around the 301-331
region) and add a regression test that feeds input with a shebang (e.g.
"#!/usr/bin/env bun\n...") and asserts the returned string begins with the
original shebang.
- Around line 261-271: Replace the banned alloc::string::String::from_utf8_lossy
call in the NodeModuleModule.rs error path: instead of
String::from_utf8_lossy(text) use the repository's approved byte→string helper
(import it) to convert text: &[u8] into a safe String/&str and pass that into
the format_args! call; specifically modify the return expression that builds
jsc::ErrorCode::ERR_INVALID_TYPESCRIPT_SYNTAX.throw(...) so it uses the approved
helper (e.g. crate::util::decode_lossy_bytes(text) or the project's equivalent)
in place of String::from_utf8_lossy, and add the necessary use/import for that
helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06eccc6a-97b4-439a-b16a-7a0978d9881a
📒 Files selected for processing (2)
src/jsc/NodeModuleModule.rstest/js/node/module/strip-typescript-types.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the RELEASE_AND_RETURN nit. I didn't find further issues, but this is a substantial new public API touching parser internals, one-shot Transpiler setup with unsafe arena lifetime handling, and documented divergences from Node's behavior — worth a human look.
Extended reasoning...
Overview
This PR implements module.stripTypeScriptTypes for node:module, backed by Bun's native transpiler. It spans 11 files: new TsRuntimeSyntax tracking fields on Ast and the parser P struct; instrumentation in parse_stmt.rs, parse_typescript.rs, and visit/mod.rs to record non-ambient TS runtime constructs and the module keyword; two new ErrorCode discriminants (ERR_INVALID_TYPESCRIPT_SYNTAX, ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX); a ~200-line Rust FFI entrypoint in NodeModuleModule.rs that builds a one-shot Transpiler, parses, gates on the recorded flags, prints (optionally with a source map via a custom VlqCapture handler), and assembles the output with hashbang/sourceURL/sourceMappingURL handling; ~80 lines of C++ in NodeModuleModule.cpp for Node-style argument validation and the .lut.h table entry; and a 262-line test file with 34 tests.
Security risks
None identified. The API takes an in-memory string and returns a transformed string — no filesystem, network, auth, or privilege boundaries are involved. The unsafe block (bun_ptr::detach_lifetime_ref to launder the arena borrow to 'static for Transpiler<'static>) follows the same pattern documented as used by JSTranspiler / TransformTask, and the arena demonstrably outlives all uses within the function body.
Level of scrutiny
This warrants human review. It is a new public Node-compat API surface with explicitly documented behavioral divergences from Node (output is re-printed rather than blanked in place, so positions/comments are not preserved; decorators are lowered; ambient param-prop edge case differs) — those are design tradeoffs a maintainer should sign off on. The parser instrumentation, while additive and guarded to only record state, runs on every TypeScript file parsed by Bun, not just calls to this API. The Rust side configures ~15 transpiler option flags by hand, manages arena/AST-allocator scoping, a scopeguard for macro_context cleanup, and hand-builds the source-map JSON — all reasonable but non-trivial.
Other factors
My prior inline comment (wrap the throwing FFI tail call in RELEASE_AND_RETURN) was addressed in bc8ee08 and the thread is resolved. CodeRabbit's one finding was withdrawn after the author showed it matches Node's semantics. Test coverage is thorough (validation errors with exact Node messages, strip-mode rejections vs. ambient allowances, transform-mode lowering verified by evaluation, source-map shape including the hashbang line-shift). The initial CI run on 2b35529 showed failures, but a follow-up commit landed afterward; I can't see the current CI state from here.
There was a problem hiding this comment.
I didn't find any bugs, but this is a substantial new public API that threads tracking state through the core TS parser and ships with documented Node-compat divergences (re-printed output vs. blanking, comments/positions dropped), so it warrants a human look at those trade-offs and the parser instrumentation.
Extended reasoning...
Overview
This PR implements module.stripTypeScriptTypes from Node v22.13+. It adds ~280 lines of Rust in NodeModuleModule.rs (a one-shot Transpiler invocation with sourcemap assembly and hashbang handling), ~85 lines of C++ option validation in NodeModuleModule.cpp, two new ErrorCode discriminants kept index-aligned across Rust/TS/C++, and — most significantly — instruments the core parser (p.rs, parse_stmt.rs, parse_typescript.rs, visit/mod.rs) plus the Ast result struct to record the first non-ambient TS runtime-semantics construct and the module keyword. A 262-line test file with 34 tests covers validation, strip/transform behavior, ambient allowances, and sourcemap shape.
Security risks
None apparent. The function operates on caller-supplied source strings and returns a string; no filesystem, network, or privilege boundaries are crossed. The unsafe arena-lifetime laundering follows the established JSTranspiler/TransformTask pattern, and the new error codes are appended at the end of the table so existing discriminants are unchanged.
Level of scrutiny
High. The parser changes are additive (only recording state, not altering parse decisions) and look correctly placed — each record_ts_runtime_syntax call sits after the is_typescript_declare early-return so ambient constructs stay erasable, and the ParameterProperty recording in the visit pass relies on declare class never reaching visit. But these files are on the hot path for every JS/TS file Bun touches, so a maintainer should confirm the placement and that the two new Ast fields don't interact badly with any caching/serialization. The PR also documents intentional divergences from Node (output is re-printed from AST rather than blanked in place, so positions/comments aren't preserved and decorators are lowered) — those are user-visible API contract decisions that deserve explicit human sign-off rather than bot approval.
Other factors
Both prior inline review threads (CodeRabbit's sourceURL note and my RELEASE_AND_RETURN note) are resolved; the latter was addressed in bc8ee08. Test coverage is thorough and the existing transpiler/bundler suites are reported passing. No CODEOWNERS match the changed files.
|
The diff is green. The only red check is the
All other 285 jobs pass, and the new tests plus the transpiler/parser/module suites pass on every lane. No test failure is attributable to this PR. Ready for a maintainer to merge (or re-run the darwin lane). |
robobun
force-pushed
the
farm/ed35d672/node-module-strip-typescript-types
branch
from
bc4b14a to
5eb376d
Compare
|
Rebased onto main to resolve merge conflicts. Both conflicts were in the index-aligned error-code table where main's #32414 (WebKit upgrade branch) had appended
Verified post-rebase: the new suite passes (36/0) and both TS codes throw |
|
Heads up on CI state after the rebase (sha 5eb376d):
Locally on the rebased tip, A Buildkite "Rebuild" should clear the red check. Ready for a maintainer. |
robobun
force-pushed
the
farm/ed35d672/node-module-strip-typescript-types
branch
from
5eb376d to
f458d89
Compare
|
Rebased onto main again (now past #32494). Same conflict class as before: the index-aligned error-code table. Main has since appended Resolved by keeping all of main's new codes and placing this PR's two after them:
Rust consts, Verified on the rebased tip (f458d89): the suite passes 36/0, the two TS codes throw |
|
CI status on the rebased tip (f458d89, build 63387): the diff is green; the only red is on the two darwin test lanes, both from environment-sensitive flake, not this PR.
The darwin lanes have now been the sole blocker across builds 62171, 62219, 63387 while the diff stayed green. None of the failures (flaky networking, teardown SIGKILL, PTY timeouts) touch this PR's surface (node:module / parser / ErrorCode). Conflict resolution verified locally: Ready for a maintainer to merge (or re-run the darwin lanes on healthy agents). |
Adds module.stripTypeScriptTypes(code, options) backed by Bun's native
transpiler, exported from node:module for both CJS and ESM.
- Validates arguments with Node's exact error codes and messages
(ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE).
- strip mode rejects TypeScript syntax with runtime semantics (enums,
instantiated namespaces, parameter properties, import =, export =)
with ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX; ambient (declare) contexts
are allowed. The identifier-named module keyword is rejected in both
modes, matching Node's amaro behavior.
- transform mode lowers those constructs and optionally emits an inline
base64 source map ({"version":3,"sources":[sourceUrl],...}).
- sourceUrl appends a //# sourceURL comment.
- Parse errors surface as ERR_INVALID_TYPESCRIPT_SYNTAX (SyntaxError).
The parser now records the first non-ambient TS runtime-syntax
construct (Ast::ts_runtime_syntax) and whether an identifier-named
module declaration was used (Ast::uses_ts_module_keyword) so strip
mode can reject them after parsing.
Unlike Node's amaro, output is re-printed from the AST rather than
blanked in place: original line/column positions are not preserved and
comments are dropped. Decorators are lowered rather than passed
through.
Fixes #32196
A leading #! line is consumed by the lexer and was dropped from the re-printed output; carry it through like the bundler's entry-point handling, and shift the inline source map down one generated line so mappings stay aligned (matches Node's output byte for byte). Replace the clippy-disallowed String::from_utf8_lossy with bstr::BStr for displaying parser error text.
robobun
force-pushed
the
farm/ed35d672/node-module-strip-typescript-types
branch
from
f458d89 to
524b763
Compare
|
Rebased onto main again (now past #31830, which appended three Resolved by keeping main's new codes and placing this PR's two after them:
Rust consts, Verified on the rebased tip (524b763): suite passes 36/0; the two TS codes throw CI on this tip (build 63478): 285 jobs pass; the sole red is |
1 participant
Fixes #32196
Fixes #25058 (the docs listed
stripTypeScriptTypesas supported; it now is)Problem
node:moduledid not export or implementstripTypeScriptTypes(added in Node v22.13.0):Fix
Implements
module.stripTypeScriptTypes(code[, options])backed by Bun's native transpiler, exported fromnode:modulefor bothrequireand named ESM imports. The contract was verified empirically against Node v24.3.0:ERR_INVALID_ARG_TYPEfor non-stringcode/sourceUrland non-booleansourceMap,ERR_INVALID_ARG_VALUEfor badmodeand forsourceMap: truein strip mode.mode: 'strip'(default) rejects TypeScript syntax with runtime semantics, with Node's exact messages and new error codesERR_UNSUPPORTED_TYPESCRIPT_SYNTAX/ERR_INVALID_TYPESCRIPT_SYNTAX(bothSyntaxError): enums, instantiated namespaces, parameter properties,import x = require(...),export = .... Ambient constructs (declare enum,declare namespace,declare module "x", type-only namespaces,import type x = require(...)) are stripped as erasable. The identifier-namedmodule N {}keyword is rejected in both modes, like amaro.mode: 'transform'lowers enums, namespaces, parameter properties,import =andexport =.sourceUrlappends\n\n//# sourceURL=...;sourceMap: true(transform mode only) appends an inline base64 map shaped like Node's:{"version":3,"sources":[<sourceUrl>],"names":[],"mappings":"..."}.ERR_INVALID_TYPESCRIPT_SYNTAXwith Bun's parser message.trim_unused_importsoff),process.env.*is not inlined, no dead-code elimination, macros disabled.To support the strip-mode rejections, the parser records the first non-ambient TS runtime-syntax construct (
Ast::ts_runtime_syntax) and whether an identifier-namedmoduledeclaration was used (Ast::uses_ts_module_keyword). Recording happens exactly where the parser materializes these constructs, sodeclarecontexts (which returnS::TypeScriptearlier or never reach the visit pass) never set the flags.Known divergences from Node
Node's amaro blanks types in place; Bun re-prints from the AST. So:
declare class C { constructor(public x); }) are stripped instead of throwingERR_INVALID_TYPESCRIPT_SYNTAX(the input is invalid TS per tsc).Verification
test/js/node/module/strip-typescript-types.test.ts(34 tests): the issue repro via named ESM import in a subprocess, CJS export, validation errors with exact Node messages, all strip-mode rejections and ambient allowances, transform-mode lowering verified by evaluating the output, sourceURL/sourceMap output shape including decoding the base64 map. All fail on the unfixed build with the error above.Existing suites pass:
test/bundler/transpiler/transpiler.test.js(188),test/bundler/esbuild/ts.test.ts(67),test/bundler/esbuild/importstar_ts.test.ts(23),test/bundler/bundler_edgecase.test.ts(119), decorator suites (78),test/js/node/module/(98).