Conversation
The dataurl and base64 loaders were accepted by the option validator and
documented in the bundler docs, but emitted an empty-string module with
success: true and no warning. A loader config like {".png": "dataurl"}
(the example shown in docs/bundler/index.mdx) produced var x_default = ""
for any input.
Implement both loaders in the ParseTask switch, matching esbuild:
base64 standard base64 encoding of the file bytes, as a string
dataurl a data: URL with MIME type from the file extension (falling
back to a binary-byte content sniff for unknown extensions),
encoded as percent-escaped or base64, whichever is shorter
Both reuse existing helpers: bun_base64::encode for the base64 loader
and DataURL::encode_string_as_shortest_data_url for the dataurl loader.
Also mark both loaders as handles_empty_file so an empty input yields
an empty string rather than {} / undefined.
Enables five previously-skipped tests ported from esbuild's loader suite
and updates the esbuild comparison docs.
|
Status: diff is green, CI red is unrelated flake; needs a maintainer to merge Reproduced with: USE_SYSTEM_BUN=1 bun test test/bundler/bundler_loader.test.ts -t "loader-base64|loader-dataurl"
# 10 fail (emit "", CSS url() emits resolved path)
bun bd test test/bundler/bundler_loader.test.ts -t "loader-base64|loader-dataurl"
# 10 passThe bundler loader tests ( Also addressed from review:
Deferred to #36334 (pre-existing, different dispatches, not the bundler
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesBase64 and dataurl loaders now classify assets as pure data, infer MIME types, encode JavaScript and CSS imports, and handle empty files. Bundler tests cover imports, CSS URLs, DCE, binary content, and unknown extensions. Documentation and loader validation lists were updated. Base64 and dataurl loader support
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:13 AM PT - Jul 29th, 2026
❌ @robobun, your commit 2a5be9b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36327That installs a local version of the PR into your bun-36327 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Both emit a pure E::String lazy export exactly like Loader::Text does, so they belong in the NoSideEffectsPureData arm of Loader::side_effects. Without this an unused base64/dataurl import is kept in the bundle.
Build the data URL for CSS url() inlining in the same bump allocation as the base64 payload, reusing the encoded bytes (the JS module exports the base64 tail, CSS gets the full data: URL). Matches esbuild LoaderBase64 and the sibling dataurl/text/md arms.
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/ParseTask.rs`:
- Line 1340: Remove the non-informative `// TODO:` comment in the affected
branch of `ParseTask`; do not add a replacement unless there is a specific
compatibility invariant that must be documented.
🪄 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 Plus
Run ID: 72a4566a-45bc-4be2-bf80-5e99f0125d29
📒 Files selected for processing (7)
docs/bundler/esbuild.mdxdocs/bundler/loaders.mdxsrc/ast/loader.rssrc/bundler/ParseTask.rstest/bundler/bundler_loader.test.tstest/bundler/esbuild/loader.test.tstest/bundler/expectBundled.ts
by_extension_no_default is case-sensitive with all-lowercase keys, so LOGO.PNG missed the table and fell through to the content sniff.
There was a problem hiding this comment.
No issues found in this pass — the uppercase-extension fix in 2a5be9b and the earlier type/docs updates address everything raised. That said, this is a new user-facing bundler feature (~90 lines of native code in get_ast, new MIME-sniff helper, url_for_css wiring) with a few design calls a maintainer should sign off on: the intentional full-file-UTF-8 vs 512-byte-sniff asymmetry, and the three sibling dispatches (transpiler.rs, jsc_hooks.rs, BOM-stripping in resolver/fs.rs) deferred to #36334 rather than fixed here.
What was reviewed:
- Both new
Loader::Base64/Loader::Dataurlarms inParseTask.rs— arena allocation,url_for_css, lazy-export AST shape mirror the existingTextarm. guess_mime_type_for_data_url: extension lowercasing,;charset=utf-8append only fortext/*without an existing charset param, WHATWG binary-byte sniff fallback.handles_empty_file/side_effectsadditions and the DCE test that exercises them.- Test coverage: loader map + import attribute, text/binary, known/unknown ext, empty file, CSS
url(), uppercase ext, plus the five un-skipped esbuild-ported tests.
Extended reasoning...
Overview
Implements the base64 and dataurl bundler loaders that were previously accepted but stubbed to an empty string. Touches src/bundler/ParseTask.rs (new guess_mime_type_for_data_url helper + two new get_ast match arms, ~90 lines), src/ast/loader.rs (adds both variants to handles_empty_file and side_effects), packages/bun-types/bun.d.ts, three docs pages, and the bundler test suite (new tests in bundler_loader.test.ts, one todo marker in esbuild/loader.test.ts, and adds both loader names to expectBundled.ts's supported list).
Security risks
None identified. The new code encodes already-read file bytes into a string literal in the output AST; input is bundler source files, not untrusted network data. No auth, crypto, path-traversal, or resource-limit surface. Buffer sizing for the base64 arm uses bun_base64::encode_len and slices into a bump-allocated buffer of exactly prefix_len + encode_len.
Level of scrutiny
Medium-high. This is production bundler code on the get_ast hot path and a user-facing feature (fixes #20917, matches esbuild semantics). It went through five rounds of inline review here — every point was either applied (Loader type union in .d.ts and index.mdx, extension lowercasing + test) or explicitly deferred with justification (full-file UTF-8 check kept intentionally; transpiler.rs/jsc_hooks.rs/BOM-stripping tracked in #36334 as pre-existing). The implementation itself is straightforward and mirrors the sibling Loader::Text arm, but the deferred-sibling scope decision and the sniff-semantics divergence from esbuild are the kind of calls REVIEW.md flags for maintainer confirmation ("fix the whole class in the same PR" vs. scope-tight follow-up).
Other factors
Test coverage is thorough: import-attribute and loader-map entry points, text and binary payloads, known/unknown/uppercase extensions, empty files, CSS url() inlining for both loaders, and DCE of unused imports (verifying the side_effects addition). Five previously-skipped esbuild-ported tests are enabled by adding the loader names to expectBundled's allowlist. The one newly-todo'd test (RequireCustomExtensionPreferLongest) is for a separate multi-dot-extension gap and is commented as such. Given the feature scope and the deferred follow-ups, I'm deferring rather than approving so a maintainer can confirm the #36334 split is acceptable.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-29, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#20917) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Fixes #20917
Problem
Bun.build/bun buildaccept thedataurlandbase64loader values (vialoader: {".png": "dataurl"},import x from "./f" with {type: "base64"}, or--loader .png:dataurl) but silently emit an empty-string module withsuccess: trueand no warning:This is the exact
loaderexample shown indocs/bundler/index.mdx, so users following the docs get silent data loss.For reference, esbuild with the same input:
Cause
The
get_astloader switch insrc/bundler/ParseTask.rshad a stub arm:so both loaders returned a lazy-export AST with an empty
E::Stringinstead of encoding the source.Fix
Implement both loaders, matching esbuild semantics:
base64: standard base64 of the raw file bytes as a string, viabun_base64::encode.dataurl:data:URL with MIME type derived from the file extension (viaMimeType::by_extension_no_default, with a whatwg binary-byte content sniff for unknown extensions), encoded as percent-escaped or base64 via the existingDataURL::encode_string_as_shortest_data_url. The resulting URL is also set asurl_for_cssso CSSurl(...)references resolve inline.Loader::handles_empty_filegainsBase64 | Dataurlso an empty input yields""/"data:<mime>,"rather than{}.Loader::Bunshis left unchanged here; the shell-script bundling path is a separate feature.The esbuild comparison table in
docs/bundler/esbuild.mdxis updated (dataurlandbase64are no longer listed as unimplemented), anddocs/bundler/loaders.mdxgains entries for both loaders.Verification
New tests in
test/bundler/bundler_loader.test.tscover both loader config andwith {type: ...}import attributes, text and binary inputs, known and unknown extensions, and empty files.This also enables five previously-skipped tests ported from esbuild's loader suite by adding
base64anddataurltoexpectBundled's supported-loader list:loader/RequireCustomExtensionBase64loader/RequireCustomExtensionDataURLloader/AutoDetectMimeTypeFromExtensionloader/Base64CommonJSAndES6loader/DataURLCommonJSAndES6loader/RequireCustomExtensionPreferLongestis markedtodo(it tests multi-dot extension matching in theloadermap, which is a separate gap).no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/esbuild/loader.test.ts