Repository navigation
Conversation
…o parse The transpile-only CSS path parsed without an import record list, so the parser rejected every url() token with "Unexpected token". Collect the records during parse and hand them to the printer so url() values are printed back as written, the same way @import already is.
|
Warning Review limit reached
Next review available in: 2 minutes 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 (2)
Comment |
|
Reproduced on bun 1.4.0 and current main with a one-line stylesheet: Same result for unquoted and Fix is in this PR (#38528): |
There was a problem hiding this comment.
LGTM — small, well-scoped fix that wires the CSS url() import-record list through the --no-bundle transpile path.
What was reviewed:
build_css_outputnow passes a localVec<ImportRecord>toStyleSheet::parseandImportInfo::init_outside_of_bundler(&records)toto_css, matching the established pattern insrc/css_jsc/css_internals.rs.- Confirmed
get_import_record_urlfalls through torecord.path.textwhensource_indexis invalid, so URLs round-trip as written; the emptyast_urls_for_css/ast_unique_key_for_additional_fileslices are only indexed inside theis_valid()branch. - The 9 new tests cover quoted/unquoted/data:/fragment/
@font-face/custom-property/image-set()/@import-alongside/--minify, usetempDir/bunEnv, drain pipes concurrently, and assert exact output before exit code.
Extended reasoning...
Overview
Two files: a ~6-line functional change in src/bundler/transpiler.rs::build_css_output (the --no-bundle CSS path) plus a 9-case describe.concurrent block in test/bundler/cli.test.ts. Before, StyleSheet::parse was called with None for the import-record list, so Parser::add_import_record returned an unexpected-token error on the first url(). Now a local Vec<ImportRecord> collects the records and is handed to the printer via ImportInfo::init_outside_of_bundler, exactly as src/css_jsc/css_internals.rs already does for the internal CSS test binding.
Security risks
None. This is a parse/print plumbing fix inside the offline --no-bundle transpiler path — no I/O, network, auth, or filesystem writes are introduced. The URL strings are round-tripped verbatim from the input stylesheet; nothing is resolved or opened.
Level of scrutiny
Low-to-medium. The change is mechanical: replace two None arguments with the record list, mirroring an existing in-tree call site. I verified in src/css/printer.rs that get_import_record_url only indexes into the (empty) ast_urls_for_css / ast_unique_key_for_additional_file tables when record.source_index.is_valid(), which never holds here (parse is called with Index::INVALID and nothing resolves), so init_outside_of_bundler's empty tables are safe. The Vec and the source it borrows (entry.contents(), backed by the arena per the existing SAFETY comment at the call site) both remain in scope through to_css.
Other factors
Tests follow repo conventions well: describe.concurrent, tempDir with using, Bun.spawn with bunEnv/bunExe, Promise.all over stdout/stderr/exited, exact toBe on stdout, exit-code asserted last. The variant matrix is thorough for a bug that previously failed on any url(). The PR description is precise about mechanism and explicitly scopes out the pre-existing .module.css panic, which is unrelated to this change. No prior human review comments to address and no CODEOWNERS on these paths.
Problem
bun build --no-bundle x.cssexits 1 witherror: Unexpected token: ./img.png parsingas soon as the stylesheet contains aurl(), quoted, unquoted ordata:alike.bun build x.css(bundled) works, and a--no-bundlestylesheet that only has@importworks.Transpiler::build_css_output(src/bundler/transpiler.rs). It calledStyleSheet::parsewith no import record list. bun_css stores everyurl()as an index into that list, and when there is no listParser::add_import_record(src/css/css_parser.rs) returns an unexpected-token error for the url, which fails the whole parse.nullas well.Fix
build_css_outputcollects the records into a localVec<ImportRecord>while parsing and passesImportInfo::init_outside_of_bundler(&records)toto_css.--no-bundlepath resolves anything, so the records keep an invalidsource_index, and for such records the printer'sget_import_record_urlreturnsrecord.path.text, the url exactly as written. That is the behavior@importalready has on this path, and it is the same parse/print pairing the in-tree CSS test binding uses (src/css_jsc/css_internals.rs).bun build --no-bundle keeps css url() references as writtenblock in test/bundler/cli.test.ts (9 cases: quoted, unquoted anddata:urls, a#fragment,@font-face src, a custom property,image-set(), a file that also has@import, and--minify). All 9 fail on bun 1.4.0 with the error above and pass with this change; the rest of cli.test.ts still passes.--no-bundleon a.module.cssfile still panics inPrinter::lookup_symbol. That happens with or without aurl()and is a separate problem in the same function.Background
bun_ast::ImportRecord, one entry per thing a file references (the path as written, the kind, and after resolution asource_index). The CSS parser creates one perurl(), and CSS values such asUrlkeep only the record index, so the printer needs the same list to print them back.ImportInfo(src/css/printer.rs) is what the CSS printer reads urls from. Inside the bundler it also carries the tables used to rewrite records that resolved to an inlineddata:URL or a copied asset.init_outside_of_bundlerleaves those tables empty; they are only consulted for records with a validsource_index, which this path never produces.--no-bundle(transform_only) makesbun buildtranspile each entry point on its own throughTranspiler::transforminstead ofBundleV2, leaving imports for whoever consumes the output to resolve.