refactor: move jsxSideEffects from tsconfig to jsx build config - #22665
Conversation
|
Updated 4:18 AM PT - Sep 20th, 2025
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 22665That installs a local version of the PR into your bun-22665 --bun |
WalkthroughAdds a public nested Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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
🧹 Nitpick comments (7)
src/bun.js/api/JSBundler.zig (1)
390-406: Consider validating unsupported runtime valuesThe current implementation silently ignores invalid runtime values. Consider adding validation to provide better error messages:
if (try jsx_value.getOptional(globalThis, "runtime", ZigString.Slice)) |slice| { defer slice.deinit(); if (strings.eqlComptime(slice.slice(), "classic")) { this.jsx.runtime = .classic; } else if (strings.eqlComptime(slice.slice(), "automatic")) { this.jsx.runtime = .automatic; } else if (strings.eqlComptime(slice.slice(), "react")) { this.jsx.runtime = .classic; } else if (strings.eqlComptime(slice.slice(), "react-jsx")) { this.jsx.runtime = .automatic; } else if (strings.eqlComptime(slice.slice(), "react-jsxdev")) { this.jsx.runtime = .automatic; this.jsx.development = true; } else if (strings.eqlComptime(slice.slice(), "solid")) { this.jsx.runtime = .solid; + } else { + return globalThis.throwInvalidArguments("jsx.runtime must be one of 'classic', 'automatic', 'react', 'react-jsx', 'react-jsxdev', or 'solid'", .{}); } }test/bundler/expectBundled.ts (1)
726-733: Bug: Bun CLI branch reads jsx.side_effects (snake_case) instead of jsx.sideEffectsThis makes --jsx-side-effects never set when backend="cli".
Apply:
- jsx.side_effects && ["--jsx-side-effects"], + jsx.sideEffects && ["--jsx-side-effects"],test/bundler/bundler_jsx.test.ts (5)
576-597: API-only guard hides CLI regressionsbackend: "api" ensures the test passes even if CLI flag wiring is broken. Once the Bun CLI property typo is fixed in expectBundled.ts, add a companion run with backend: "cli" to prevent regressions.
643-673: Nice: explicit jsx.development: false under productionConsider adding a mirror test where NODE_ENV=development but jsx.development:false (and vice‑versa) to assert explicit flag overrides environment heuristics for both backends.
I can draft those two override tests if you want.
715-721: Test name now misleads: not driven by tsconfig anymoresideEffects is set via API config while tsconfig is empty. Rename to “jsx/sideEffectsTrueExplicitConfig” or update the description to avoid confusion.
752-773: Same naming nit: “TsconfigClassic” still uses API overrideConsider renaming to reflect explicit config rather than tsconfig.
779-812: Same naming nit: “TsconfigAutomatic” uses API overrideRecommend renaming for clarity; the assertions themselves are correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/bun-types/bun.d.ts(1 hunks)src/bun.js/api/JSBundler.zig(1 hunks)src/bundler/bundle_v2.zig(2 hunks)src/resolver/tsconfig_json.zig(0 hunks)test/bundler/bundler_jsx.test.ts(7 hunks)test/bundler/expectBundled.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/resolver/tsconfig_json.zig
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/api/JSBundler.zigsrc/bundler/bundle_v2.zig
**/*.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
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/api/JSBundler.zigsrc/bundler/bundle_v2.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/bun.js/api/JSBundler.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/JSBundler.zig
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
packages/bun-types/bun.d.tstest/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_jsx.test.tstest/bundler/expectBundled.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/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_jsx.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_jsx.test.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#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
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#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/bun.js/api/JSBundler.zigsrc/bundler/bundle_v2.zig
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Applies to src/js/bun/**/*.{js,ts} : Place Bun-specific modules (e.g., `bun:ffi`, `bun:sqlite`) under `bun/`
Applied to files:
packages/bun-types/bun.d.tstest/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests, use normalizeBunSnapshot when asserting snapshots
Applied to files:
test/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev-and-prod.ts : Use devAndProductionTest to run the same test in both dev and production; these tests must not attempt to test hot reloading
Applied to files:
test/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/expectBundled.ts
🧬 Code graph analysis (1)
test/bundler/expectBundled.ts (1)
test/snippets/jsx-attributes.tsx (1)
jsx(28-34)
🔇 Additional comments (9)
packages/bun-types/bun.d.ts (1)
1902-1913: JSX config addition aligns with the PR goal.The new
BuildConfigBase["jsx"]shape matches the parser/transpiler wiring described in the PR and unblocks movingjsxSideEffectsintobuild.jsx.sideEffects. Nice.src/bundler/bundle_v2.zig (2)
1804-1813: LGTM! JSX config translation correctly wired upThe JSX configuration is properly mapped from the config object to the
api.Jsxstruct that gets passed to the transpiler. All fields are correctly transferred with appropriate defaults.
1834-1834: JSX config properly propagated to TransformOptionsThe
jsx_apiobject is correctly passed as the.jsxfield in the API transform options, ensuring the JSX configuration is available to the transpiler initialization.src/bun.js/api/JSBundler.zig (2)
384-437: JSX configuration parsing implementation is correct with proper memory managementThe implementation correctly:
- Validates that
jsxis an object before proceeding- Parses all JSX runtime options including
react,react-jsx,react-jsxdev, andsolid- Properly duplicates strings for
factory,fragment, andimportSourceto avoid lifetime issues- Uses
defer slice.deinit()consistently for temporary string slices- Sets
this.jsx.parse = trueto enable JSX parsing when any JSX options are providedThe memory management is sound with proper cleanup of temporary slices and appropriate ownership transfer for duplicated strings.
420-424: setImportSource correctly derives JSX importSource from package_namesetImportSource (src/options.zig:1293–1313) sets pragma.import_source.development = package_name ++ "/jsx-dev-runtime" and pragma.import_source.production = package_name ++ "/jsx-runtime"; calling this.jsx.setImportSource(allocator) after duping package_name in src/bun.js/api/JSBundler.zig is correct.
test/bundler/expectBundled.ts (2)
189-191: Type additions look good; ensure behavior is respected in CLI pathssideEffects/development on BundlerTestInput.jsx are clear. See follow-ups below to wire development into CLI flag selection and to fix a property typo in the Bun CLI branch.
1090-1098: Good: API build path plumbs JSX config through BuildConfigThis covers runtime, importSource, factory, fragment, sideEffects, development. No issues spotted.
test/bundler/bundler_jsx.test.ts (2)
452-465: Test correctly asserts sideEffects:true removes / @PURE / markers (classic)Solid coverage for classic runtime. No changes requested.
513-539: Test correctly asserts sideEffects:true removes / @PURE / markers (automatic, dev)Looks good; complements the classic case.
| /** | ||
| * JSX configuration options | ||
| */ | ||
| jsx?: { | ||
| runtime?: "automatic" | "classic"; | ||
| importSource?: string; | ||
| factory?: string; | ||
| fragment?: string; | ||
| sideEffects?: boolean; | ||
| development?: boolean; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broaden runtime union to match accepted aliases and add minimal JSDoc.
If the parser accepts "react-jsx"/"react-jsxdev" as aliases for the automatic runtime, reflect that in types to avoid unnecessary TS errors. Also add terse docs and applicability notes for classic vs automatic fields.
Apply this diff:
/**
* JSX configuration options
*/
- jsx?: {
- runtime?: "automatic" | "classic";
- importSource?: string;
- factory?: string;
- fragment?: string;
- sideEffects?: boolean;
- development?: boolean;
- };
+ jsx?: {
+ /**
+ * JSX transform runtime.
+ * Aliases `"react-jsx"` and `"react-jsxdev"` (if accepted by the parser) map to `"automatic"`.
+ */
+ runtime?: "automatic" | "classic" | "react-jsx" | "react-jsxdev";
+ /** Applies when runtime is automatic. */
+ importSource?: string;
+ /** Applies when runtime is classic. */
+ factory?: string;
+ /** Applies when runtime is classic. */
+ fragment?: string;
+ /**
+ * Treat JSX helper calls as having side effects (disables DCE of JSX calls when true).
+ */
+ sideEffects?: boolean;
+ /**
+ * Enable dev JSX transform (e.g. jsxDEV helpers).
+ */
+ development?: boolean;
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * JSX configuration options | |
| */ | |
| jsx?: { | |
| runtime?: "automatic" | "classic"; | |
| importSource?: string; | |
| factory?: string; | |
| fragment?: string; | |
| sideEffects?: boolean; | |
| development?: boolean; | |
| }; | |
| /** | |
| * JSX configuration options | |
| */ | |
| jsx?: { | |
| /** | |
| * JSX transform runtime. | |
| * Aliases `"react-jsx"` and `"react-jsxdev"` (if accepted by the parser) map to `"automatic"`. | |
| */ | |
| runtime?: "automatic" | "classic" | "react-jsx" | "react-jsxdev"; | |
| /** Applies when runtime is automatic. */ | |
| importSource?: string; | |
| /** Applies when runtime is classic. */ | |
| factory?: string; | |
| /** Applies when runtime is classic. */ | |
| fragment?: string; | |
| /** | |
| * Treat JSX helper calls as having side effects (disables DCE of JSX calls when true). | |
| */ | |
| sideEffects?: boolean; | |
| /** | |
| * Enable dev JSX transform (e.g. jsxDEV helpers). | |
| */ | |
| development?: boolean; | |
| }; |
| * JSX configuration options | ||
| */ | ||
| jsx?: { | ||
| runtime?: "automatic" | "classic"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm accepted runtime strings to keep types accurate.
If JSBundler.Config.fromJS recognizes "react-jsx"/"react-jsxdev" (as the PR text suggests), we should include them in the union. Otherwise, current typing is correct.
🏁 Script executed:
#!/bin/bash
# Locate and inspect runtime parsing to confirm accepted strings/aliases.
fd -a 'JSBundler.zig' src | xargs -I{} rg -n -C3 -e 'react-jsx' -e 'react-jsxdev' -e 'runtime' {}
# Also check where the JSX config is passed through.
fd -a 'bundle_v2.zig' src | xargs -I{} rg -n -C3 -e '\.jsx' {}Length of output: 5687
Add missing JSX runtime strings to bun.d.ts
JS parsing accepts: "classic", "automatic", "react", "react-jsx", "react-jsxdev" (sets development), and "solid" — update packages/bun-types/bun.d.ts (line 1906) from
runtime?: "automatic" | "classic"
to
runtime?: "automatic" | "classic" | "react" | "react-jsx" | "react-jsxdev" | "solid".
🤖 Prompt for AI Agents
In packages/bun-types/bun.d.ts around line 1906, the JSX runtime union type is
missing several accepted string literals; update the runtime?: "automatic" |
"classic" declaration to include the additional valid values "react",
"react-jsx", "react-jsxdev", and "solid" so the type accurately reflects the JS
parser accepted options.
| jsx.sideEffects && `--jsx-side-effects`, | ||
| env?.NODE_ENV !== "production" && `--jsx-dev`, | ||
| entryNaming && |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Honor explicit jsx.development when constructing esbuild CLI flags
Currently --jsx-dev is keyed only off NODE_ENV. Prefer explicit option when provided.
Apply:
- env?.NODE_ENV !== "production" && `--jsx-dev`,
+ (jsx.development !== undefined
+ ? jsx.development
+ : env?.NODE_ENV !== "production") && `--jsx-dev`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jsx.sideEffects && `--jsx-side-effects`, | |
| env?.NODE_ENV !== "production" && `--jsx-dev`, | |
| entryNaming && | |
| jsx.sideEffects && `--jsx-side-effects`, | |
| (jsx.development !== undefined | |
| ? jsx.development | |
| : env?.NODE_ENV !== "production") && `--jsx-dev`, | |
| entryNaming && |
🤖 Prompt for AI Agents
In test/bundler/expectBundled.ts around lines 774-776, the code sets the
--jsx-dev flag based only on NODE_ENV; change it to honor an explicit
jsx.development option when provided by checking whether jsx.development is
defined and using that value, otherwise falling back to env.NODE_ENV !==
"production". Update the condition that emits `--jsx-dev` to use the
jsx.development value if present (e.g. use a nullish/defined check) and only
fall back to the NODE_ENV check when jsx.development is undefined.
- Remove jsxSideEffects parsing from tsconfig.json - Add jsx configuration object to Bun.build() API with sideEffects option - Fix jsx config not being passed through TransformOptions - Update TypeScript definitions to include jsx config - Update test infrastructure to support jsx config in API mode - All 27 jsx tests passing This change makes jsx configuration more consistent by consolidating all jsx-related options into a single jsx object in the build configuration, rather than having jsxSideEffects scattered in tsconfig.json. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…fig.jsx to api.Jsx type - Replace string comparisons with RuntimeMap.get() for case-insensitive JSX runtime parsing - Change Config.jsx field type from options.JSX.Pragma to api.Jsx to align with API schema - Update JSBundler.fromJS to work with api.Jsx string fields instead of arrays - Convert api.Jsx back to options.JSX.Pragma in bundle_v2 for transpiler compatibility - This avoids memory leaks by keeping values as their inputs in the API layer
c711536 to
b83f752
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/bundler/expectBundled.ts (1)
184-191: Unify option naming: use jsx.sideEffects everywhereInterface adds
sideEffects/development(good), but the Bun CLI path still readsjsx.side_effects(snake_case), so the flag won’t be emitted in CLI runs.Apply:
@@ - jsx.side_effects && ["--jsx-side-effects"], + (jsx.sideEffects ?? (jsx as any).side_effects) && ["--jsx-side-effects"],Also consider removing the legacy
side_effectsfallback after downstream PRs land.
♻️ Duplicate comments (1)
test/bundler/expectBundled.ts (1)
774-776: Honor explicit jsx.development when emitting esbuild flagsPrefer
jsx.developmentif defined; fall back toNODE_ENV.- env?.NODE_ENV !== "production" && `--jsx-dev`, + ((jsx.development !== undefined ? jsx.development : env?.NODE_ENV !== "production")) && `--jsx-dev`,
🧹 Nitpick comments (3)
test/bundler/expectBundled.ts (1)
1090-1098: Avoid passing an empty jsx object into BuildConfigToday
jsx = {}makes this always truthy; only passjsxwhen at least one field is set.- jsx: jsx ? { + jsx: (jsx.runtime !== undefined || + jsx.importSource !== undefined || + jsx.factory !== undefined || + jsx.fragment !== undefined || + jsx.sideEffects !== undefined || + jsx.development !== undefined) ? { runtime: jsx.runtime, importSource: jsx.importSource, factory: jsx.factory, fragment: jsx.fragment, sideEffects: jsx.sideEffects, development: jsx.development, - } : undefined, + } : undefined,test/bundler/bundler_jsx.test.ts (2)
643-656: Explicit jsx.development=false in production is clearThis makes intent explicit and future‑proofs against env inference. Consider adding a test where
NODE_ENV="production"butjsx.development=trueto ensure the explicit option wins.Happy to add that test in this file if you want.
712-721: Test naming nit: “Tsconfig” cases now exercise API-driven sideEffectsThese tests set
sideEffectsviajsx(API) whiletsconfig.jsonis present but not the source of truth. Consider renaming to “with tsconfig present” to avoid implying tsconfig controls side effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/bun-types/bun.d.ts(1 hunks)src/bun.js/api/JSBundler.zig(2 hunks)src/bundler/bundle_v2.zig(3 hunks)src/resolver/tsconfig_json.zig(0 hunks)test/bundler/bundler_jsx.test.ts(7 hunks)test/bundler/expectBundled.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/resolver/tsconfig_json.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bun-types/bun.d.ts
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/api/JSBundler.zigsrc/bundler/bundle_v2.zig
**/*.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
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/api/JSBundler.zigsrc/bundler/bundle_v2.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/bun.js/api/JSBundler.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/JSBundler.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_jsx.test.tstest/bundler/expectBundled.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/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_jsx.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_jsx.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#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/)
Learnt from: CR
PR: oven-sh/bun#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
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/bundler/**/* : Place bundler/transpiler/CSS/bun build tests under test/bundler/
Learnt from: CR
PR: oven-sh/bun#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
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#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/bun.js/api/JSBundler.zigsrc/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#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/bun.js/api/JSBundler.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#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/bun.js/api/JSBundler.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#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/bun.js/api/JSBundler.zig
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Follow the build pipeline: Source TS/JS → Preprocessor → Bundler → C++ Headers; IDs assigned A–Z; `$` replaced with `__intrinsic__`; `require("x")` replaced with `$requireId(n)`; `export default` converted to `return`; `__intrinsic__` replaced with `@`; inlined into C++; modules loaded by numeric ID
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests, use normalizeBunSnapshot when asserting snapshots
Applied to files:
test/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.ts
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Applies to src/js/bun/**/*.{js,ts} : Place Bun-specific modules (e.g., `bun:ffi`, `bun:sqlite`) under `bun/`
Applied to files:
test/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/bundler_jsx.test.tstest/bundler/expectBundled.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev-and-prod.ts : Use devAndProductionTest to run the same test in both dev and production; these tests must not attempt to test hot reloading
Applied to files:
test/bundler/bundler_jsx.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#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/bundler/expectBundled.ts
🧬 Code graph analysis (1)
test/bundler/expectBundled.ts (1)
test/snippets/jsx-attributes.tsx (1)
jsx(28-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (8)
src/bun.js/api/JSBundler.zig (1)
16-21: JSX default config on API surface: LGTMSane defaults for factory/fragment/runtime/import_source.
src/bundler/bundle_v2.zig (2)
1804-1828: Passing APIjsxthrough TransformOptions: LGTM (confirm no double-source-of-truth issues)You pass
jsxinto Transpiler.init and also settranspiler.options.jsxlater. Since both derive fromconfig.jsx, they’re consistent, but please confirm there’s no path where one is read before the latter override and diverges.
1839-1864: Pragma mapping looks correct; sensible defaults for classic/automaticFallbacks to React defaults when
import_sourceis empty and preservesside_effects/development. Looks good.test/bundler/bundler_jsx.test.ts (5)
442-459: Good coverage: classic runtime respects jsx.sideEffects=trueAssertions correctly check that pure annotations are omitted when
sideEffectsis set.
505-540: Good coverage: automatic runtime respects jsx.sideEffects=trueDev runtime path validated; snapshot expectations look correct.
576-589: API backend choice is correct for production-sideEffects testsForcing
backend: "api"avoids CLI path quirks and exercises the newjsxconfig wiring.Please add a companion CLI-backed test (backend: "cli") after fixing the
jsx.side_effectsbug in expectBundled.ts to ensure parity across backends. I can draft it once that change lands.
753-760: Classic + tsconfig presence + API sideEffects=true looks goodConfirms classic factory/fragment mapping with side effects enabled.
783-786: Automatic + tsconfig presence + API sideEffects=true looks goodDev runtime path validated with side effects enabled.
| // Parse JSX configuration | ||
| if (try config.getTruthy(globalThis, "jsx")) |jsx_value| { | ||
| if (!jsx_value.isObject()) { | ||
| return globalThis.throwInvalidArguments("jsx must be an object", .{}); | ||
| } | ||
|
|
||
| if (try jsx_value.getOptional(globalThis, "runtime", ZigString.Slice)) |slice| { | ||
| defer slice.deinit(); | ||
| var str_lower: [128]u8 = undefined; | ||
| const len = @min(slice.len, str_lower.len); | ||
| _ = strings.copyLowercase(slice.slice()[0..len], str_lower[0..len]); | ||
| if (options.JSX.RuntimeMap.get(str_lower[0..len])) |runtime| { | ||
| this.jsx.runtime = runtime.runtime; | ||
| if (runtime.development) |dev| { | ||
| this.jsx.development = dev; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (try jsx_value.getOptional(globalThis, "factory", ZigString.Slice)) |slice| { | ||
| defer slice.deinit(); | ||
| this.jsx.factory = try allocator.dupe(u8, slice.slice()); | ||
| } | ||
|
|
||
| if (try jsx_value.getOptional(globalThis, "fragment", ZigString.Slice)) |slice| { | ||
| defer slice.deinit(); | ||
| this.jsx.fragment = try allocator.dupe(u8, slice.slice()); | ||
| } | ||
|
|
||
| if (try jsx_value.getOptional(globalThis, "importSource", ZigString.Slice)) |slice| { | ||
| defer slice.deinit(); | ||
| this.jsx.import_source = try allocator.dupe(u8, slice.slice()); | ||
| } | ||
|
|
||
| if (try jsx_value.getBooleanLoose(globalThis, "development")) |dev| { | ||
| this.jsx.development = dev; | ||
| } | ||
|
|
||
| if (try jsx_value.getBooleanLoose(globalThis, "sideEffects")) |val| { | ||
| this.jsx.side_effects = val; | ||
| } | ||
| } |
There was a problem hiding this comment.
Leak: jsx.factory/fragment/import_source are duped but never freed in Config.deinit()
You dupe these with the provided allocator but Config.deinit() doesn’t free them, causing leaks per build.
Apply this diff in Config.deinit(self: *Config, allocator: std.mem.Allocator):
self.banner.deinit();
if (self.compile) |*compile| {
compile.deinit();
}
self.env_prefix.deinit();
self.footer.deinit();
self.tsconfig_override.deinit();
+ // Free API JSX strings allocated via allocator.dupe(...)
+ if (self.jsx.factory.len > 0) allocator.free(self.jsx.factory);
+ if (self.jsx.fragment.len > 0) allocator.free(self.jsx.fragment);
+ if (self.jsx.import_source.len > 0) allocator.free(self.jsx.import_source);
+ self.jsx.factory = "";
+ self.jsx.fragment = "";
+ self.jsx.import_source = "";Additionally, consider rejecting unknown jsx.runtime values instead of silently ignoring them to surface config mistakes early.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Parse JSX configuration | |
| if (try config.getTruthy(globalThis, "jsx")) |jsx_value| { | |
| if (!jsx_value.isObject()) { | |
| return globalThis.throwInvalidArguments("jsx must be an object", .{}); | |
| } | |
| if (try jsx_value.getOptional(globalThis, "runtime", ZigString.Slice)) |slice| { | |
| defer slice.deinit(); | |
| var str_lower: [128]u8 = undefined; | |
| const len = @min(slice.len, str_lower.len); | |
| _ = strings.copyLowercase(slice.slice()[0..len], str_lower[0..len]); | |
| if (options.JSX.RuntimeMap.get(str_lower[0..len])) |runtime| { | |
| this.jsx.runtime = runtime.runtime; | |
| if (runtime.development) |dev| { | |
| this.jsx.development = dev; | |
| } | |
| } | |
| } | |
| if (try jsx_value.getOptional(globalThis, "factory", ZigString.Slice)) |slice| { | |
| defer slice.deinit(); | |
| this.jsx.factory = try allocator.dupe(u8, slice.slice()); | |
| } | |
| if (try jsx_value.getOptional(globalThis, "fragment", ZigString.Slice)) |slice| { | |
| defer slice.deinit(); | |
| this.jsx.fragment = try allocator.dupe(u8, slice.slice()); | |
| } | |
| if (try jsx_value.getOptional(globalThis, "importSource", ZigString.Slice)) |slice| { | |
| defer slice.deinit(); | |
| this.jsx.import_source = try allocator.dupe(u8, slice.slice()); | |
| } | |
| if (try jsx_value.getBooleanLoose(globalThis, "development")) |dev| { | |
| this.jsx.development = dev; | |
| } | |
| if (try jsx_value.getBooleanLoose(globalThis, "sideEffects")) |val| { | |
| this.jsx.side_effects = val; | |
| } | |
| } | |
| pub fn deinit(self: *Config, allocator: std.mem.Allocator) void { | |
| self.banner.deinit(); | |
| if (self.compile) |*compile| { | |
| compile.deinit(); | |
| } | |
| self.env_prefix.deinit(); | |
| self.footer.deinit(); | |
| self.tsconfig_override.deinit(); | |
| // Free API JSX strings allocated via allocator.dupe(...) | |
| if (self.jsx.factory.len > 0) allocator.free(self.jsx.factory); | |
| if (self.jsx.fragment.len > 0) allocator.free(self.jsx.fragment); | |
| if (self.jsx.import_source.len > 0) allocator.free(self.jsx.import_source); | |
| self.jsx.factory = ""; | |
| self.jsx.fragment = ""; | |
| self.jsx.import_source = ""; | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/api/JSBundler.zig around lines 389-430, the code dupes slice bytes
into this.jsx.factory, this.jsx.fragment, and this.jsx.import_source but
Config.deinit() does not free them; update Config.deinit(self: *Config,
allocator: std.mem.Allocator) to check each of jsx.factory, jsx.fragment, and
jsx.import_source for non-null and call allocator.free(u8, ptr) (or
allocator.free for the appropriate pointer type) then set them to null to avoid
double-free, and also free any other allocated jsx-related fields; additionally,
instead of silently ignoring unknown jsx.runtime values, validate the lowercased
runtime result and return/throw an invalid-argument error when the provided
runtime is not found so misconfigurations are surfaced early.
All 27 jsx tests now passing - the default development flag should be true to maintain compatibility with existing behavior where jsx-dev-runtime is used by default
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/api/JSBundler.zig (1)
757-783: Free API JSX strings in Config.deinit()Add frees for the duped JSX strings to prevent leaks.
Apply this diff:
self.tsconfig_override.deinit(); + // Free API JSX strings allocated via allocator.dupe(...) + if (self.jsx.factory.len > 0) allocator.free(self.jsx.factory); + if (self.jsx.fragment.len > 0) allocator.free(self.jsx.fragment); + if (self.jsx.import_source.len > 0) allocator.free(self.jsx.import_source); + self.jsx.factory = ""; + self.jsx.fragment = ""; + self.jsx.import_source = "";
♻️ Duplicate comments (1)
src/bun.js/api/JSBundler.zig (1)
409-422: Leak risk: factory/fragment/importSource are duped but never freedallocator.dupe() is used for jsx.factory/fragment/import_source, but Config.deinit() does not free them, leading to per-build leaks.
🧹 Nitpick comments (1)
src/bun.js/api/JSBundler.zig (1)
396-407: Reject invalid jsx.runtime instead of silently ignoring; add length guardCurrently unknown values are ignored. Fail fast to surface config mistakes, and guard against truncation of long inputs.
Apply this diff:
- if (try jsx_value.getOptional(globalThis, "runtime", ZigString.Slice)) |slice| { - defer slice.deinit(); - var str_lower: [128]u8 = undefined; - const len = @min(slice.len, str_lower.len); - _ = strings.copyLowercase(slice.slice()[0..len], str_lower[0..len]); - if (options.JSX.RuntimeMap.get(str_lower[0..len])) |runtime| { - this.jsx.runtime = runtime.runtime; - if (runtime.development) |dev| { - this.jsx.development = dev; - } - } - } + if (try jsx_value.getOptional(globalThis, "runtime", ZigString.Slice)) |slice| { + defer slice.deinit(); + var str_lower: [128]u8 = undefined; + if (slice.len > str_lower.len) { + return globalThis.throwInvalidArguments("jsx.runtime is too long (>{d} bytes)", .{str_lower.len}); + } + _ = strings.copyLowercase(slice.slice()[0..slice.len], str_lower[0..slice.len]); + if (options.JSX.RuntimeMap.get(str_lower[0..slice.len])) |runtime| { + this.jsx.runtime = runtime.runtime; + if (runtime.development) |dev| { + this.jsx.development = dev; + } + } else { + return globalThis.throwInvalidArguments("Invalid jsx.runtime: {s}", .{slice.slice()}); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/api/JSBundler.zig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/api/JSBundler.zig
**/*.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
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/api/JSBundler.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/bun.js/api/JSBundler.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/JSBundler.zig
🧠 Learnings (8)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#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/bun.js/api/JSBundler.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#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/bun.js/api/JSBundler.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#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/bun.js/api/JSBundler.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Always implement proper cleanup in deinit() and finalize() for JS-exposed types
Applied to files:
src/bun.js/api/JSBundler.zig
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to **/*.zig : In Zig, manage memory carefully with allocators and use defer for cleanup
Applied to files:
src/bun.js/api/JSBundler.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#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/bun.js/api/JSBundler.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#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 finalize() for objects holding JS references, release them using .deref() before destroy
Applied to files:
src/bun.js/api/JSBundler.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/bun.js/api/JSBundler.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (2)
src/bun.js/api/JSBundler.zig (2)
16-22: Default JSX config: confirm sideEffects default or initialize explicitlyYou set development = true, but side_effects relies on api.Jsx's implicit default. Please confirm this matches prior jsxSideEffects semantics; if not, initialize it explicitly here to avoid ambiguity.
428-430: Confirm sideEffects parsing aligns with prior behaviorDouble-check that the default and semantics of jsx.sideEffects here match the old tsconfig jsxSideEffects. If defaults changed, call it out in release notes and add a test.
- Fix bun-build-api 'warnings do not fail' test by setting default package_name to 'react' when import_source is empty - Add proper memory cleanup in Config.deinit() for jsx.factory, jsx.fragment, and jsx.import_source to prevent memory leaks - Add validation error for unknown jsx.runtime values to surface misconfigurations early - All 36 bun-build-api tests now passing - All 27 jsx bundler tests remain passing
- Remove 'solid' from RuntimeMap as it's not a supported JSX runtime - Update error message to list only valid runtimes: 'classic', 'automatic', 'react', 'react-jsx', 'react-jsxdev' - All tests remain passing
2 participants
Summary
jsxSideEffects(nowsideEffects) from tsconfig.json compiler options to the jsx object in the build APIChanges
src/resolver/tsconfig_json.zigsrc/bun.js/api/JSBundler.zigConfig.fromJSsrc/bundler/bundle_v2.zigsideEffectsin the jsx config instead ofside_effectsin tsconfigTest plan
All 27 jsx bundler tests are passing with the new configuration structure.
🤖 Generated with Claude Code