jsx:preserve - #20435
jsx:preserve#20435xhjkl wants to merge 1 commit into
Conversation
| // JSX attribute values: strings can be quoted, everything else needs braces | ||
| switch (v.data) { | ||
| .e_string => { | ||
| p.printExpr(v, Level.lowest, ExprFlag.None()); |
There was a problem hiding this comment.
What happens if the string has a ' or a ``` in it?
There was a problem hiding this comment.
Apologies if I'm misinterpreting your question; I'm still new to Bun's codebase.
Single quotes are handled the same way as esbuild does this: the fewest backslashes option wins.
I've added this, specifically to this purpose:
https://github.com/oven-sh/bun/pull/20435/files#diff-0ec1a66026f75c78596b8e38fe787aa6114b04566c0ae7dc45e5352bf5a81f4f
And these as well:
https://github.com/oven-sh/bun/pull/20435/files#diff-d4462df60c1819ea23eda2fc2ef00a6a79e90482d2111557c3ce740974b52c5eR34-R105
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Thank you for this, its a good start.
- I think the string escaping probably needs work.
- There's probably a sourcemap-related function call that's missing
- We probably need to handle multi-line JSX children differently
- Please add additional tests in
bundler_jsx.test.ts.
I think it would also be worth comparing esbuild's implementation to see if there are any gotchas. Bun's JS printer is still pretty similar to esbuild's.
|
👍 for the initiative I wonder if this will also handles minification/mangling let greeting = "Hello"
let elements = <h3>{greeting}</h3>should be something like this var e="Hello",l=<h3>{e}</h3>;and not var e="Hello",l=<h3>{greeting}</h3>;another case will befunction Comp(){}
let elements = <><Comp/></>should be function C(){}var a=<><C/></>;and not function C(){}var a=<><Comp/></>;I have compiled the above using esbuild, which handles preserve minify in the JSX esbuild added it here evanw/esbuild@7a727e3 , you can see additional tests cases there. |
95a3efd to
f8a15aa
Compare
|
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:
WalkthroughAdds a ChangesJSX Preserve Runtime Support
🚥 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/Arguments.zig (1)
75-75: Update --jsx-runtime help text to include all supported valuesHelp still says "automatic" or "classic". Add "solid" and "preserve" for accuracy.
Apply:
- clap.parseParam("--jsx-runtime <STR> \"automatic\" (default) or \"classic\"") catch unreachable, + clap.parseParam("--jsx-runtime <STR> \"automatic\" (default), \"classic\", \"solid\", or \"preserve\"") catch unreachable,
♻️ Duplicate comments (1)
src/js_printer.zig (1)
3084-3193: Preserve-mode JSX must emit valid JSX: fix attribute string quoting, self-closing detection, and raw non-ASCII namesIssues:
- Attribute string values go through printExpr, which may choose backticks. JSX attributes must be quoted (not backticks). Also risks the prior concern “What happens if the string has a ' or a ` in it?”
- Renders "<.../>" whenever children.len == 0. That mutates syntax for into
, losing original form. Use the AST’s self-closing flag instead.- For UTF‑16 tag/attr names, printIdentifierUTF16 will emit \u escapes under ascii_only; JSX tag/attr names must be raw (escapes are not valid in JSX names). Emit raw UTF‑8 for JSX names.
- No source map points added around JSX tokens (tag, attrs, children). Consider mapping to improve DX.
Minimal patch within this hunk:
@@ - .e_jsx_element => |e_| { + .e_jsx_element => |e_| { // Print raw JSX in preserve mode p.print("<"); if (e_.tag) |tag_expr| { switch (tag_expr.data) { .e_identifier => |id| { // component or HTMLElement identifier p.printSymbol(id.ref); }, .e_string => |str| { if (str.isUTF8()) { p.print(str.data); } else { - p.printIdentifierUTF16(str.slice16()) catch unreachable; + // JSX names must be raw (no \u escapes) + p.printJSXNameUTF16(str.slice16()) catch unreachable; } }, else => { // fallback p.printExpr(tag_expr, Level.member, ExprFlag.None()); }, } } // Props and spreads const props = e_.properties.slice(); for (props) |prop| { if (prop.kind == .spread) { p.print(" {..."); p.printExpr(prop.value.?, Level.lowest, ExprFlag.None()); p.print("}"); } else { p.print(" "); // Print JSX attribute keys as identifiers switch (prop.key.?.data) { .e_string => |str| { if (str.isUTF8()) { p.print(str.data); } else { - p.printIdentifierUTF16(str.slice16()) catch unreachable; + // JSX names must be raw (no \u escapes) + p.printJSXNameUTF16(str.slice16()) catch unreachable; } }, .e_identifier => |id| { p.printSymbol(id.ref); }, else => { // For complex expressions, fall back to regular printing p.printExpr(prop.key.?, Level.lowest, ExprFlag.None()); }, } if (prop.value) |v| { p.print("="); // JSX attribute values: strings can be quoted, everything else needs braces switch (v.data) { - .e_string => { - p.printExpr(v, Level.lowest, ExprFlag.None()); - }, + .e_string => |s| { + // Force JSX-compliant quoting (no backticks) + p.print('"'); + p.printStringCharactersEString(&s, '"'); + p.print('"'); + }, else => { p.print("{"); p.printExpr(v, Level.lowest, ExprFlag.None()); p.print("}"); }, } } } } // Children const children = e_.children.slice(); - if (children.len == 0) { + if (e_.self_closing) { p.print("/>"); } else { p.print(">"); for (children) |child| { switch (child.data) { .e_string => |s| { if (s.isUTF8()) { p.print(s.data); } else { - p.printIdentifierUTF16(s.slice16()) catch unreachable; + // Child text must be raw text + p.printJSXNameUTF16(s.slice16()) catch unreachable; } }, .e_jsx_element => { p.printExpr(child, Level.lowest, ExprFlag.None()); }, else => { p.print("{"); p.printExpr(child, Level.lowest, ExprFlag.None()); p.print("}"); }, } } p.print("</"); if (e_.tag) |tag_expr| { switch (tag_expr.data) { .e_identifier => |id| { p.printSymbol(id.ref); }, .e_string => |str| { if (str.isUTF8()) { p.print(str.data); } else { - p.printIdentifierUTF16(str.slice16()) catch unreachable; + p.printJSXNameUTF16(str.slice16()) catch unreachable; } }, else => { p.printExpr(tag_expr, Level.lowest, ExprFlag.None()); }, } } p.print(">"); } },Add this helper (place alongside other print* helpers):
pub fn printJSXNameUTF16(p: *Printer, name: []const u16) !void { // Identical to printIdentifierUTF16 but never escapes non-ASCII. const n = name.len; var i: usize = 0; const CodeUnitType = u32; while (i < n) { var c: CodeUnitType = name[i]; i += 1; if (c & ~@as(CodeUnitType, 0x03ff) == 0xd800 and i < n) { c = 0x10000 + (((c & 0x03ff) << 10) | (name[i] & 0x03ff)); i += 1; } var buf_ptr = p.writer.reserve(4) catch unreachable; p.writer.advance(strings.encodeWTF8RuneT(buf_ptr[0..4], CodeUnitType, c)); } }Follow-ups:
- Add tests for:
- preserve + minify_identifiers: local vars and component names referenced in JSX are mangled while JSX stays intact.
- Non-ASCII tag/attr names round-trip (no \u escapes).
- Keep original close style ( vs
).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 7978476 and f8a15aac3300f3595923745480eb926f593f87e5.
📒 Files selected for processing (7)
src/api/schema.zig(1 hunks)src/bunfig.zig(1 hunks)src/cli/Arguments.zig(1 hunks)src/js_printer.zig(2 hunks)src/options.zig(1 hunks)src/resolver/tsconfig_json.zig(1 hunks)test/jsx/jsx_preserve.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/api/schema.zigsrc/options.zigsrc/cli/Arguments.zigsrc/bunfig.zigsrc/js_printer.zigsrc/resolver/tsconfig_json.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/api/schema.zigsrc/options.zigsrc/cli/Arguments.zigsrc/bunfig.zigsrc/js_printer.zigsrc/resolver/tsconfig_json.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or invent random port functions
Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Never write tests that assert absence of crashes (e.g., no "panic" or "uncaught exception") in output
Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
When spawning processes in tests, assert on stdout before asserting exitCode
Do not use setTimeout in tests; await conditions instead to avoid flakiness
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Files:
test/jsx/jsx_preserve.test.ts
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/js_printer.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/js_printer.zig
🧠 Learnings (34)
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/api/schema.zigsrc/bunfig.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/api/schema.zigsrc/cli/Arguments.zigsrc/bunfig.zigsrc/js_printer.zigsrc/resolver/tsconfig_json.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/api/schema.zigsrc/options.zigsrc/cli/Arguments.zigsrc/bunfig.zigsrc/resolver/tsconfig_json.zig
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/api/schema.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to src/**/*.zig : Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Applied to files:
src/api/schema.zigsrc/options.zigsrc/resolver/tsconfig_json.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/cli/Arguments.zigsrc/js_printer.zigsrc/resolver/tsconfig_json.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
src/cli/Arguments.zigsrc/js_printer.zigsrc/resolver/tsconfig_json.zig
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/cli/Arguments.zigsrc/bunfig.zig
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/react-spa.test.ts : react-spa.test.ts should contain React SPA, react-refresh, and basic server component transform tests
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/html.test.ts : html.test.ts should contain tests relating to HTML files themselves
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/css.test.ts : css.test.ts should contain CSS bundling tests in dev mode
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/esm.test.ts : esm.test.ts should cover ESM feature behavior in development mode
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bunfig.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bunfig.zigsrc/js_printer.zigsrc/resolver/tsconfig_json.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bunfig.zigsrc/js_printer.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
src/bunfig.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bunfig.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for classes with C++ fields in JavaScriptCore bindings
Applied to files:
src/bunfig.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Applied to files:
src/resolver/tsconfig_json.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/resolver/tsconfig_json.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Applied to files:
src/resolver/tsconfig_json.zig
📚 Learning: 2025-10-25T22:53:31.261Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-25T22:53:31.261Z
Learning: Applies to src/**/*.zig : Place import statements at the bottom of the file (formatter will handle ordering)
Applied to files:
src/resolver/tsconfig_json.zig
🧬 Code graph analysis (1)
test/jsx/jsx_preserve.test.ts (1)
test/harness.ts (1)
bunExe(102-105)
🔇 Additional comments (8)
src/cli/Arguments.zig (1)
36-37: Add preserve to CLI runtime resolver — LGTMMapping "preserve" correctly returns Api.JsxRuntime.preserve.
Please also ensure downstream logic (printer, auto-import) treats .preserve as “no transform, no auto-import”.
src/options.zig (1)
1231-1233: RuntimeMap "preserve" addition verified — auto-import correctly disabled in preserve modeThe mapping is correctly added. The key safeguard already exists: in
src/ast/Parser.zigline 1344, JSX auto-import is explicitly guarded to only run whenp.options.jsx.runtime == .automatic, so no jsx-runtime helpers will be injected when runtime is set to.preserve. Minify-identifiers has no runtime-specific conditions, so identifier renaming will continue to work as expected.src/bunfig.zig (1)
1008-1008: Error message correctly updated.The error message now accurately lists all supported JSX runtime options including the new "preserve" mode.
src/resolver/tsconfig_json.zig (1)
217-221: LGTM! Preserve mode precedence correctly implemented.The guard correctly prevents
jsxImportSourcefrom overriding a preserve runtime. The comment clearly documents the precedence behavior, ensuring preserve mode is not inadvertently changed to solid when a solid-js import source is specified.test/jsx/jsx_preserve.test.ts (3)
21-43: LGTM! Test correctly validates preserve mode.The test properly verifies that JSX syntax is preserved in the output and not transformed to
React.createElementcalls. Correctly checks exit code before stderr and usesbunExe()andbunEnvfrom harness.
45-66: LGTM! Minification compatibility test is valuable.This test ensures that
--minifyand--jsx-runtime=preservework together correctly, addressing the use case mentioned in the PR objectives about preserving JSX while minifying identifiers.
68-134: LGTM! Comprehensive edge case coverage.The test suite covers important JSX preserve scenarios:
- Spread props (
{...})- Inline expressions (
{1+2})- Nested element structures
All tests properly verify that JSX syntax is preserved and React transformation doesn't occur.
src/js_printer.zig (1)
5287-5287: Based on my verification of the Zig import system and the codebase evidence:In Zig, @import() takes a string literal that is either a relative path to another file or a package name. Both
rename.Renamerandbun.renamer.Renamerresolve to the samerenamer.zigmodule (as shown in the script output:pub const renamer = @import("./renamer.zig");in bun.zig line 1489).When the same source file is imported via different paths within the codebase, they resolve to the same module and produce compatible types. The code compiles successfully despite using both
rename.Renamer(at line 5287 in theinitparameter) andbun.renamer.Renamer(at lines 5992, 6024, 6053 in public function parameters), because both paths reference the same underlyingrenamer.zigfile.This pattern mirrors the intentional design noted in the learnings, where mixing different import paths for the same module is safe when protected by module resolution.
Likely an incorrect or invalid review comment.
f8a15aa to
971a969
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/Arguments.zig (1)
75-75: Update CLI help to list all valid jsx runtimesHelp still says "automatic (default) or classic". Include "solid" and "preserve" to avoid user confusion.
Apply:
- clap.parseParam("--jsx-runtime <STR> \"automatic\" (default) or \"classic\"") catch unreachable, + clap.parseParam("--jsx-runtime <STR> \"automatic\" (default), \"classic\", \"solid\", or \"preserve\"") catch unreachable,
♻️ Duplicate comments (2)
src/bunfig.zig (1)
1005-1007: Fix casing: useapi.JsxRuntime.preserve
Apihere breaks consistency and likely fails to compile; the file usesapielsewhere.- } else if (strings.eqlComptime(value, "preserve")) { - jsx_runtime = Api.JsxRuntime.preserve; + } else if (strings.eqlComptime(value, "preserve")) { + jsx_runtime = api.JsxRuntime.preserve;test/jsx/jsx_preserve.test.ts (1)
4-15: Use harness temp directory utilities instead of mkdtemp/rmSyncReplace custom withTmpDir + node:os/tmpdir with harness helpers.
-import { mkdtempSync, writeFileSync, readFileSync, rmSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { writeFileSync, readFileSync } from "node:fs"; import path from "node:path"; - -function withTmpDir(cb: (dir: string) => void) { - const dir = mkdtempSync(path.join(tmpdir(), "bun-jsx-preserve-")); - try { - cb(dir); - } finally { - rmSync(dir, { recursive: true, force: true }); - } -} +import { tempDirWithFiles } from "harness";Then refactor each
withTmpDir(dir => { ... })block to:await tempDirWithFiles({}, async dir => { // same body... });As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between f8a15aac3300f3595923745480eb926f593f87e5 and 971a9699727ce821b542920f92bfc6054ded6daa.
📒 Files selected for processing (7)
src/api/schema.zig(1 hunks)src/bunfig.zig(1 hunks)src/cli/Arguments.zig(1 hunks)src/js_printer.zig(2 hunks)src/options.zig(1 hunks)src/resolver/tsconfig_json.zig(1 hunks)test/jsx/jsx_preserve.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/resolver/tsconfig_json.zigsrc/cli/Arguments.zigsrc/options.zigsrc/js_printer.zigsrc/bunfig.zigsrc/api/schema.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/resolver/tsconfig_json.zigsrc/cli/Arguments.zigsrc/options.zigsrc/js_printer.zigsrc/bunfig.zigsrc/api/schema.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or invent random port functions
Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Never write tests that assert absence of crashes (e.g., no "panic" or "uncaught exception") in output
Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
When spawning processes in tests, assert on stdout before asserting exitCode
Do not use setTimeout in tests; await conditions instead to avoid flakiness
Files:
test/jsx/jsx_preserve.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Files:
test/jsx/jsx_preserve.test.ts
src/**/js_*.zig
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
src/**/js_*.zig: Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Files:
src/js_printer.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/js_printer.zig
🧠 Learnings (45)
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to src/**/*.zig : Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Applied to files:
src/resolver/tsconfig_json.zigsrc/options.zigsrc/api/schema.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/resolver/tsconfig_json.zigsrc/cli/Arguments.zigsrc/js_printer.zigsrc/bunfig.zigsrc/api/schema.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
src/resolver/tsconfig_json.zigsrc/cli/Arguments.zigsrc/js_printer.zigsrc/bunfig.zigsrc/api/schema.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/resolver/tsconfig_json.zigsrc/js_printer.zigsrc/bunfig.zigsrc/api/schema.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Applied to files:
src/resolver/tsconfig_json.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/resolver/tsconfig_json.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/resolver/tsconfig_json.zigsrc/cli/Arguments.zigsrc/options.zigsrc/js_printer.zigsrc/bunfig.zigsrc/api/schema.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/resolver/tsconfig_json.zigsrc/options.zigsrc/js_printer.zigsrc/bunfig.zigsrc/api/schema.zig
📚 Learning: 2025-10-25T22:53:31.261Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-25T22:53:31.261Z
Learning: Applies to src/**/*.zig : Place import statements at the bottom of the file (formatter will handle ordering)
Applied to files:
src/resolver/tsconfig_json.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Applied to files:
src/resolver/tsconfig_json.zig
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/react-spa.test.ts : react-spa.test.ts should contain React SPA, react-refresh, and basic server component transform tests
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/css.test.ts : css.test.ts should contain CSS bundling tests in dev mode
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/esm.test.ts : esm.test.ts should cover ESM feature behavior in development mode
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Use `tempDirWithFiles` (or `tempDir`) from `harness` for temporary directories/files
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.{ts,tsx} : Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.test.{ts,tsx} : Do not use setTimeout in tests; await conditions instead to avoid flakiness
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/jsx/jsx_preserve.test.ts
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/js_printer.zigsrc/bunfig.zigsrc/api/schema.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bunfig.zig
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/bunfig.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
src/bunfig.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bunfig.zigsrc/api/schema.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bunfig.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for classes with C++ fields in JavaScriptCore bindings
Applied to files:
src/bunfig.zig
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/install/PackageManager/PackageManagerOptions.zig:187-193
Timestamp: 2025-10-16T17:32:03.074Z
Learning: In Bun's codebase (particularly in files like src/install/PackageManager/PackageManagerOptions.zig), mixing bun.EnvVar.*.get() and bun.EnvVar.*.platformGet() for environment variable lookups is intentional and safe. The code is protected by compile-time platform checks (Environment.isWindows, etc.), and compilation will fail if the wrong function is used on the wrong platform. This pattern should not be flagged as a consistency issue.
Applied to files:
src/bunfig.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bunfig.zigsrc/api/schema.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Applied to files:
src/bunfig.zig
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/api/schema.zig
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/api/schema.zig
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
Applied to files:
src/api/schema.zig
📚 Learning: 2025-09-02T19:17:26.376Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.
Applied to files:
src/api/schema.zig
🧬 Code graph analysis (1)
test/jsx/jsx_preserve.test.ts (1)
test/harness.ts (1)
bunExe(102-105)
🔇 Additional comments (3)
src/cli/Arguments.zig (1)
36-37: Add preserve: LGTMBranch correctly recognizes "preserve" and maps to Api.JsxRuntime.preserve.
src/options.zig (1)
1231-1231: Verify that the "solid" runtime addition is intended for this PR.The PR objectives and description focus exclusively on adding the "preserve" JSX runtime mode, with no mention of "solid". While the "solid" runtime (likely for SolidJS) may be useful, including it here appears to be scope creep or an undocumented addition.
Please confirm whether:
- The "solid" runtime should be part of this PR or split into a separate change
- If intentional, the PR description should document this addition
- Tests exist for the "solid" runtime mode
src/js_printer.zig (1)
3084-3193: Review comment partially verified; proposed fix syntax requires manual confirmationAll identified issues are valid:
- Empty fragments incorrectly emit
</>instead of<></>- Props loop lacks guard for fragments (when
e_.tag == null)printSymbolminifies attribute keys viarenamer.nameForSymbol, breaking preserve mode- No sourcemap calls in the JSX block
- Text content printed raw without escaping
However, the proposed fix using
p.symbols()could not be fully verified: search results show this pattern used elsewhere (line 1553), but thesymbols()method definition was not located. Before implementing the attribute-key fix, manually confirm that the Printer has asymbols()method returning a map withget()andfollow()capabilities.The core issues are sound; add tests for fragments, attribute handling with minify+preserve, and text escaping before committing fixes.
2ef804b to
0cf229d
Compare
20b1518 to
8607989
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/cli/Arguments.zig`:
- Around line 30-36: The lookup currently discards pair.development by returning
only pair.runtime; change the CLI lookup to preserve and return both runtime and
development (e.g., return the pair or a small struct/tuple containing
pair.runtime and pair.development) instead of just pair.runtime, and then update
the call site that currently sets opts.jsx.development = false to consume the
returned development flag and assign it to opts.jsx.development; locate the code
around options.JSX.RuntimeMap.get, lower_buf, and pair (and the caller that
writes opts.jsx.development) to apply this change.
In `@src/js_parser/ast/visitExpr.zig`:
- Around line 373-414: The preserve branch is double-visiting the JSX tag and
properties: stop the second visits by ensuring the earlier "tagger" code writes
its visited result back into e_.tag (or skip the later p.visitExpr call in the
preserve branch if tagger already ran), and prevent the unconditional
property-visiting block (the code that iterates e_.properties and calls
p.visitExpr on property.key/value/initializer) from running twice by moving
those visits into the runtime-specific branches or by adding a guard so only one
path visits e_.properties; specifically adjust the logic around p.visitExpr,
e_.tag, the existing tagger block, and the
e_.properties/property.key|value|initializer loops so each expression is visited
exactly once.
In `@src/js_printer/js_printer.zig`:
- Around line 1363-1370: nextUTF16CodePoint currently unconditionally combines a
high surrogate with the following code unit; change the condition so it only
forms a surrogate pair when the next code unit is a low-surrogate
(0xDC00..0xDFFF). Concretely, in fn nextUTF16CodePoint check bounds i.* <
text.len and additionally test that (text[i.*] & ~@as(u32, 0x03ff)) == 0xdc00
(or equivalently that the next unit is in 0xDC00–0xDFFF) before merging and
advancing i.*; otherwise leave the high surrogate as-is. Ensure the unique
symbol nextUTF16CodePoint is updated accordingly and keep existing bounds
checks.
- Around line 1474-1517: The JSX helper functions printJSXStringExpression,
printJSXAttributeValueString, printJSXChildText, and printJSXNameString
currently read str.data / str.slice16() directly and must first call
resolveRopeIfNeeded(&str) (or the appropriate resolveRopeIfNeeded function for
*E.String) to materialize rope-backed strings; update each helper to resolve the
rope-backed E.String at the start (or right before any direct access) so
subsequent isUTF8(), data, and slice16() reads operate on a resolved buffer and
preserve-mode output is correct.
In `@src/runtime/api/JSBundler.zig`:
- Around line 653-656: The error message in JSBundler.zig is hardcoded and
duplicates the accepted JSX runtimes; instead derive the display text from the
canonical source (options.JSX.RuntimeMap) or hoist it into a shared constant
used by both CLI and bundler so the list cannot drift. Modify the code that
calls globalThis.throwInvalidArguments (the block using slice.slice()) to build
the allowed-runtime string from options.JSX.RuntimeMap (or reference a new
shared symbol like JSX_RUNTIME_NAMES / getAllowedJsxRuntimes()) and use that
derived string in the error message; also add/replace the duplicated list in
src/cli/Arguments.zig to use the same shared symbol. Ensure you only change the
error construction and introduce the shared constant/function so both callers
use the same source of truth.
In `@test/jsx/jsx_preserve.test.ts`:
- Around line 92-95: Replace the manual for-loop that iterates `cases` and calls
`buildAndReadOut` with a parameterized test using Jest's describe.each/test.each
so each case becomes its own test; specifically, convert the block that
references `cases` and `buildAndReadOut` into a `describe.each(cases)(...)` or
`test.each(cases)([input, expectation])` which calls `await
buildAndReadOut(input)` and asserts using `expect(...).toContain(expectation)`
or better `toMatchSnapshot()`; apply the same refactor to the other occurrences
mentioned (the blocks around lines 106-109 and 127-130) so tests follow the repo
convention and provide isolated failures.
🪄 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: c01b39ec-00d6-4430-b18b-d5c25bf7d32a
📥 Commits
Reviewing files that changed from the base of the PR and between f8a15aac3300f3595923745480eb926f593f87e5 and e8118a2.
📒 Files selected for processing (11)
src/bundler/options.zigsrc/cli/Arguments.zigsrc/cli/bunfig.zigsrc/js_parser/ast/P.zigsrc/js_parser/ast/visitExpr.zigsrc/js_printer/js_printer.zigsrc/options_types/schema.zigsrc/resolver/tsconfig_json.zigsrc/runtime/api/JSBundler.zigtest/bundler/bundler_jsx.test.tstest/jsx/jsx_preserve.test.ts
💤 Files with no reviewable changes (1)
- src/js_parser/ast/P.zig
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/js_parser/ast/visitExpr.zig (3)
196-206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFragment tag computation records an unwanted runtime import in preserve mode.
The
taggerblock callsp.jsxImport(.Fragment, expr.loc)(line 204) for null-tag fragments regardless of runtime. Forruntime == .preservethis records usage of the JSX runtime'sFragmentsymbol even though the new branch (lines 373–393) discardstagfor fragments and emits raw<>...</>syntax. The result is a phantom import and incremented use-count forFragmentin preserved output.Suggest skipping the fragment import when in preserve/solid:
🐛 Suggested fix
const tag: Expr = tagger: { if (e_.tag) |_tag| { break :tagger p.visitExpr(_tag); } else { + if (p.options.jsx.runtime == .preserve or p.options.jsx.runtime == .solid) { + break :tagger Expr{ .data = .{ .e_missing = .{} }, .loc = expr.loc }; + } if (p.options.jsx.runtime == .classic) { break :tagger p.jsxStringsToMemberExpression(expr.loc, p.options.jsx.fragment) catch unreachable; } break :tagger p.jsxImport(.Fragment, expr.loc); } };🤖 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/js_parser/ast/visitExpr.zig` around lines 196 - 206, The fragment tag logic in visitExpr's tagger currently always calls p.jsxImport(.Fragment, expr.loc) for null-tag fragments, which records a runtime import even when p.options.jsx.runtime == .preserve (and similarly for .solid); update the tagger else branch to check p.options.jsx.runtime and skip calling p.jsxImport(.Fragment, expr.loc) when runtime is .preserve or .solid (return the same sentinel/handled value as the classic branch uses for preserved fragments), so preserved JSX does not cause a phantom Fragment import; locate this change around the tagger block inside visitExpr where tag is computed and p.jsxImport is invoked.
229-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
is_key_after_spreadforces preserve/solid into the classiccreateElementpath.The condition at line 229 is
runtime == .classic or is_key_after_spread. When the preserve (or solid) runtime encounters<Comp {...props} key="x" />,is_key_after_spreadis true and this branch fires, transforming the JSX into acreateElement(...)call instead of preserving it. The whole point ofpreserveis to leave JSX syntax intact for downstream tools, so this is a correctness gap.Consider gating the spread-key fallback on
runtime == .classic(orruntime == .automatic) only, and letting.preserve/.solidalways take the new branch:🐛 Suggested guard
- if (runtime == .classic or is_key_after_spread) { + if (runtime == .classic or (is_key_after_spread and runtime != .preserve and runtime != .solid)) {🤖 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/js_parser/ast/visitExpr.zig` around lines 229 - 267, The current guard "runtime == .classic or is_key_after_spread" incorrectly forces preserve/solid runtimes into the classic createElement path when a spread is followed by a key; update the condition so the spread+key fallback only applies for runtimes that should be lowered (e.g., .classic or .automatic) and NOT for .preserve or .solid. Concretely, change the if condition that references runtime and is_key_after_spread in visitExpr.zig (the block that constructs args, children_elements, and returns p.newExpr(E.Call{...})) to only take the createElement branch when runtime == .classic or (runtime == .automatic and is_key_after_spread) (or otherwise restrict is_key_after_spread to runtimes that lower JSX), leaving .preserve/.solid to follow the non-createElement path.
208-221:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd test cases for spread props and JSX children expressions in minify-identifiers mode.
The existing
minify-identifiers renames JSX component tags and expression valuestest covers component tag and attribute value renaming. However, it lacks coverage for:
- Spread props:
<Comp {...props} />wherepropsshould be renamed- JSX children expressions:
<h3>{greeting}</h3>where expressions in element content should be renamedAdd test cases for these patterns to
test/jsx/jsx_preserve.test.tsmirroring esbuild's reference coverage.🤖 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/js_parser/ast/visitExpr.zig` around lines 208 - 221, Add two test cases to the existing "minify-identifiers renames JSX component tags and expression values" test in test/jsx/jsx_preserve.test.ts: one that asserts identifier minification applies inside JSX spread props (e.g., <Comp {...props} /> where the spread expression should be renamed) and one that asserts minification applies to JSX children expressions (e.g., <h3>{greeting}</h3> where the greeting identifier should be renamed). Mirror the structure and expectations of the existing test (same input/expected transform pattern and minify-identifiers flag) so the suite verifies renaming for spread props and element child expressions in the same way as attribute values and component tags.
🤖 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/bundler/options.zig`:
- Around line 1187-1188: The two new runtime entries use full struct
initializers for RuntimeDevelopmentPair; replace the explicit initializers for
the "solid" and "preserve" entries with Zig declaration literals so they follow
repo style (i.e., use the compact .{ ... } form instead of
RuntimeDevelopmentPair{ .runtime = ..., .development = ... }) while keeping the
same runtime (.solid / .preserve) and development (null) values.
In `@src/cli/Arguments.zig`:
- Around line 1633-1639: The current assignment for opts.jsx always sets
.side_effects to the CLI boolean jsx_side_effects, which overrides an existing
opts.jsx?.side_effects when only the runtime is changed; change the
.side_effects assignment inside the Api.Jsx construction so it preserves the
existing value when the CLI flag wasn't provided (i.e., use
opts.jsx?.side_effects when no explicit jsx_side_effects override is present),
leaving the rest of the fields (factory, fragment, import_source, runtime,
development) unchanged; update the logic around opts.jsx and
runtime_pair/jsx_side_effects accordingly.
In `@test/jsx/jsx_preserve.test.ts`:
- Around line 57-92: Replace the use of test.each(...) with describe.each(...)
for the parameterized suites (including the group that uses buildAndReadOut and
asserts expect(out).toContain(expectation)); wrap each set of parameters in
describe.each and move the async assertion into an inner test(...) so each case
becomes a separate test that calls buildAndReadOut and asserts via
expect(out).toContain(expectation); apply the same change to the other
parameterized blocks in this file that currently use test.each.
---
Outside diff comments:
In `@src/js_parser/ast/visitExpr.zig`:
- Around line 196-206: The fragment tag logic in visitExpr's tagger currently
always calls p.jsxImport(.Fragment, expr.loc) for null-tag fragments, which
records a runtime import even when p.options.jsx.runtime == .preserve (and
similarly for .solid); update the tagger else branch to check
p.options.jsx.runtime and skip calling p.jsxImport(.Fragment, expr.loc) when
runtime is .preserve or .solid (return the same sentinel/handled value as the
classic branch uses for preserved fragments), so preserved JSX does not cause a
phantom Fragment import; locate this change around the tagger block inside
visitExpr where tag is computed and p.jsxImport is invoked.
- Around line 229-267: The current guard "runtime == .classic or
is_key_after_spread" incorrectly forces preserve/solid runtimes into the classic
createElement path when a spread is followed by a key; update the condition so
the spread+key fallback only applies for runtimes that should be lowered (e.g.,
.classic or .automatic) and NOT for .preserve or .solid. Concretely, change the
if condition that references runtime and is_key_after_spread in visitExpr.zig
(the block that constructs args, children_elements, and returns
p.newExpr(E.Call{...})) to only take the createElement branch when runtime ==
.classic or (runtime == .automatic and is_key_after_spread) (or otherwise
restrict is_key_after_spread to runtimes that lower JSX), leaving
.preserve/.solid to follow the non-createElement path.
- Around line 208-221: Add two test cases to the existing "minify-identifiers
renames JSX component tags and expression values" test in
test/jsx/jsx_preserve.test.ts: one that asserts identifier minification applies
inside JSX spread props (e.g., <Comp {...props} /> where the spread expression
should be renamed) and one that asserts minification applies to JSX children
expressions (e.g., <h3>{greeting}</h3> where the greeting identifier should be
renamed). Mirror the structure and expectations of the existing test (same
input/expected transform pattern and minify-identifiers flag) so the suite
verifies renaming for spread props and element child expressions in the same way
as attribute values and component tags.
🪄 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: ece862c2-3604-4ad3-8286-dbaf5a329df4
📒 Files selected for processing (6)
src/bundler/options.zigsrc/cli/Arguments.zigsrc/js_parser/ast/visitExpr.zigsrc/js_printer/js_printer.zigsrc/runtime/api/JSBundler.zigtest/jsx/jsx_preserve.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 (1)
src/bundler/options.zig (1)
1181-1188:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
react-jsxandautomaticare incorrectly mapped to the development transform.Per the TypeScript handbook,
"react-jsx"uses the production automatic JSX runtime (emitting_jsxcalls), while only"react-jsxdev"should use the development runtime. Line 1185 incorrectly maps"react-jsx"to.development = true, causing the downstream JSX pipeline to emitjsxDEVinstead of the correct production automatic transform. Similarly, line 1184 incorrectly sets"automatic"to.development = true, which should likely benullorfalseto allow users to control development mode independently.Suggested fix
pub const RuntimeMap = bun.ComptimeStringMap(RuntimeDevelopmentPair, .{ .{ "classic", RuntimeDevelopmentPair{ .runtime = .classic, .development = null } }, - .{ "automatic", RuntimeDevelopmentPair{ .runtime = .automatic, .development = true } }, + .{ "automatic", RuntimeDevelopmentPair{ .runtime = .automatic, .development = null } }, .{ "react", RuntimeDevelopmentPair{ .runtime = .classic, .development = null } }, - .{ "react-jsx", RuntimeDevelopmentPair{ .runtime = .automatic, .development = true } }, + .{ "react-jsx", RuntimeDevelopmentPair{ .runtime = .automatic, .development = false } }, .{ "react-jsxdev", RuntimeDevelopmentPair{ .runtime = .automatic, .development = true } }, .{ "solid", `@as`(RuntimeDevelopmentPair, .{ .runtime = .solid, .development = null }) }, .{ "preserve", `@as`(RuntimeDevelopmentPair, .{ .runtime = .preserve, .development = null }) }, });🤖 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/bundler/options.zig` around lines 1181 - 1188, The RuntimeMap entries incorrectly mark "automatic" and "react-jsx" as development; update RuntimeMap (the bun.ComptimeStringMap of RuntimeDevelopmentPair) so that the "automatic" and "react-jsx" keys use a non-development value (e.g., .development = null) while keeping "react-jsxdev" as .development = true; locate the entries for "automatic", "react-jsx", and "react-jsxdev" in RuntimeMap and change the .development flags accordingly so production automatic JSX is emitted for "react-jsx" and "automatic".
🤖 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/bundler/options.zig`:
- Around line 1181-1188: The RuntimeMap entries incorrectly mark "automatic" and
"react-jsx" as development; update RuntimeMap (the bun.ComptimeStringMap of
RuntimeDevelopmentPair) so that the "automatic" and "react-jsx" keys use a
non-development value (e.g., .development = null) while keeping "react-jsxdev"
as .development = true; locate the entries for "automatic", "react-jsx", and
"react-jsxdev" in RuntimeMap and change the .development flags accordingly so
production automatic JSX is emitted for "react-jsx" and "automatic".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1109ecb-a8be-4281-addb-9d6180bda290
📒 Files selected for processing (4)
src/bundler/options.zigsrc/cli/Arguments.zigsrc/js_parser/ast/visitExpr.zigtest/jsx/jsx_preserve.test.ts
|
@Jarred-Sumner, @mizulu, may I ask you to give it another glance? If I missed some points from your original feedback, can you point which ones? |
Add the preserve JSX runtime across CLI, API, bunfig, and tsconfig handling. Keep JSX syntax through bundling while still visiting nested expressions for minification and sourcemaps, and cover the preserve edge cases in bundler tests.
b7e1abd to
ea04b88
Compare
This branch has not been deployed
3 participants
What does this PR do?
This adds a new JSX runtime mode preserve, available through
--jsx-runtime=preserveor"jsx": "preserve"in tsconfig. When enabled, Bun leaves<tag />syntax untouched and emits raw JSX instead of transforming it. This is useful when you want a later build step — Babel, SWC, or another bundler — to handle the JSX transform or when distributing source that must remain uncompiled.How did you verify your code works?
Built a small SolidJS single-page demo both before and after this change, using tsconfig with
{ "compilerOptions": { "jsx": "preserve" } }.Reactis not defined."I wrote automated tests:
test/jsx/jsx_preserve.test.tsbun-debug test test-file-name.test)