Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68c516ba9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (match (!current, Hashtbl.find_opt marker_locs id) with | ||
| | Some builder, Some loc -> | ||
| let generated_line, generated_column = Ext_pp.position fmt in | ||
| add_mapping builder ~generated_line ~generated_column loc |
There was a problem hiding this comment.
comment_of_loc stores every marker ID in the process-global marker_locs, but mark_comment only does a find_opt and never removes the entry. In long-lived processes (notably rewatch watch mode), each rebuild/file compile adds more IDs and this table grows without bound, which steadily increases memory use and retained location data even after maps are emitted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
👍 Good catch. The rewatch-specific leak is limited today because rewatch spawns bsc as a separate process, so the OCaml global table does not live for the whole watch session. Still, consumed markers should be removed from marker_locs; they are one-shot internal markers and keeping them unnecessarily retains location data for the rest of the compiler process. I’ll update mark_comment to remove marker entries after lookup.
| | from_root :: _, to_root :: _ when from_root = to_root -> | ||
| let from_rest, to_rest = drop_common from_parts to_parts in | ||
| let parts = repeat ".." (List.length from_rest) @ to_rest in | ||
| if parts = [] then Filename.basename to_file else String.concat "/" parts | ||
| | _ -> Filename.basename to_file |
There was a problem hiding this comment.
When the first normalized path segment differs, relative_path falls back to Filename.basename, which drops directory context entirely. If generated outputs and source files resolve through different absolute prefixes (for example symlink/canonicalized roots), the map will emit only bare filenames, causing ambiguous or unresolvable sources entries when sourcesContent is disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
👍 Good catch. Falling back to Filename.basename can lose useful source path context and make sources ambiguous when sourcesContent is disabled. I updated the path logic to preserve relative paths whenever possible, even when the first normalized path segment differs. It now only falls back to a normalized absolute path for cross-drive Windows paths, where a filesystem-relative path is not representable.
|
|
||
| pub namespace: Option<NamespaceConfig>, | ||
| pub jsx: Option<JsxSpecs>, | ||
| #[serde(rename = "sourceMap")] |
There was a problem hiding this comment.
maybe these new options need to be nested in another "sourceMap": {}
object? You only use them together anyway, right?
There was a problem hiding this comment.
That’s a fair point. I kept the MVP shape as top-level fields to keep the implementation simple, but nesting the source map-specific options under a sourceMap object also looks like a good direction, especially if we add more options later.
{
"sourceMap": {
"mode": "linked",
"sourcesContent": true,
"sourceRoot": "",
...
}
}
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
amazing!! really excited to see this land. will help me leak my app source-code more easily :-p |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7ca25e7a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match Hashtbl.find_opt marker_locs id with | ||
| | None -> None | ||
| | Some loc -> | ||
| Hashtbl.remove marker_locs id; |
There was a problem hiding this comment.
lambda_as_module renders the same compiled program once per package spec (e.g. both CommonJS and ESM), but mark_comment resolves locations via take_marker_loc, which deletes each marker after the first render pass. That means the first .js.map gets mappings, while subsequent outputs encounter missing marker ids and end up with empty/partial mappings, breaking sourcemaps whenever package-specs contains more than one output target.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| t.column <- 0; | ||
| loop (i + 1) | ||
| | c -> | ||
| let byte = Char.code c in |
There was a problem hiding this comment.
Might be good to add some comment here to explain what's going on with those hex constants.
There was a problem hiding this comment.
Please refer to the comment f941252 (this PR)
| @@ -0,0 +1,293 @@ | |||
| type source = {relative_path: string; content: string option} | |||
There was a problem hiding this comment.
Maybe you can let your agent add some comments in this file, at least at the top, to explain the structure of source maps and any other relevant considerations.
There was a problem hiding this comment.
Please refer to the comment f941252 (this PR)
|
I was wondering about the following: Aren't we sort of "abusing" comments here in order to store references to source locations? Wouldn't it be cleaner to introduce a separate field? I chatted with Codex about this and got: In PR #8393, source-map locations are carried through the JS printer by putting internal marker strings into `J.expression.comment`, `J.statement.comment`, and `J.case_clause.comment`. The key flow is:
So yes, it is an overload of the comment channel. “Abuse” is a fair characterization, though it is a pragmatic one: the comment field already exists on expressions/statements/case clauses and is already printed exactly where source-map segments need to be observed. I agree a specific field would be cleaner if this is meant to become maintained infrastructure. Something like: type node_meta = {
comment: string option;
source_loc: Location.t option;
}or even just adding
The tradeoff is churn. The current PR reuses an existing field and existing optional |
|
@cknitt You’re right. The current implementation does overload the existing comment field. I reused it because comment is already present on the JS IR nodes where source map positions are useful, such as expressions, statements, and case clauses. It also flows through the printer at almost exactly the points where we need to observe generated line/column positions. For the MVP, this kept the implementation small and avoided touching a large number of JS IR constructors and helper APIs. That said, I agree that source locations are metadata rather than comments, and a dedicated field would be cleaner conceptually. As you mentioned, introducing something like source_loc would likely create a much larger diff because it needs to be threaded through expression, statement, case clause constructors, folds/maps, and printer code. So my intent here was to keep the first implementation pragmatic and low-churn, but I agree this is a good direction if we want to harden the source map support beyond the MVP. |
Yes, MVP is great so that people can already do first testing (which I see from the feedback here that they are already doing: https://forum.rescript-lang.org/t/feedback-wanted-experimental-rescript-source-map-support/7170). I personally didn't have time to test yet, but still planning to. But before merging, I think we should move to the cleaner implementation. |
I agree. This will likely create a fairly large diff across the JS IR helpers and related code, but I’ll keep working on it. |
|
@cknitt I refactor the implementation to avoid reusing comment as the source map marker channel. Source locations are now carried explicitly on the JS IR via |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1892e2f1e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SourceMapConfig::Bool(true) => { | ||
| panic!("sourceMap true is unsupported; use {{ \"mode\": \"linked\" }}") | ||
| } |
There was a problem hiding this comment.
sourceMap: true instead of panicking at runtime
This branch panics for sourceMap: true, and because SourceMapConfig currently deserializes any boolean, a user config typo can crash rescript watch/rescript build with an unrecoverable panic instead of a normal validation error. That turns an input error into process termination (including watch-session interruption), so this should be handled as a parse/config error path rather than panic!.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
👍 Good catch, this is valid.
sourceMap: true is intentionally unsupported, but it should be rejected during config parsing rather than accepted and handled later with panic!. I updated SourceMapConfig so true is no longer representable after deserialization: false becomes Disabled, object config becomes Options, and true now returns a normal config error. I also replaced the panic-based test with a parse-error test.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Great work! I wonder if test coverage should be expanded? |
# Conflicts: # rewatch/src/build.rs
Good point. I expanded the source map build test fixture so The test now checks exact generated-to-original mappings for both |
4 participants
[2026-05-03 UPDATE] Add inline and hidden source map modes
Summary
Introduce Source Map v3 generation for ReScript JavaScript output, including linked, inline, and hidden source map modes.
This adds per-file source map generation and wires source map options through
rescript.json,bsc, and rewatch. Depending onsourceMap.mode, the compiler can emit linked.js.mapfiles, inline data-URI source maps, or hidden.js.mapfiles without a JSsourceMappingURLcomment.Basic
rescript.jsonusage:{ "sourceMap": { "enabled": "dev", "mode": "linked", "sourcesContent": true, "sourceRoot": "" } }enabled: "dev"generates source maps only duringrescript watch.enabled: "always"generates source maps during bothrescript buildandrescript watch.mode: "linked"generates*.js.mapfiles next to generated JavaScript and links them from the JS output with asourceMappingURLcomment.mode: "inline"embeds the source map into the generated JavaScript as adata:application/json;base64,...source map comment and does not emit a sibling.js.mapfile.mode: "hidden"generates*.js.mapfiles next to generated JavaScript without adding asourceMappingURLcomment. This is useful for workflows that upload source maps to error monitoring services without publicly linking them from JS.sourcesContent: trueembeds original.ressource text insourcesContent. Withmode: "inline", this also embeds the original source text directly into the generated JS.sourceRootis optional and most local/Vite workflows do not need it.This PR also preserves source locations through function output, call expressions, pipe expressions, and pattern-match branch output so browser breakpoints and Node stack traces can resolve back to .res files more usefully.
Validation
From the repository root:
The build test suite includes
tests/build_tests/source_map, which verifies linked, inline, and hidden source map modes. It checks linked.js.mapoutput and JS comments, hidden.js.mapoutput without JS comments, inline data-URI map decoding, stale.mapcleanup, non-empty mappings, source contents,sourceRoot,sourcesContent: false, and inline maps on stdout.For manual browser and Node sourcemap behavior, see the external Vite test project:
https://github.com/mununki/rescript-sourcemap-test
The test project README explains how to check out the sourcemap branch of a local ReScript compiler clone, link it from package.json, and verify sourcemaps in Vite/Chrome DevTools and Node.
Next To Do
namesmappings for better symbol-level debugging.