feat: support formatting embedded CSS in JS#7973
Conversation
🦋 Changeset detectedLatest commit: a10ddee The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
78061bb to
70a3ab4
Compare
CodSpeed Performance ReportMerging #7973 will not alter performanceComparing Summary
Footnotes
|
146e1d6 to
93e570a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs (1)
123-133: Keep nested-rule validation anchored on}.When
end_kindisEOF(the inline-root case), this guard never sees the closing brace that actually terminates a nested rule, so we fall back to parsing it as a declaration and produce a bogus tree. Please keep matching againstT so inline CSS with nested selectors continues to parse correctly.- if p.last().is_some_and(|kind| kind == self.end_kind) { + if p.last().is_some_and(|kind| kind == T!['}']) {crates/biome_css_parser/src/lib.rs (1)
164-166: Inline CSS panics in offset parses
parse_css_with_offsetcan now yieldCssInlineRoot, butCssOffsetParse::treestill insists onCssRoot, so the unwrap will panic as soon as we feed it an inline snippet (which the CSS-in-JS path is about to do). Let’s returnAnyCssRoothere so the new embedding survives without throwing a tantrum at runtime.Apply this diff:
- pub fn tree(&self) -> CssRoot { - CssRoot::unwrap_cast(self.root.inner().clone()) + pub fn tree(&self) -> AnyCssRoot { + AnyCssRoot::unwrap_cast(self.root.inner().clone()) }
🧹 Nitpick comments (3)
crates/biome_css_formatter/src/utils/block_like.rs (1)
49-68: Consider adding a comment explaining the formatting strategy.The switch to
hard_line_break()andblock_indent()makes the formatting more rigid and predictable. A brief comment explaining this choice (especially in the context of embedded CSS support) would help future maintainers understand the rationale.Example:
// When the list is empty, we still print a hard line to put the // closing curly on the next line. + // Note: We use hard_line_break() and block_indent() for predictable + // formatting, especially important for embedded CSS contexts. if self.block.is_empty() || self.block.has_only_empty_declarations() {crates/biome_css_syntax/src/selector_ext.rs (1)
50-50: Consider usingCssFileSource::css()explicitly for consistency.Whilst
Default::default()may be equivalent, other call sites in this PR uniformly useCssFileSource::css(). Explicit is clearer here.- let parsed = parse_css(source, Default::default(), CssParserOptions::default()); + let parsed = parse_css(source, CssFileSource::css(), CssParserOptions::default());crates/biome_service/src/workspace/server.tests.rs (1)
422-484: Well-structured test for the embedded CSS feature!The test covers the happy path nicely. Consider adding a test case that verifies the current limitation with template interpolations (
${...}), as mentioned in the PR objectives—even if just to document expected behaviour.
ematipico
left a comment
There was a problem hiding this comment.
Great feature! I left some comments but overall the feature looks sound
|
What would it take to get this PR over the finishing line? It's such a promising feature! |
# Conflicts: # crates/biome_css_syntax/src/file_source.rs # crates/biome_css_syntax/src/lib.rs # crates/biome_service/src/file_handlers/css.rs # crates/biome_service/src/file_handlers/javascript.rs # crates/biome_service/src/workspace/server.tests.rs
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
|
@arendjr Nothing! Sorry for my late response, I was busy recently. I'll move this forward. |
ematipico
left a comment
There was a problem hiding this comment.
Users will be happy! What's the plan for the experimental flag? Are we looking for ways to support interpolation?
Let's make sure to have a docs PR ready before merging, so we don't lose it
Awesome work 💯
As introducing this feature is technically breaking change and some users may not want to format embedded languages, I would gate this behind the option for a while. Full support for HTML-like languages is also gated behind an option, so this feature will follow it. Lacking support for interpolations is also one of the reasons. |
# Conflicts: # crates/biome_css_analyze/src/lib.rs # crates/biome_css_analyze/src/services/semantic.rs # crates/biome_css_semantic/src/semantic_model/builder.rs # crates/biome_css_semantic/src/semantic_model/model.rs # crates/biome_js_formatter/src/syntax_rewriter.rs # crates/biome_service/src/file_handlers/css.rs # crates/biome_service/src/file_handlers/svelte.rs # crates/biome_service/src/file_handlers/vue.rs # xtask/rules_check/src/lib.rs
6b04f6f to
a10ddee
Compare
Summary
#7802
CssSnippetRootnode to allow parsing CSS snippets without a rule block.parse_embedded_nodesandformat_embeddedhandlers for JavaScript.styledandcsstemplate literals to be parsed and formatted as CSS.Caveats:
styledandcsstag for styled-components or Emotion is supported for now.${foo}) are not supported yet.javascript.experimentalEmbeddedSnippetsEnabledoption.Test Plan
Added a snapshot test to handle simple cases.
Docs
biomejs/website#3622