Conversation
The type attribute used to be carried in ParsedPath.import_tag, which the
two export-from branches checked to reject it. Since it moved to
ParsedPath.loader those branches no longer saw it, so
`export { default as x } from "./a.bin" with { type: "text" }` (and the
export * forms) silently loaded the file with its extension's loader, both
at runtime and in the bundler.
Set the record's loader for export-from statements the same way import
statements do (validating the names imported from sqlite/text/file modules),
and print the attribute back out for export-from statements on bun targets,
including the ModuleInfo fetch parameters used by bun test --isolate and
bytecode builds, so the runtime module loader sees it too. The remaining
import_tag check only ever matched bunBakeGraph; its messages now say so.
Bumps the runtime transpiler cache version since the printed output of
re-exports changed.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
|
Status: fix pushed, self-review in progress. Reproduced on the released 1.4.0 and on main: with Fail-before / pass-after (released binary vs this branch's debug build):
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes parser/printer semantics for every export ... from statement and the ModuleInfo fetch-parameters that JSC consumes, a human sign-off would still be worthwhile.
What was reviewed:
set_import_record_loaderrefactor is behavior-preserving forS::Import(same error strings, same alias/loc iteration);original_nameis populated byparse_export_clauseso the export-from validation checks the imported name, not the re-exported one.- Split
ws!output inprint_import_record_type_attributeis byte-identical to the old inline match in both normal and minified modes (_ws_minifyjust strips spaces). import_record()returns&'a ImportRecorddetached from&self, so holdingrecordacross the new&mut selfprinter/module_info calls is sound.- Moving
add_import_recordabove theis_macro/import_tagchecks in theexport { } frombranch doesn't change the type-only early-return path (still before the record is created).
Extended reasoning...
Overview
The PR makes export ... from "x" with { type: "..." } honor the type attribute the same way import ... from already does. It touches the JS parser (p.rs, parse_stmt.rs) to copy ParsedPath.loader onto the import record for both export-from forms and to share the name-validation logic with imports; the JS printer (lib.rs) to print the with { type } clause and record matching FetchParameters in ModuleInfo for S::ExportStar and S::ExportFrom; and bumps the RuntimeTranspilerCache version. Tests cover runtime, bundler (bun and node targets), --no-bundle, the dev server, and the bun test --isolate ModuleInfo path.
Security risks
None identified. The change routes an already-parsed attribute to the same loader machinery imports already use; no new untrusted-input parsing, no filesystem/network surface.
Level of scrutiny
High. The parser and printer sit on the hot path of every module Bun runs or bundles, and ModuleInfo feeds JSC's module record directly. The refactor also collapses two per-loader match blocks into shared helpers, so the byte-identity claim for existing S::Import output matters (verified: _ws_minify strips all spaces, so the split ws! calls produce the same normal and minified bytes). The design decision — honor rather than reject — is well-argued (spec allows it, Node requires it for JSON, Bun has silently accepted it since v1.2.5), but it is a decision a maintainer should ratify.
Other factors
Test coverage is unusually thorough: each of the three re-export forms is exercised at runtime, in the bundler for both targets, in transpile-only output, in the dev server, and under --isolate (which diffs ModuleInfo against JSC's own parse in debug builds). The two error messages that changed ("type" → "bunBakeGraph") are more accurate given import_tag is only ever set for that attribute since #16624. The add_import_record reorder in the export { } from branch was needed so the record index exists before set_import_record_loader is called; the type-only early return still precedes it, and the macro/bake-graph checks only add errors without returning, so no observable ordering change.
|
Updated 6:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 8c590f8 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 38407That installs a local version of the PR into your bun-38407 --bun |
|
Closing in favor of #40836, which carries the Checked against a debug build of #40836: 12 of the 14 cases added here pass as written (the runtime re-exports, The two remaining cases only check the parser message for a named re-export that a text or sqlite module does not provide, such as |
1 participant
Problem
export { default as text } from "./data.bin" with { type: "text" },export * as ns from ... with { type }andexport * from ... with { type }ignore the attribute: the file is loaded with the loader its extension implies (filefor.bin, so the re-export is a path string).import text from "./data.bin" with { type: "text" }honors it. Affectsbun run,bun build,bun build --no-bundleand the dev server;with { type: "sqlite" }on a re-export bundles the database as a file asset and exports its path.parse_path()(src/js_parser/parse/mod.rs:1396) stores thetypeattribute inParsedPath.loader. Import statements copy it onto the import record (validate_and_set_import_type, src/js_parser/p.rs). The two export-from branches in src/js_parser/parse/parse_stmt.rs (export *at ~1248,export { .. } fromat ~1303) never look atParsedPath.loader: they only checkParsedPath.import_tag, which since d502df3 (Supportimport with { type: "json" }and others #16624, v1.2.5) is set forbunBakeGraphalone. Before that commit the attribute lived inimport_tagand these branches rejected it; after it the attribute falls through.with { type }clause back out forS::Import, so even with the record fixed,bun run(which transpiles and lets JSC's module loader read the attribute) would still drop it on re-exports.Fix
ParsedPath.loader, through a newset_import_record_loadershared with import statements (p.rs). It also keeps the existing name validation for sqlite (default/db) and text/file (default); forexport { a as b } fromthe name imported from the module isClauseItem.original_name, for imports it isalias. The remainingimport_tagchecks only ever fire forbunBakeGraph, so their messages now say that instead of claimingtypeis not allowed.S::ExportStarandS::ExportFromprint the samewith { type: "..." }clause asS::Importand record the sameFetchParametersin the ModuleInfo export records. The three statement kinds now shareprint_import_record_type_attribute/module_info_fetch_parameters/import_attribute_type_name, which replace the two per-loader match blocks that were inlined in theS::Importbranch (output is byte-identical for imports, including the minified form).with { type: "json" }on JSON re-exports, and Bun has acceptedexport { default } from "./x.json" with { type: "json" }since v1.2.5; a hard error would break that code. Everything downstream of the parser is already per import record (the bundler resolves, parses and externalizes records byrecord.loaderregardless of which statement created them, and converts export-from into imports before printing), so a re-export record with a loader behaves exactly like an import record with one.bun test --isolate(and bytecode builds) the module record is built from ModuleInfo instead of JSC's own parse, so the export records need the matching fetch parameters; debug builds diff the two and reject the module if they disagree, which the--isolatetest exercises.export { default as x } from,export * as ns from,export * from, sqlitedefault/dbre-export, the two validation errors, and abun test --isolaterun covering the ModuleInfo path (the JSON-with-matching-attribute case passes both ways and guards the decision above).--no-bundleoutput keeping the clause on all three statement kinds..dbextension, since.sqlitealready maps to the sqlite loader on its own)..htmlfile with the text loader.cargo clippyon bun_js_parser / bun_js_printer is clean.Background
with { type: "..." }clause onimportandexport ... fromstatements (ES2025). Bun uses thetypevalue to pick a loader for the target file instead of the one implied by its extension; Bun's parser records that choice asImportRecord.loaderon the statement's import record. (type: "macro"andbunBakeGraphare separate attributes handled byParsedPath.is_macro/ParsedPath.import_tag.)require,import()), holding the path, how it was referenced and the loader to use. The bundler resolves and parses files from these records, so a loader set on a record is honored no matter which statement produced it.bun runtranspiles each file and hands the printed source to JSC. JSC collects the attributes of every module request and passes them as fetch parameters to Bun's module loader (moduleLoaderFetchin ZigGlobalObject.cpp), which is where the runtime reads thetypeattribute. Anything the printer drops is therefore lost at runtime.bun test --isolate, bytecode builds) JSC builds the module record from it instead of re-analyzing the source, and each requested module and export entry carries fetch parameters that have to match what analyzing the printed source would give.