bundler(options): collapse bool pairs into OfflineMode/CompileMode enums - #36768
Conversation
Two related type-hardening refactors with no behavior change: install_preference: OfflineMode Replaces the prefer_offline_install + prefer_latest_install bool pair on the bundler and resolver BundleOptions. Every write site already decoded these from an OfflineMode and prefer_latest_install was write-only dead state. The one read site (resolver disk-cache lookup) now checks the enum directly. compile_mode: CompileMode Replaces the compile + compile_to_standalone_html bool pair on the bundler BundleOptions and LinkerOptions. The two were mutually exclusive by construction (both build_command and Bun.build clear compile when setting compile_to_standalone_html) so a three-state enum makes the invariant unrepresentable-if-wrong. The resolver projection keeps its compile: bool and is fed compile_mode.is_executable(). Net -1 line across 18 files; three write sites each drop ~10 lines of duplicated enum-to-bool decoding plus the comments explaining why the resolver lacks prefer_latest_install.
|
Type-hardening refactor with no behavior change. Verified against existing bundler test suites (compile, standalone HTML, Bun.build API, CLI). The two pre-existing test failures noted in the PR body reproduce identically on main at 2569ae3. Self-review pass clean; both review comments (doc-comment length, |
WalkthroughThe change replaces compile and install-preference booleans with ChangesTyped mode migration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — mechanical type-hardening refactor; the one inline finding is a cosmetic placement nit.
Extended reasoning...
Overview
Two bool-pair → enum collapses in bundler option structs across 18 files: (prefer_offline_install, prefer_latest_install) → install_preference: OfflineMode, and (compile, compile_to_standalone_html) → compile_mode: CompileMode. Every read site is a 1:1 substitution (.compile → .compile_mode.is_executable(), .compile_to_standalone_html → .compile_mode.is_standalone_html(), prefer_offline_install → install_preference == Offline); prefer_latest_install was write-only and is removed.
Semantic equivalence checks
I traced every changed read/write site:
- Both write paths (
build_command.rs,js_bundle_completion_task.rs) previously setcompile = falsewhenevercompile_to_standalone_html = true, so the two bools were already mutually exclusive; the new enum encodes exactly that. - Defaults line up:
(false, false)→CompileMode::None;prefer_offline_install = false→OfflineMode::Online(now#[default]). - The resolver-side
BundleOptions.compile: boolis intentionally kept and fedcompile_mode.is_executable()intranspiler.rs; the two remainingc.resolver().opts.compilereads ingenerateChunksInParallel.rstherefore behave identically. - Grepped the tree for stragglers: remaining
compile_to_standalone_htmlidentifiers are all local variables / theHTMLLoaderstruct field, populated fromcompile_mode.is_standalone_html().
Security risks
None. Pure internal type refactor of build-configuration state; no user input parsing, no I/O, no auth/crypto surface.
Level of scrutiny
Wide (18 files) but shallow — each hunk is a single-expression substitution against a Copy enum with two matches! helpers. The Rust type system guarantees no site was missed (removed fields would fail to compile). Existing bundler test suites (bundler_compile, standalone, bundler_html, bun-build-api, cli, bun-build-compile) exercise both Executable and StandaloneHtml modes per the PR body.
Other factors
The one bug-hunter finding is a code-organization nit (CompileMode inserted between SourceMapOption and its impl); zero behavioral impact. The comment-cop bot flag on the 3-line CompileMode doc comment looks like a false positive — it's an ordinary enum description, not a workaround justification. Neither warrants blocking.
|
Closing: this is a no-behavior-change type-hardening refactor, so there is no regression test that can distinguish before from after. Both bool pairs were already mutually exclusive at every write site (the Happy to reopen if a maintainer wants to land it as a cleanup; the branch is |
There was a problem hiding this comment.
LGTM — mechanical type-hardening refactor; the earlier nit about CompileMode placement was addressed in 9d0ff35.
What was reviewed:
- Traced every
compile/compile_to_standalone_htmlread site →is_executable()/is_standalone_html()are 1:1 equivalent given both writers clearedcompilewhen setting standalone HTML. - Resolver's sole
prefer_offline_installreader now checks== OfflineMode::Offline, matching how every writer derived the bool;prefer_latest_installconfirmed write-only. - New
Defaultderives (OfflineMode::Online,CompileMode::None) match the priorfalse/falsedefaults;for_worker/LinkerOptions::default/resolver projection all updated consistently.
Extended reasoning...
Overview
This PR collapses two pairs of mutually-exclusive booleans into single enums across the bundler option structs: prefer_offline_install + prefer_latest_install → install_preference: OfflineMode, and compile + compile_to_standalone_html → compile_mode: CompileMode. It touches 18 files but every change is a mechanical field rename or a bool → enum.is_variant() substitution. Net -1 line.
Security risks
None. This is a pure type refactor of internal option-struct fields with no user-facing surface, no parsing, no I/O, and no security-relevant logic.
Level of scrutiny
Low-to-medium. While 18 files are touched, the Rust type system guarantees every read/write site was updated (a missed site would fail to compile). I traced each semantic change:
- Every former
options.compileread becomescompile_mode.is_executable(); everycompile_to_standalone_htmlread becomescompile_mode.is_standalone_html(). The two writers (build_command.rs,js_bundle_completion_task.rs) previously enforced mutual exclusion by clearingcompilewhen setting standalone HTML, so the tri-state enum is exactly equivalent. - The resolver's one reader of
prefer_offline_installnow checks== OfflineMode::Offline— identical to how all three writers derived the bool.prefer_latest_installwas confirmed write-only (dead field removal). - Defaults preserved:
#[default] Online/#[default] Nonematch the priorfalseinitializers infrom_api,for_worker,LinkerOptions::default, and the resolver'sBundleOptions::default. - The resolver-side
compile: boolprojection is fedcompile_mode.is_executable(), which isfalseforStandaloneHtml— matching prior behavior where writers clearedcompilein that branch.
Other factors
The author verified against six existing bundler test suites (compile, standalone HTML, Bun.build API, CLI, HTML) with only pre-existing failures that reproduce on main. My earlier nit about CompileMode splitting SourceMapOption from its impl was addressed in 9d0ff35, and the comment-cop feedback about the doc comment was addressed in 7151926. Both inline threads are resolved. This is a textbook "make illegal states unrepresentable" refactor with no behavior change.
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 `@src/bundler/linker_context/postProcessJSChunk.rs`:
- Around line 99-101: The ESM bytecode module_info eligibility condition is
duplicated across four sites. Add a shared LinkerOptions::wants_module_info()
helper containing the generate_bytecode_cache, ESM output_format, and executable
compile_mode checks, use it in postProcessJSChunk.rs#L99-L101, and replace the
inline conditions in OutputFileListBuilder.rs#L130-L137 and
generateChunksInParallel.rs#L452-L455 and `#L1102-L1105` with the helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb47d18c-4d78-4f16-a56c-8e8abf031790
📒 Files selected for processing (18)
src/bundler/HTMLImportManifest.rssrc/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/bundle_v2.rssrc/bundler/linker_context/OutputFileListBuilder.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/generateCompileResultForHtmlChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/options_types/offline_mode.rssrc/resolver/options.rssrc/resolver/resolver.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/production.rssrc/runtime/cli/build_command.rssrc/runtime/cli/repl_command.rssrc/runtime/cli/run_command.rs
There was a problem hiding this comment.
LGTM — mechanical bool-pair → enum refactor, prior nit addressed.
Checked that every former options.compile / compile_to_standalone_html read maps to compile_mode.is_executable() / .is_standalone_html() with identical truth tables; the resolver's opts.compile: bool projection and CLI-level ctx.bundler_options.compile are intentionally unchanged.
Verified prefer_latest_install was write-only (no readers in tree) and OfflineMode::default() == Online matches the old prefer_offline_install: false default.
Confirmed LinkerOptions now copies compile_mode once where it previously copied both bools separately, and sync_resolver_opts() runs after all compile_mode mutations in both build_command.rs and js_bundle_completion_task.rs.
Extended reasoning...
Overview
Two type-hardening refactors across 18 files in the bundler/resolver/CLI: (1) prefer_offline_install: bool + dead prefer_latest_install: bool → install_preference: OfflineMode, and (2) compile: bool + compile_to_standalone_html: bool → compile_mode: CompileMode enum on BundleOptions and LinkerOptions. The resolver-side BundleOptions.compile: bool and CLI-level ctx.bundler_options.compile: bool are kept as-is by design.
Security risks
None. No parsing of untrusted input, no auth/crypto/permissions, no FFI or memory-safety surface. Pure internal option-struct reshaping.
Level of scrutiny
Medium — 18 files is broad, but every change is a mechanical field rename/accessor swap that the Rust compiler enforces (a missed read of a removed field would fail to build). I traced each read site: is_executable() ↔ old compile, is_standalone_html() ↔ old compile_to_standalone_html, == OfflineMode::Offline ↔ old prefer_offline_install. Defaults (CompileMode::None, OfflineMode::Online) match the old false defaults. The bundle_v2.rs linker-options copy correctly folds the two removed bool copies into one compile_mode copy. Grepped for stragglers: remaining compile_to_standalone_html / .compile hits are local variable names, the resolver bool, or the CLI flag — all intentional.
Other factors
This is the fifth PR in a batch applying the same "collapse dependent bool pairs into enums" pattern (#36762/64/65/66 already merged). My earlier placement nit (CompileMode wedged between SourceMapOption and its impl) was addressed in 9d0ff35 — the diff now shows CompileMode after SOURCE_MAP_OPTION_MAP. The comment-cop finding was addressed in 7151926. Existing bundler test suites (bundler_compile, standalone, bundler_html, bun-build-api, cli, bun-build-compile) exercise all three CompileMode variants via both CLI and Bun.build, and a no-behavior-change refactor cannot have a distinguishing regression test.
|
Follow-up: while adding coverage for the |
Two related type-hardening refactors in the bundler option structs. No behavior change.
install_preference: OfflineModeBundleOptions(both the bundler and resolver copies) carriedprefer_offline_install: boolplus, on the bundler side,prefer_latest_install: bool. Every write site (run_command.rs,repl_command.rs,bake/production.rs) derived both from a singleOfflineModevalue:prefer_latest_installwas write-only: nothing in the tree reads it.(true, true)would have been contradictory but the type allowed it.Now both structs store
install_preference: OfflineModedirectly. The one reader (resolver auto-install disk-cache lookup) checks== OfflineMode::Offline. The three write sites each drop ~10 lines of enum-to-bool decoding plus the comments explaining why the resolver lackedprefer_latest_install.compile_mode: CompileModeBundleOptionsandLinkerOptionseach carriedcompile: boolandcompile_to_standalone_html: bool. The two are mutually exclusive by construction: bothbuild_command.rsandjs_bundle_completion_task.rsexplicitly clearcompilewhen they setcompile_to_standalone_html:Now both structs store:
with
is_executable()/is_standalone_html()helpers. The(true, true)state is no longer representable. The resolver'sBundleOptions.compile: boolprojection is kept as-is (resolver never cares about standalone HTML) and is fedcompile_mode.is_executable().The CLI-level
ctx.bundler_options.compile: boolis unchanged: that is the raw--compileflag before HTML/browser detection decides which mode it becomes.Why
Making illegal states unrepresentable removes a class of drift bugs where one bool gets updated and the other doesn't, and it deletes the cross-file comments that existed only to explain which struct had which subset of the bools.
prefer_latest_installwas already dead; this removes it rather than leaving it to bit-rot.Verification
No behavior change; verified against existing coverage:
test/bundler/bundler_compile.test.ts(60 pass, 1 pre-existing fail on main:HelloWorldWithProcessVersionsBun)test/bundler/standalone.test.ts(23 pass; coversCompileMode::StandaloneHtmlvia both CLI andBun.build)test/bundler/bundler_html.test.ts(22 pass)test/bundler/bun-build-api.test.ts(49 pass)test/bundler/cli.test.ts(16 pass)test/bundler/bun-build-compile.test.ts(11 pass, 1 pre-existing timeout on main:compile with relative outfile paths)Net -1 line across 18 files.