[robobun]

Problem

• new SourceMap({ version: 3, sources: ["a.js"], names: [], mappings: "" }) from node:module throws TypeError: payload 'mappings' must be a string (ERR_INVALID_ARG_TYPE). Node accepts it and findEntry returns {}.
• "mappings": "" is what esbuild and tsc emit for modules that generate no code (re-export-only or side-effect-only modules), so a tool that walks a build's maps with new SourceMap(json) fails on the first such module.
• Cause: the constructor reads the property with get_stringish (src/sourcemap_jsc/JSSourceMap.rs:118), and get_stringish maps an empty string to None (src/jsc/JSValue.rs:1199). That helper is meant for options like Bun.serve's hostname, where "" means "not set". For mappings, "" is a real value. The Zig implementation used the same helper, so this is not a port regression.

Fix

• Read mappings with a plain property get, reject only undefined and null with the existing error, and convert everything else to a string as before. The VLQ parser already returns an empty mapping list for an empty input, so findEntry/findOrigin on such a map return {} like Node.
• Behavior for values other than "" is unchanged except that false now goes through string conversion like every other non-string value (it fails to parse with a SyntaxError instead of being reported as missing) and a symbol now throws the engine's "Cannot convert a symbol to a string" TypeError, which is what Node throws too.
• Verified with test/js/node/module/sourcemap.test.js: the new "accepts an empty mappings string" test fails on the released build with the error above and passes with this change. Also added tests pinning the ;;; case and the undefined/null/missing rejections, which pass before and after.
• test/js/node/module/module-sourcemap.test.js, sourcemap-simd.test.ts, node-module-module.test.js and test/js/bun/sourcemap/negative-vlq-mapping.test.ts still pass.
• The 0-based findOrigin divergence from Node in the same class is a separate issue handled by node:module: align SourceMap.findOrigin/findEntry/payload with Node.js #35123 and is not touched here.

Background

• module.SourceMap is Node's API for querying a source map v3 payload: the constructor takes the parsed JSON object and findEntry(line, column) returns the mapping segment covering a generated position, or {} when there is none.
• mappings is the VLQ-encoded segment list of a source map. Segments are separated by ,, generated lines by ;, and a map for a file with no generated code has an empty string here.
• get_stringish is a JSValue helper in src/jsc/JSValue.rs that reads a property, coerces it to a string, and returns None for missing, null, false, and empty-string values so callers can treat all of those as "option not provided".

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 22c1826c-91f6-47fd-9736-83e6f680f519

📥 Commits

Reviewing files that changed from the base of the PR and between 9b3b847 and f02488c.

📒 Files selected for processing (2)

• src/sourcemap_jsc/JSSourceMap.rs
• test/js/node/module/sourcemap.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: closed. #45677 supersedes this PR.

The bug this PR addressed, reproduced on bun 1.4.0:

const { SourceMap } = require("node:module");
new SourceMap({ version: 3, sources: ["a.js"], names: [], mappings: "" });
// bun:  TypeError: payload 'mappings' must be a string (ERR_INVALID_ARG_TYPE)
// node v26.3.0: constructs, findEntry(0, 0) returns {}

Main at 9634520 no longer throws for this input. A missing, undefined or null mappings still throws the same TypeError.

[claude]

LGTM — small, focused Node compat fix.

What was reviewed:

• Confirmed get() already maps missing/undefined to None and the new is_undefined_or_null() guard handles null, so the rejection path covers exactly missing/undefined/null as tested.
• Confirmed to_bun_string returns JsResult so symbol coercion throws and propagates via ? (matches Node's TypeError, as noted).
• Checked that the ;;; and empty-string paths flow into the existing VLQ parser unchanged; findEntry/findOrigin early-return {} when no mapping is found.
• Tests use test.each with exact name/code/message assertions and pin both the fix and the unchanged rejection behavior.

Extended reasoning...

Overview

This PR changes one property read in the node:module SourceMap constructor (src/sourcemap_jsc/JSSourceMap.rs): it replaces get_stringish — which treats "" as "not provided" — with a plain get + is_undefined_or_null filter + to_bun_string coercion. This lets mappings: "" (a valid source map v3 payload emitted by esbuild/tsc for no-code modules) construct successfully instead of throwing ERR_INVALID_ARG_TYPE. Three tests are added to test/js/node/module/sourcemap.test.js: the empty-string acceptance case, a ;;; line-separators-only case, and a test.each matrix pinning that missing/undefined/null still reject with the same error.

Security risks

None. This is input validation for a source-map JSON field in a Node compat API. The value flows into the existing VLQ parser (mapping::parse), which already handles empty input by returning an empty mapping list, and non-string values still fail (either at to_bun_string for symbols or at VLQ parse for e.g. false → "false"). No new untrusted-input paths, no allocations sized from unvalidated data, no filesystem/network effects.

Level of scrutiny

Low. The diff is ~10 lines of safe Rust replacing one helper call with the equivalent inline logic minus the empty-string filter. I traced get() in src/jsc/JSValue.rs:1105-1133 to confirm it returns None for both missing properties and undefined, so the match arm covers all three rejection cases the tests assert. to_bun_string returns JsResult, so exception propagation via ? is preserved. The rest of the constructor (sources/names iteration, VLQ parsing, cached-value setters) is untouched.

Other factors

The PR description accurately documents the two intentional behavior changes for non-string values (false now reaches the VLQ parser and fails with SyntaxError; symbols throw the engine's coercion TypeError, matching Node). Tests follow repo conventions: exact error object matching via expect.objectContaining with name/code/message, test.each for the rejection matrix, and assertions on findEntry/findOrigin returning {} rather than just "doesn't throw". The author reports the new test fails on the released build and passes with the fix, and that adjacent sourcemap test files still pass. No prior reviewer comments to address.

[robobun]

^{Updated 4:09 AM PT - Aug 14th, 2026}

✅ @robobun, your commit f02488c44d043d1fe37c21476ad5a05e56573782 passed in Build #95536! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 38276

That installs a local version of the PR into your bun-38276 executable, so you can run:

bun-38276 --bun

[claude]

LGTM — targeted Node compat fix that stops get_stringish from treating an empty mappings string as "not provided".

What was reviewed:

• Confirmed JSValue::get already maps missing/undefined to None, so the is_undefined_or_null guard plus the fallthrough arm preserves the existing rejection for missing/undefined/null.
• to_bun_string(global)? propagates JS exceptions correctly (symbols now throw the engine's TypeError, matching Node) and the returned string is still wrapped in OwnedString for the same lifetime as before.
• Tests cover the fix (""), a sibling case (";;;"), and pin the unchanged rejection paths via test.each; added alongside existing tests in the right file.

Extended reasoning...

Overview

This PR fixes a Node.js compatibility bug in node:module's SourceMap constructor: passing mappings: "" (which esbuild and tsc emit for modules that generate no code) incorrectly threw ERR_INVALID_ARG_TYPE because the constructor read the property via get_stringish, a helper that intentionally maps empty strings to None for optional-config use cases. The fix replaces the get_stringish call with a plain get + is_undefined_or_null check + to_bun_string, so "" reaches the VLQ parser (which already handles it correctly, returning an empty mapping list). Six lines of Rust in src/sourcemap_jsc/JSSourceMap.rs and 33 lines of new tests in the existing test/js/node/module/sourcemap.test.js.

Security risks

None. This is input-validation loosening on a public Node API in the direction of Node's own behavior, and the value still flows into the same VLQ parser it did before. No new allocation, FFI, or unsafe code.

Level of scrutiny

Low. The change is a mechanical replacement of one property-read helper with the equivalent inline logic minus the empty-string filter. I verified against src/jsc/JSValue.rs that get returns None for both missing and undefined properties, so the match's fallthrough arm covers exactly the same rejection set as before (missing/undefined/null), and to_bun_string(global)? propagates coercion exceptions via JsResult just as get_stringish did internally. The only intentional behavior deltas beyond "" — false now fails at VLQ parse instead of the type check, and symbols throw the engine's TypeError — are called out in the PR description and move Bun closer to Node.

Other factors

Test coverage is solid: the primary fix has a test that would fail on the released build; the ;;; sibling case is covered; and the missing/undefined/null rejection paths are pinned with a test.each matrix asserting the exact error name/code/message. Tests were added to the correct existing file rather than a new one. The comment-cop bot's feedback about a verbose code comment was already addressed in f02488c (comment removed; behavior pinned by tests instead), and that thread is resolved. No memory-safety or GC concerns — the string ownership path (to_bun_string → OwnedString::new → to_utf8) is identical to the pre-change code.

[robobun]

Closing: #45677 supersedes this PR. It merged on 2026-10-20 and makes the SourceMap constructor accept an empty mappings string.

On a debug build of main at 9634520, new SourceMap({ version: 3, sources: ["a.js"], names: [], mappings: "" }) constructs, and findEntry(0, 0) and findOrigin(0, 0) return {}. The tests that this PR adds pass on main without the source change.

Reopen this PR if the constructor still throws for an empty mappings string.