Delete the schema::api mirror types and bun_api; one loader numbering across Rust/C++/JS - #37095
Delete the schema::api mirror types and bun_api; one loader numbering across Rust/C++/JS#37095dylan-conway wants to merge 4 commits into
Conversation
…lete bun_api `bun_options_types::schema::api` was a hand-maintained mirror of types the rest of the codebase already has (`Target`, JSX runtime, sourcemap/packages modes, log level, dotenv behavior), left over from the peechy schema, plus the CLI/bunfig option structs. Every consumer converted back and forth through `from_api`/`to_api` shims. - `TransformOptions` (src/options_types/transform_options.rs) now holds `bun_ast::Target`, `bun_ast::Level`, `jsx::Options`/`jsx::Runtime`, `SourceMapOption`, `PackagesOption` and `Option<DotEnvBehavior>` directly; `define`/`serve_define` are ordered `StringPairs` instead of parallel key/value arrays; write-only fields (`no_summary`, `disable_hmr`, `serve`, `inject`) are gone. - `BunInstall`/`NpmRegistry`/`NpmRegistryMap`/`Ca` move to src/options_types/install_config.rs; `NpmRegistry::from_url` replaces the `bun_api` crate (deleted). Unread `install.lockfile.path`/`savePath` fields and their bunfig parsing are removed. - `SourceMapOption`/`PackagesOption` move down into `bun_options_types` so the CLI can name them; `TargetExt`, `MessageLevel`, `JsxRuntime`, `SourceMapMode`, `PackagesMode`, `StringMap`, `source_map_mode_jsc.rs` and the `EnvBehavior`/`StringPointer` alias paths are deleted. - `DotEnvBehavior` gets normal variant names and loses its `_none` sentinel (callers that meant "unset" use `Option`). `jsx::Runtime::_None` is gone. - `schema.rs` keeps only the `api::Loader` wire enum shared with C++/JS builtins, to be unified separately. Behavior is unchanged except that bunfig `logLevel = "info"` now selects the info level instead of silently behaving like `"error"`.
WalkthroughThe PR removes the legacy ChangesCanonical type migration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bunfig/bunfig.rs (1)
232-248: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the log-level error message to include the newly accepted
infovalue.
load_log_levelnow acceptsb"info"as a valid log level (line 238), but the error message at line 242 still says"Invalid log level, must be one of debug, error, or warn". Update the message so it matches the accepted values.📝 Proposed fix for the error message
return self.add_error( expr.loc, - b"Invalid log level, must be one of debug, error, or warn", + b"Invalid log level, must be one of debug, info, warn, or error", );🤖 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/bunfig/bunfig.rs` around lines 232 - 248, Update the invalid-value message in load_log_level to list info alongside debug, error, and warn, matching the accepted match arms while leaving validation behavior unchanged.
🤖 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/options_types/install_config.rs`:
- Around line 23-32: Update the URL handling branch in the install configuration
logic to strip authentication whenever either url.username or url.password is
non-empty. Preserve username-only credentials in registry.username, retain token
handling for password-only URLs, and use the unauthenticated URL for
registry.url; add regression cases covering username-only,
username-with-empty-password, and token URLs.
---
Outside diff comments:
In `@src/bunfig/bunfig.rs`:
- Around line 232-248: Update the invalid-value message in load_log_level to
list info alongside debug, error, and warn, matching the accepted match arms
while leaving validation behavior unchanged.
🪄 Autofix
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: 9816770c-94a8-4484-abca-1e337cbfe39b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
Cargo.tomlsrc/api/Cargo.tomlsrc/api/lib.rssrc/ast/runtime.rssrc/ast/target.rssrc/bun.js.rssrc/bun_core/util.rssrc/bundler/ParseTask.rssrc/bundler/lib.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/bundler_jsc/lib.rssrc/bundler_jsc/source_map_mode_jsc.rssrc/bunfig/Cargo.tomlsrc/bunfig/bunfig.rssrc/dotenv/env_loader.rssrc/http/HeaderBuilder.rssrc/http/Headers.rssrc/http_jsc/headers_jsc.rssrc/http_types/ETag.rssrc/ini/Cargo.tomlsrc/ini/lib.rssrc/install/NetworkTask.rssrc/install/PackageManager.rssrc/install/PackageManager/PackageManagerOptions.rssrc/install/auto_installer.rssrc/install/lib.rssrc/install/npm.rssrc/install_jsc/Cargo.tomlsrc/install_jsc/ini_jsc.rssrc/js_parser/parser.rssrc/jsc/Cargo.tomlsrc/jsc/VirtualMachine.rssrc/options_types/bundle_enums.rssrc/options_types/context.rssrc/options_types/install_config.rssrc/options_types/jsx.rssrc/options_types/lib.rssrc/options_types/schema.rssrc/options_types/transform_options.rssrc/resolver/options.rssrc/resolver/resolver.rssrc/runtime/api/JSBundler.rssrc/runtime/api/JSTranspiler.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/bake_body.rssrc/runtime/bake/mod.rssrc/runtime/bake/production.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/build_command.rssrc/runtime/cli/exec_command.rssrc/runtime/cli/mod.rssrc/runtime/cli/repl_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rssrc/runtime/jsc_hooks.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/StaticRoute.rssrc/url/lib.rstest/bundler/cli.test.ts
💤 Files with no reviewable changes (9)
- src/api/Cargo.toml
- Cargo.toml
- src/bundler_jsc/lib.rs
- src/bundler/lib.rs
- src/api/lib.rs
- src/jsc/Cargo.toml
- src/ast/target.rs
- src/bundler_jsc/source_map_mode_jsc.rs
- src/http_types/ETag.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this is a 63-file refactor of the shared option types flowing through the CLI, bunfig, bundler, resolver, install, bake, and JSC layers, with one acknowledged behavior change (logLevel = "info") and several removed struct fields — worth a human pass.
What was reviewed:
jsx::Runtime::_Noneremoval —Automaticis now#[default]and theto_parser_jsx_pragmafold it replaced is dead; discriminants kept stable for the transpiler-cache hash.- The dropped manual
Pragmabuild injs_bundle_completion_task.rs::configure_bundler—create_and_configure_transpileralready routesconfig.jsxthroughfrom_transform_options → Pragma::from_options, which is field-for-field equivalent. DotEnvBehavior::_none → Option— checkedrun_env_loader,Env::default(was already explicitDisable),HTMLBundle,ServerConfig, and both bakeinit_transpilercopies.--compiledefine-precedence rewrite inbuild_command.rs—mem::take+extendpreserves compile-target-first, user-last ordering.
Extended reasoning...
Overview
This PR deletes the hand-maintained bun_options_types::schema::api mirror layer (and the bun_api crate that re-exported it) and threads the canonical types (bun_ast::Target, bun_ast::Level, jsx::Options/Runtime, SourceMapOption, PackagesOption, DotEnvBehavior, NpmRegistry/BunInstall) directly through TransformOptions and every consumer. It touches 63 files across the bundler, resolver, install/npmrc, bunfig, CLI arg parsing, JSC option converters, bake dev/prod, HTTP header StringPointer re-exports, and url. define/serve_define change from parallel key/value Vecs to Vec<(Box<[u8]>, Box<[u8]>)>; _None/_none sentinels become Option<T>; NpmRegistry::from_url absorbs the old bun_api::npm_registry::Parser; and four write-only TransformOptions fields plus two unread BunInstall lockfile-path fields are dropped.
Security risks
None identified. The change is a type-level refactor of internal option plumbing; the one reshaped input-parsing path (NpmRegistry::from_url) is a byte-for-byte move of the old parse_registry_url_string_impl body.
Level of scrutiny
High. This is not mechanical: it removes enum variants whose absence changes match exhaustiveness across every consumer, reshapes how define maps flow into the bundler's RawDefines, deletes a large hand-rolled Pragma construction in Bun.build's bundler thread on the claim that from_transform_options now produces the equivalent, drops fields the author asserts are unread, and intentionally changes what bunfig logLevel = "info" means. Each of those is individually plausible and my spot-checks held up, but the blast radius (option types read by essentially every subsystem) puts it well outside the auto-approve envelope.
Other factors
I traced the highest-risk removals: (1) the deleted configure_bundler JSX block — confirmed create_and_configure_transpiler sets TransformOptions.jsx = Some(config.jsx.clone()) before Transpiler::init, and Pragma::from_options produces the same factory/fragment/import-source/development/side-effects/parse fields the manual block did; (2) DotEnvBehavior::_none — Env::default() was already explicit Disable so the enum-default change is unobservable there, and serve_env_behavior consumers (HTMLBundle, ServerConfig) correctly branch on the new Option; (3) the --compile define-merge rewrite keeps compile-target defines first so later user defines still win in RawDefines::insert; (4) bake_body's sourcemap swap from the deleted source_map_mode_from_js to SOURCE_MAP_OPTION_MAP.from_js — the new path stringifies non-strings and returns None, so the fallthrough error still fires. The new cli.test.ts case exercises the intentional logLevel change. I did not independently verify the "never read" claim for the removed lockfile_path/save_lockfile_path/inject/serve/no_summary/disable_hmr fields — a human should confirm nothing (docs, C++, codegen) still references them.
|
Updated 12:54 PM PT - Aug 7th, 2026
✅ @dylan-conway, your commit 2ca944091f0fc115481eb307d09b37dffea8b749 passed in 🧪 To try this PR locally: bunx bun-pr 37095That installs a local version of the PR into your bun-37095 --bun |
There was a problem hiding this comment.
No bugs found, but this is a 64-file architectural refactor spanning CLI/bunfig parsing, bundler options, install config, bake, and JSC option paths — worth a human pass.
What was reviewed: Verified --compile define precedence in build_command.rs preserves compile-first/user-last ordering; confirmed the removed inline Pragma construction in js_bundle_completion_task.rs is equivalent to the new Pragma::from_options path via TransformOptions.jsx; checked the four dropped TransformOptions fields (inject/serve/no_summary/disable_hmr) and BunInstall.{lockfile_path,save_lockfile_path} have no remaining readers; confirmed DotEnvBehavior::_none → Option<DotEnvBehavior> sentinel replacement is handled at every match site.
Extended reasoning...
Overview
This PR removes the schema::api mirror-type layer and the bun_api crate, replacing them with the canonical types they duplicated. TransformOptions moves to its own file and now holds bun_ast::Target, bun_ast::Level, jsx::Options, SourceMapOption, PackagesOption, and Option<DotEnvBehavior> directly; define/serve_define become Vec<(Box<[u8]>, Box<[u8]>)> instead of parallel-array StringMap. BunInstall/NpmRegistry/Ca move to install_config.rs. Deleted: TargetExt, MessageLevel, JsxRuntime, api::Jsx, SourceMapMode, PackagesMode, StringMap, source_map_mode_jsc.rs, jsx::Runtime::_None, the to_parser_jsx_pragma fold, and a duplicate Pragma construction in Bun.build. One intentional behavior change: bunfig logLevel = "info" now maps to info instead of falling through to error.
Security risks
None identified. The NpmRegistry::from_url credential-splitting logic is a verbatim move (confirmed against the deleted bun_api body); CodeRabbit's userinfo concern was resolved as intentionally behavior-neutral. No new parsing of untrusted input; the StringPointer re-export cleanup is import-path-only.
Level of scrutiny
High. This touches option plumbing across CLI parsing, bunfig, bundler, install, bake, and the JS-facing Bun.build/Bun.Transpiler APIs. While the changes are largely mechanical type-swaps, there are several representation shifts whose equivalence is subtle: sentinel enum variants replaced by Option<T> (DotEnvBehavior::_none, jsx::Runtime::_None, api::Target::_none), Option<StringMap> collapsed to an always-present Vec, and the Bun.build JSX path now routing through Pragma::from_options instead of an inline construction. Each of these needs the reader to confirm every consumer's "is set" check was updated to the new shape.
Other factors
I spot-checked the highest-risk equivalences: (1) build_command.rs compile-define ordering — the take + extend preserves compile-first so user --define still wins; (2) the removed transpiler.options.jsx = Pragma { .. } block in js_bundle_completion_task.rs is now covered by passing jsx: Some(config.jsx.clone()) through TransformOptions → from_transform_options → Pragma::from_options, and from_options computes the same factory/fragment/import_source/package_name values; (3) grep confirmed no remaining readers of the dropped TransformOptions fields or BunInstall.lockfile_path/save_lockfile_path; (4) bake_body.rs swapping source_map_mode_from_js for SOURCE_MAP_OPTION_MAP.from_js — the new path coerces non-strings via toString before lookup rather than short-circuiting to None, but the net result (None → error branch) is the same for the documented input space. The PR ships a new test only for the logLevel behavior change; the rest relies on existing suites the author lists as passing. Given the breadth (64 files, multiple subsystems, an intentional user-visible change, and struct-field removals), this should get a maintainer's eyes even though nothing looks wrong.
…ader
There were two loader numberings in the binary: `bun_ast::Loader` (0-based,
also the public native-plugin ABI in bundler_plugin.h) and the 1-based
`schema::api::Loader` mirrored by C++ `BunLoaderType*` constants and the JS
builtins' `$LoaderLabelToId`, bridged by `to_api`/`from_api`. The bridge was
lossy: `jsonc`, `sh` and `sqlite_embedded` given through `--loader`, bunfig
`[loader]` or `Bun.build({ loader })` silently became `json`, `file` and
`sqlite`, so a `jsonc` mapping rejected comments.
- `bun_ast::Loader` discriminants are now the only numbering. C++
`BunLoaderType*` and the JS id tables use them (`BunLoaderTypeNone = 255`);
`bun_jsc::BunLoaderType` is the FFI carrier; `schema.rs`, `LoaderExt`
(options_types), `LOADER_API_NAMES` and `LoaderMap` are deleted and loader
maps are `(ext, Loader)` pairs parsed with `Loader::from_string`.
- src/codegen/replacements.ts derives `$Loader*` and `$ImportKind*` from the
Rust enums instead of hand-written lists. The ImportKind list had drifted
(9 labels for 12 variants), so onResolve plugins saw CSS `url()` imports as
`kind: "internal"`; they now get `"url-token"`.
- The public bundler_plugin.h / bun-native-plugin-rs `BunLoader` values were
stale for ids >= 7 (e.g. `BUN_LOADER_TOML = 7` while the runtime sends 8);
corrected and completed. A source lint keeps all copies in sync with
src/ast/loader.rs.
- Tests: loader-map jsonc/json5 by extension (API + CLI), plugin/ResolveKind,
loader-numbering source lint; `Loader` type gains "json5" | "md".
dylan-conway
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bun-native-bundler-plugin-api/bundler_plugin.h`:
- Around line 15-28: Preserve the existing native-plugin loader numeric ABI by
keeping BUN_LOADER_TOML and BUN_LOADER_WASM at their prior values and appending
BUN_LOADER_JSONC and later loaders after the existing assignments. Apply the
same enum/value ordering in
packages/bun-native-bundler-plugin-api/bundler_plugin.h (anchor, lines 15-28),
packages/bun-native-plugin-rs/headers/bun-native-bundler-plugin-api/bundler_plugin.h
(lines 15-32), and packages/bun-native-plugin-rs/src/sys.rs (lines 111-125);
keep src/jsc/bindings/headers-handwritten.h aligned as well.
🪄 Autofix
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: 6ea10a6d-2ac8-48ee-8862-80dbdf0bdf86
📒 Files selected for processing (29)
packages/bun-native-bundler-plugin-api/bundler_plugin.hpackages/bun-native-plugin-rs/headers/bun-native-bundler-plugin-api/bundler_plugin.hpackages/bun-native-plugin-rs/src/sys.rspackages/bun-types/bun.d.tsscripts/build/codegen.tssrc/ast/lib.rssrc/ast/loader.rssrc/bundler/options.rssrc/bunfig/bunfig.rssrc/codegen/replacements.tssrc/jsc/ModuleLoader.rssrc/jsc/NodeModuleModule.rssrc/jsc/bindings/headers-handwritten.hsrc/jsc/lib.rssrc/options_types/bundle_enums.rssrc/options_types/lib.rssrc/options_types/schema.rssrc/options_types/transform_options.rssrc/runtime/api/JSBundler.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/colon_list_type.rssrc/runtime/cli/mod.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/jsc_hooks.rstest/bundler/bundler_loader.test.tstest/bundler/bundler_plugin.test.tstest/bundler/expectBundled.tstest/internal/source-lints/loader-numbering.test.ts
💤 Files with no reviewable changes (2)
- src/options_types/schema.rs
- src/options_types/bundle_enums.rs
| BUN_LOADER_JSONC = 7, | ||
| BUN_LOADER_TOML = 8, | ||
| BUN_LOADER_WASM = 9, | ||
| BUN_LOADER_NAPI = 10, | ||
| BUN_LOADER_BASE64 = 11, | ||
| BUN_LOADER_DATAURL = 12, | ||
| BUN_LOADER_TEXT = 13, | ||
| BUN_LOADER_BUNSH = 14, | ||
| BUN_LOADER_SQLITE = 15, | ||
| BUN_LOADER_SQLITE_EMBEDDED = 16, | ||
| BUN_LOADER_HTML = 17, | ||
| BUN_LOADER_YAML = 18, | ||
| BUN_LOADER_JSON5 = 19, | ||
| BUN_LOADER_MD = 20, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate native-plugin ABI version checks and loader-byte decoding paths.
rg -n -C 4 'ABI_VERSION|BUN_LOADER_|BunLoader|default_loader|OnBeforeParseResult|loader.*from_repr' \
src packagesRepository: oven-sh/bun
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed files and enum diff ---'
git diff --stat
git diff -- packages/bun-native-bundler-plugin-api/bundler_plugin.h \
packages/bun-native-plugin-rs/headers/bun-native-bundler-plugin-api/bundler_plugin.h \
packages/bun-native-plugin-rs/src/sys.rs
printf '%s\n' '--- native plugin loading and loader conversion sites ---'
rg -n -C 5 \
'native plugin|NativePlugin|native_plugin|OnBeforeParseResult|default_loader|loader as u8|transmute.*loader|BunLoader::' \
src packages/bun-native-plugin-rs packages/bun-native-bundler-plugin-api \
-g '*.rs' -g '*.zig' -g '*.cpp' -g '*.h' -g '*.ts' \
| rg -n -m 250 'ABI|version|loader|OnBeforeParse|native.plugin|NativePlugin'
printf '%s\n' '--- likely ABI/version declarations ---'
rg -n -i -C 4 \
'abi|version|compatible|incompatible|struct_size|plugin.*load|load.*plugin' \
packages/bun-native-plugin-rs packages/bun-native-bundler-plugin-api src/bundler src \
-g '*.rs' -g '*.zig' -g '*.cpp' -g '*.h' -g '*.ts' \
| head -n 500Repository: oven-sh/bun
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("packages/bun-native-bundler-plugin-api/bundler_plugin.h"),
Path("packages/bun-native-plugin-rs/headers/bun-native-bundler-plugin-api/bundler_plugin.h"),
Path("packages/bun-native-plugin-rs/src/sys.rs"),
]
for path in files:
text = path.read_text()
pairs = re.findall(r'\b(BUN_LOADER_[A-Z0-9_]+)\s*=\s*(\d+)', text)
print(path)
print(" enum:", pairs)
print(" loader fields:", [
(i + 1, line.strip())
for i, line in enumerate(text.splitlines())
if re.search(r'\b(default_loader|loader)\b', line)
])
print("loader conversion candidates:")
for path in Path(".").rglob("*"):
if not path.is_file() or path.suffix not in {".rs", ".zig", ".cpp", ".cc", ".c", ".h", ".ts"}:
continue
try:
lines = path.read_text(errors="replace").splitlines()
except OSError:
continue
for i, line in enumerate(lines):
if re.search(r'(transmute|from_repr|try_from|default_loader|result\.loader|loader\s*=)', line, re.I):
context = "\n".join(f"{j+1}:{lines[j]}" for j in range(max(0, i-2), min(len(lines), i+3)))
if any(x in context.lower() for x in ("native", "plugin", "onbeforeparse", "loader")):
print(f"{path}:{i+1}\n{context}")
PYRepository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository revision context ---'
git status --short
git rev-parse HEAD
git rev-list --parents -n 3 HEAD
printf '%s\n' '--- loader enums in recent revisions ---'
for rev in HEAD HEAD^ HEAD^^; do
printf '\n[%s]\n' "$rev"
for file in \
packages/bun-native-bundler-plugin-api/bundler_plugin.h \
packages/bun-native-plugin-rs/headers/bun-native-bundler-plugin-api/bundler_plugin.h \
packages/bun-native-plugin-rs/src/sys.rs \
src/ast/loader.rs
do
if git cat-file -e "$rev:$file" 2>/dev/null; then
printf '%s\n' "$file"
git show "$rev:$file" | rg -n \
'BUN_LOADER_|enum Loader|pub enum Loader|JSONC|TOML|XML|from_repr|repr\(' \
| head -n 100
fi
done
doneRepository: oven-sh/bun
Length of output: 6394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- native plugin FFI call graph ---'
rg -n -C 8 \
'JSBundlerPlugin__onBeforeParse|on_before_parse|OnBeforeParseResult|default_loader.*as u8|result.*loader|\.loader.*from_repr|BunLoaderTypeXML|BunLoaderTypeTOML|BunLoaderTypeJSONC' \
src/bundler src/jsc packages \
-g '*.rs' -g '*.cpp' -g '*.h' -g '*.zig' \
| head -n 1200
printf '%s\n' '--- loader representation and conversion definitions ---'
rg -n -C 12 \
'pub enum Loader|enum Loader|BunLoaderType|from_repr|BUN_LOADER_MAX|assert_ffi_layout' \
src/ast/loader.rs src/ast src/bundler/ParseTask.rs src/bundler/bundle_v2.rs \
-g '*.rs' -g '*.h' -g '*.cpp' \
| head -n 1200Repository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- canonical loader representation ---'
cat -n src/ast/loader.rs | sed -n '1,90p'
printf '%s\n' '--- native-plugin result construction and consumption ---'
cat -n src/bundler/ParseTask.rs | sed -n '1828,1870p;2028,2060p;2128,2165p'
cat -n src/bundler/bundle_v2.rs | sed -n '770,795p'
printf '%s\n' '--- exact revision differences for the loader enum ---'
git diff --no-ext-diff --unified=3 HEAD^ HEAD -- \
packages/bun-native-bundler-plugin-api/bundler_plugin.h \
packages/bun-native-plugin-rs/headers/bun-native-bundler-plugin-api/bundler_plugin.h \
packages/bun-native-plugin-rs/src/sys.rs \
src/ast/loader.rs \
src/jsc/bindings/headers-handwritten.hRepository: oven-sh/bun
Length of output: 20040
Preserve the native-plugin loader ABI.
HEAD^ assigned BUN_LOADER_TOML = 7 and BUN_LOADER_WASM = 8. The new enum inserts BUN_LOADER_JSONC = 7, so older native plugins select the wrong loaders when Bun consumes their raw uint8_t values.
Keep existing numeric assignments and append new loaders, or add and enforce an ABI version before decoding loader values. Align packages/bun-native-bundler-plugin-api/bundler_plugin.h, packages/bun-native-plugin-rs/headers/bun-native-bundler-plugin-api/bundler_plugin.h, packages/bun-native-plugin-rs/src/sys.rs, and src/jsc/bindings/headers-handwritten.h.
📍 Affects 3 files
packages/bun-native-bundler-plugin-api/bundler_plugin.h#L15-L28(this comment)packages/bun-native-plugin-rs/headers/bun-native-bundler-plugin-api/bundler_plugin.h#L15-L32packages/bun-native-plugin-rs/src/sys.rs#L111-L125
🤖 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 `@packages/bun-native-bundler-plugin-api/bundler_plugin.h` around lines 15 -
28, Preserve the existing native-plugin loader numeric ABI by keeping
BUN_LOADER_TOML and BUN_LOADER_WASM at their prior values and appending
BUN_LOADER_JSONC and later loaders after the existing assignments. Apply the
same enum/value ordering in
packages/bun-native-bundler-plugin-api/bundler_plugin.h (anchor, lines 15-28),
packages/bun-native-plugin-rs/headers/bun-native-bundler-plugin-api/bundler_plugin.h
(lines 15-32), and packages/bun-native-plugin-rs/src/sys.rs (lines 111-125);
keep src/jsc/bindings/headers-handwritten.h aligned as well.
There was a problem hiding this comment.
No bugs found, but this is an 81-file refactor that renumbers the loader enum across five language boundaries (Rust, two C headers, C++ bindings, JS builtin codegen), changes the public bundler_plugin.h native-plugin API values, flips $LoaderLabelToId from 1- to 0-based, drops the _None/_none sentinels from several enums, and removes user-facing bunfig fields — worth a human pass.
What was reviewed:
- Traced the
$LoaderLabelToIdi+1→ichange end to end:bundle_v2.rssendsbun_ast::Loader as u8/ImportKind as u8(0-based, unchanged),BundlerPlugin.tsindexes$LoaderIdToLabel(still 0-based), and the return path now hitsLoader::from_reprinstead ofapi::Loader::from_raw— both ends move together. - C++ callers of
BunLoaderType(ModuleLoader.cpp,JSCommonJSExtensions.cpp) use only the symbolic constants, so the header renumbering + RustBunLoaderType::NONE = 255stay in lockstep; the new source-lint test pins all four copies tobun_ast::Loader. NpmRegistry::from_urlis a byte-for-byte move of the oldbun_apiparser (already discussed in the resolved thread);Bun__getDefaultLoadernow mapsBunsh → Jsexplicitly, which matches what the removedto_apidid implicitly.
Extended reasoning...
Overview
This PR deletes the bun_api crate and the bun_options_types::schema::api mirror layer, migrating ~80 files to use canonical types (bun_ast::Target/Loader/Level, jsx::Options, SourceMapOption, PackagesOption, DotEnvBehavior, BunInstall/NpmRegistry) directly. It also unifies loader numbering: bun_ast::Loader's 0-based discriminants become the single wire encoding across headers-handwritten.h (BunLoaderType*, was 1-based with None=254), the two public bundler_plugin.h copies (were stale/incomplete), bun-native-plugin-rs/sys.rs, and the JS builtins' $LoaderIdToLabel/$LoaderLabelToId (now derived from the Rust source by replacements.ts instead of hand-maintained). TransformOptions.define/loaders become Vec<(K, V)> pairs instead of parallel arrays; four write-only fields and the [install.lockfile] path/savePath bunfig keys are dropped. One acknowledged behavior change: bunfig logLevel = "info" now means info instead of falling through to error.
Security risks
None introduced. NpmRegistry::from_url is a verbatim move of the existing registry-URL credential splitter (CodeRabbit's userinfo concern was withdrawn as pre-existing behavior). No new parsing of untrusted input, no auth/crypto/permission changes.
Level of scrutiny
High. This is not a mechanical rename — it renumbers an enum that crosses FFI in raw u8 form at half a dozen sites, changes the public native-plugin C API values (BUN_LOADER_TOML 7→8, etc.), flips a codegen offset (${i+1}→${i}) whose consumer is on the other side of a Rust→C++→JS-builtin round-trip, and removes sentinel variants (DotEnvBehavior::_none, jsx::Runtime::_None, api::Loader::_none) that previously distinguished "unset" from "default". I traced the loader/import-kind round-trip (bundle_v2.rs → JSBundlerPlugin.cpp → BundlerPlugin.ts → JSBundler.rs::on_load_async_ok) and the C++ BunLoaderType consumers (ModuleLoader.cpp, JSCommonJSExtensions.cpp) and all use symbolic constants that move with the header, but the number of moving parts and the public-API surface change put this well outside auto-approve territory.
Other factors
The PR adds a source-lint test (loader-numbering.test.ts) that pins the four loader-numbering copies to bun_ast::Loader, plus a plugin/ResolveKind test that exercises $ImportKindIdToLabel, a bunfig logLevel matrix test for the intentional behavior change, and a jsonc/json5 loader-map test — good coverage for the risk areas. The removed Pragma hand-construction in js_bundle_completion_task.rs (now delegated to Pragma::from_options via transform.jsx) and the --compile define-precedence rewrite in build_command.rs both look behavior-preserving but are non-trivial rewrites. CI build #90026 is still in flight per the robobun comment.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/cli/Arguments.rs (1)
1489-1502: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
prev.developmentwhen merging JSX options.Line 1501 overwrites the existing JSX configuration with
development: false. A bunfig configuration that enables development JSX loses that setting when the user also passes a JSX override. Preserveprev.developmentin this merge branch.Based on learnings, the CLI JSX merge must preserve
prev.development, and existing bunfig-driven tests provide the relevant coverage.Proposed fix
runtime: if let Some(runtime) = jsx_runtime { resolve_jsx_runtime(runtime)? } else { prev.runtime }, - development: false, + development: prev.development, side_effects: jsx_side_effects,🤖 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/runtime/cli/Arguments.rs` around lines 1489 - 1502, Update the JSX options merge in the opts.jsx branch to preserve the existing prev.development value instead of forcing development to false. Leave the other override and fallback behavior unchanged.Source: Learnings
src/jsc/VirtualMachine.rs (1)
71-74: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReset
evaluated_as_cjsbefore every entry-point reload.Neither
reload_entry_pointnorreload_entry_point_for_test_runnerresets this field. After a CommonJS load, an ECMAScript-module load retainstrue, so rejection handling reportsuncaughtExceptioninstead ofunhandledRejection. Set it tofalseat the start of both reload methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/VirtualMachine.rs` around lines 71 - 74, Reset evaluated_as_cjs to false at the beginning of both reload_entry_point and reload_entry_point_for_test_runner, before each entry-point reload starts. Preserve the existing assignment during CommonJS evaluation so each reload reflects the current module type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/jsc/VirtualMachine.rs`:
- Around line 71-74: Reset evaluated_as_cjs to false at the beginning of both
reload_entry_point and reload_entry_point_for_test_runner, before each
entry-point reload starts. Preserve the existing assignment during CommonJS
evaluation so each reload reflects the current module type.
In `@src/runtime/cli/Arguments.rs`:
- Around line 1489-1502: Update the JSX options merge in the opts.jsx branch to
preserve the existing prev.development value instead of forcing development to
false. Leave the other override and fallback behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64e14bfb-a052-4773-9424-fb3020b64fff
📒 Files selected for processing (4)
src/jsc/VirtualMachine.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/mod.rssrc/runtime/cli/run_command.rs
| /** `bun_ast::ImportKind::label()`: `ImportKind::Stmt => b"import-statement"`, … */ | ||
| function importKindLabel(variant: string): string { | ||
| const source = readFileSync(join(import.meta.dir, "../ast/lib.rs"), "utf8"); | ||
| const arms = source.match(/pub fn label\(self\) -> &'static \[u8\] \{\s*match self \{([^}]*)\}/)?.[1]; | ||
| const label = arms?.match(new RegExp(`ImportKind::${variant} => b"([^"]*)"`))?.[1]; | ||
| if (label === undefined) | ||
| throw new Error(`replacements.ts: no ImportKind::label() arm for ${variant} in src/ast/lib.rs`); | ||
| return label; |
There was a problem hiding this comment.
🟡 importKindLabel() derives labels from bun_ast::ImportKind::label(), but ImportKind::AtConditional (discriminant 7, emitted for @import "..." supports(...)) has label() => b"", so the generated $ImportKindIdToLabel[7] === "" and $ImportKindLabelToId gains an empty-string key. An onResolve plugin whose filter matches such an import will see args.kind === "" (falsy, and not in the ImportKind d.ts union) — before this PR id 7 read as "url-token" (also wrong, but truthy). Since the PR's goal is a correct ImportKind table and the new plugin/ResolveKind test proves CSS @import records reach onResolve, mapping AtConditional to "import-rule" (same as At, matching esbuild) would be more consistent.
Extended reasoning...
What the bug is
src/codegen/replacements.ts now derives the JS builtins' $ImportKindIdToLabel / $ImportKindLabelToId tables from the Rust ImportKind enum by reading each variant's label() arm. ImportKind::AtConditional (discriminant 7) has label() => b"" at src/ast/lib.rs:93 — an intentionally-empty display string. The importKindLabel() regex b"([^"]*)" uses * (not +), so it happily matches the empty string and returns "" rather than throwing. rustEnumLabels() then populates labels[7] = "", which is not undefined so the density check passes, and the generated tables end up with $ImportKindIdToLabel[7] === "" and $ImportKindLabelToId["": 7].
The code path that triggers it
ImportKind::AtConditional is produced by the CSS parser at src/css/css_parser.rs:812-813 for @import "..." supports(...) (and layer/media-conditional forms). CSS import records reach onResolve plugins via enqueue_on_resolve_plugin_if_needed → match_on_resolve → JSBundlerPlugin__matchOnResolve, which passes import_record_kind as u8 unchanged (bundle_v2.rs:864). On the JS side, runOnResolvePlugins in src/js/builtins/BundlerPlugin.ts does kind = $ImportKindIdToLabel[kindId] and hands args.kind to the user's callback. The PR's own new plugin/ResolveKind test demonstrates that ImportKind::At ("import-rule") and ImportKind::Url ("url-token") from the same CSS parser code path do reach onResolve — there is no kind-based filtering — so AtConditional from the adjacent branch would too.
Why nothing prevents it
The importKindLabel() guard only throws when the regex fails to match (label === undefined), and the rustEnumLabels() density check only rejects undefined slots. An empty string satisfies both. The PR's new plugin/ResolveKind test only uses a plain @import "./other.css" (→ ImportKind::At), not the conditional form, so this case has no coverage.
Impact
A user's onResolve callback receives args.kind === "" for conditional CSS @import — a falsy value that is not a member of the ImportKind union in packages/bun-types/bun.d.ts, and $ImportKindLabelToId gains a nonsensical "": 7 entry. Before this PR the hand-written 9-element table also mislabelled AtConditional (index 7 read as "url-token"), so this is not a strict regression — but it goes from "wrong but a valid, truthy ImportKind string" to "wrong and falsy". Since the PR description explicitly claims to fix the ImportKind table's completeness/correctness for onResolve, and REVIEW.md asks to "fix the whole class in the same PR" for parallel switch arms, this parallel arm is worth addressing here. The trigger is narrow (CSS @import with supports()/layer()/media condition + an onResolve filter that matches its URL), and nothing crashes, so this is a nit rather than a blocker.
Step-by-step example
entry.csscontains@import "./other.css" supports(display: grid);- The CSS parser creates an import record with
kind = ImportKind::AtConditional(discriminant 7). - The bundler enqueues an on-resolve for
./other.css;match_on_resolvecallsJSBundlerPlugin__matchOnResolve(..., 7). runOnResolvePluginscomputeskind = $ImportKindIdToLabel[7]. With this PR, that array slot is"".- The user's
builder.onResolve({ filter: /\.css$/ }, args => ...)fires withargs.kind === "". Anyif (args.kind)orswitch (args.kind)on documentedImportKindvalues behaves unexpectedly.
Suggested fix
Give AtConditional a real label — the natural choice is b"import-rule" at src/ast/lib.rs:93, matching ImportKind::At and esbuild's behavior (esbuild reports both plain and conditional @import as "import-rule"). Alternatively, special-case it in importKindLabel(). Optionally extend the plugin/ResolveKind test with an @import "./cond.css" supports(display: grid); line asserting "import-rule".
| | "json5" | ||
| | "toml" | ||
| | "yaml" | ||
| | "xml" | ||
| | "md" |
There was a problem hiding this comment.
🟡 type ImportKind in this file (line 2745) wasn't updated even though $ImportKindIdToLabel now derives all 12 bun_ast::ImportKind labels — onResolve plugins and metafile imports can now surface "composes" and "html_manifest", which the union doesn't include. The new comment at src/ast/lib.rs:79 says to keep this type in sync by hand, and type Loader was updated here, so this looks like an oversight.
Extended reasoning...
What changed
Before this PR, src/codegen/replacements.ts had a hand-written 9-entry ImportKind array feeding $ImportKindIdToLabel. This PR replaces it with rustEnumLabels("../ast/lib.rs", "ImportKind", importKindLabel), which reads all 12 discriminants of bun_ast::ImportKind and looks up each variant's string in ImportKind::label(). The generated $ImportKindIdToLabel array therefore grows from 9 to 12 entries, adding index 7 → "" (AtConditional), index 9 → "composes", and index 10 → "html_manifest" (index 8 → "url-token" was already present but at the wrong index — the PR description calls that out as a fix).
At the same time, the PR rewrote the comment above ImportKind::label() in src/ast/lib.rs to say:
src/codegen/replacements.ts derives the JS builtins'
$ImportKindIdToLabelfrom the discriminants above andlabel()below; keepImportKindin packages/bun-types/bun.d.ts in sync by hand.
And the PR did update type Loader in packages/bun-types/bun.d.ts (adding "json5" and "md"), showing the .d.ts file was in scope.
What's stale
packages/bun-types/bun.d.ts:2745 still reads:
type ImportKind =
| "import-statement"
| "require-call"
| "require-resolve"
| "dynamic-import"
| "import-rule"
| "url-token"
| "internal"
| "entry-point-run"
| "entry-point-build";Missing "composes" and "html_manifest". This type feeds OnResolveArgs.kind (line ~5684) and the metafile imports[].kind field (lines ~3873/3902).
Step-by-step: how the new values reach user code
bun_ast::ImportKind::Composeshas discriminant 9 andlabel()returnsb"composes".replacements.tsrustEnumLabels()iteratespub enum ImportKind { ... Composes = 9, ... }, callsimportKindLabel("Composes"), which regexesImportKind::Composes => b"composes"out oflabel()and returns"composes".$ImportKindIdToLabel[9]in the JS builtins is now"composes"(previouslyundefined— the old array had length 9).- When the bundler resolves a CSS Modules
composes: foo from './x.module.css'reference, it creates an import record withImportKind::Composesand passes the discriminant (9) torunOnResolvePluginsinsrc/js/builtins/BundlerPlugin.ts:402, which doesargs.kind = $ImportKindIdToLabel[kindId]. - A user's
onResolvecallback receivesargs.kind === "composes", but TypeScript'sOnResolveArgs['kind']doesn't include it.
The same holds for ImportKind::HtmlManifest (discriminant 10, label "html_manifest") which is emitted for <link rel="manifest"> in HTML entrypoints. Both also flow into the metafile's imports[].kind field, which is typed as ImportKind.
Why this is same-PR, not pre-existing
Before this PR, ids 9–11 were out of bounds on the 9-element hand-written array, so $ImportKindIdToLabel[9] was undefined — the strings "composes" / "html_manifest" never reached JS. The .d.ts type and the runtime table had the same 9-string set. This PR is what makes them diverge.
Impact & fix
Types-only; no runtime effect. A TypeScript user switching on args.kind won't get exhaustiveness for the new values, and args.kind === "composes" will error under --strict as comparing to a value not in the union. Fix: add | "composes" | "html_manifest" to type ImportKind at bun.d.ts:2745. (The empty-string label for AtConditional is internal and probably shouldn't be added to the public type.)
Anchoring on the type Loader hunk since type ImportKind itself is unmodified in the diff.
2 participants
What does this PR do?
bun_options_types::schema::apiwas a hand-maintained mirror of types the codebase already has —Target, the JSX runtime, sourcemap/packages modes, log level, dotenv behavior, and a secondLoaderenum with its own numbering — left over from the old peechy schema, with every consumer converting throughfrom_api/to_apishims. This removes the whole layer (follow-up to #37068 / #37081);bun_options_typesno longer has aschemamodule and thebun_apicrate is gone.Option types.
TransformOptions(the CLI/bunfig option bag) holdsbun_ast::Target,bun_ast::Level,jsx::Options,SourceMapOption,PackagesOption,Option<DotEnvBehavior>directly;define/serve_define/loadersbecome ordered pairs instead of parallel arrays; write-only fields and unread[install.lockfile] path/savePathare dropped.BunInstall/NpmRegistry/Camove toinstall_config.rs,NpmRegistry::from_urlreplacesbun_api. Deleted:TargetExt,MessageLevel,JsxRuntime,Jsx,SourceMapMode,PackagesMode,StringMap,LoaderMap,LoaderExt::{to_api,from_api},LOADER_API_NAMES,source_map_mode_jsc.rs,jsx::Runtime::_None, a duplicatePragmabuilder inBun.build, and theEnvBehavior/StringPointeralias paths.One loader numbering. There were two in the binary:
bun_ast::Loader(0-based; also the public native-plugin ABI inbundler_plugin.h) and the 1-basedapi::Loadermirrored by C++BunLoaderType*and the JS builtins'$LoaderLabelToId. Everything now usesbun_ast::Loader's discriminants (BunLoaderTypeNone = 255,bun_jsc::BunLoaderTypeas the FFI carrier), andsrc/codegen/replacements.tsderives the JS$Loader*/$ImportKind*tables from the Rust enums instead of hand-written lists. Native plugins are unaffected — their numbering is the one that stays.Bugs this fixes along the way (each with a test that fails on current release):
jsonc(andsh,sqlite_embedded) given via--loader, bunfig[loader]orBun.build({ loader })silently degraded tojson/file/sqlitethrough the lossy bridge — a.data → jsoncmapping rejected comments.onResolveplugins saw CSSurl()imports askind: "internal"(now"url-token").bundler_plugin.h/bun-native-plugin-rs'sBunLoaderwere stale for ids ≥ 7 (BUN_LOADER_TOML = 7while the runtime sends 8, etc.); corrected and completed, and a source lint now keeps every copy in sync withsrc/ast/loader.rs. (bun-native-plugin-rswill want a version bump when next published.)logLevel = "info"(accepted but undocumented) fell through to the error level; it now means info (docs/message updated).How did you verify your code works?
cargo clippy --workspace, Windows cross-check, debug build.--define, bunfig[define]/jsx*/logLevel/[loader],--compiledefine precedence,--sourcemap/--packages,Bun.build({ jsx }), every pluginonLoadloader string (incl.jsx, now id 0), and onResolve kinds.bundler_loaderloader-map jsonc/json5 (API + CLI backends),plugin/ResolveKind,cli.test.tslogLevel rows,test/internal/source-lints/loader-numbering.test.ts.