Skip to content

Commit e868556

Browse files
committed
fix: address atrule-prelude review feedback
- Drop the => operator-merging workaround in print_feature_range; it only mattered for invalid CSS (=> isn't a real comparator), so skip the test that relied on it instead. - Lowercase media/container feature names, preserving casing for custom-property-style (--foo) names. - Add print_identifier(), a shared "lowercase unless --prefixed" helper, and use it everywhere an identifier gets lowercased (declaration/media-feature properties, function names, selector parts, attribute names, at-rule names) so custom-property-style names and custom function calls (--myFunc()) keep their casing. - Remove the redundant is_prelude_operator branch in print_prelude_component; the trailing return node.text already covers it. - Stop detecting/preserving comments dropped from structured at-rule preludes; skip the two tests that covered it, matching the existing preserves-comments skip. - Apply print_prelude_url's "never touch quotes" behavior to value-position print_url as well, and delete print_prelude_url since it's now identical to print_url. Update the url() quote-handling tests to reflect that url() quoting is no longer touched. - Trim verbose comments across the new atrule-prelude printers. @import's functional supports(...)/layer(...) notation vs. standalone @supports (...)/`@layer name;` are both valid, differently-shaped CSS syntax, not a parser bug, so no PARSER_ISSUES.md changes here.
1 parent a9448ac commit e868556

5 files changed

Lines changed: 92 additions & 155 deletions

File tree

‎src/lib/index.ts‎

Lines changed: 53 additions & 138 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,6 @@ export type FormatOptions = {
7575
}
7676

7777
const UNQUOTE_RE = /(?:^['"])|(?:['"]$)/g
78-
const DATA_URL_RE = /^['"]?data:/i
7978
const FONT_SLASH_RE = /\s*\/\s*/
8079
const ATRULE_COLON_COMMA_RE = /\s*([:,])/g
8180
const ATRULE_PAREN_TEXT_RE = /\)([a-zA-Z])/g
@@ -92,6 +91,12 @@ export function unquote(str: string): string {
9291
return str.replaceAll(UNQUOTE_RE, EMPTY_STRING)
9392
}
9493

94+
/** Lowercases a CSS identifier, except a custom-ident starting with `--`,
95+
* which must keep its case as written. */
96+
function print_identifier(name: string): string {
97+
return name.startsWith('--') ? name : name.toLowerCase()
98+
}
99+
95100
function print_string(str: string | number | null, quote?: '"' | "'"): string {
96101
str = str?.toString() || ''
97102
let inner = unquote(str)
@@ -101,26 +106,14 @@ function print_string(str: string | number | null, quote?: '"' | "'"): string {
101106
return quote + inner + quote
102107
}
103108

104-
function print_url(node: Url): string {
105-
let value = node.value ?? ''
106-
let unquoted = unquote(value)
107-
108-
let inner: string
109-
if (DATA_URL_RE.test(value)) {
110-
let has_double = unquoted.includes('"')
111-
let has_single = unquoted.includes("'")
112-
if (has_double && has_single) {
113-
inner = print_string(unquoted.replaceAll('"', '%22'), '"')
114-
} else if (has_double || has_single) {
115-
inner = print_string(unquoted)
116-
} else {
117-
inner = unquoted
118-
}
119-
} else {
120-
inner = print_string(value)
109+
/** Prints a `url(...)`: lowercases a leading `url(` keyword but leaves quote
110+
* style untouched. */
111+
function print_url(node: Url | CSSNode): string {
112+
let text = node.text
113+
if (/^url\(/i.test(text)) {
114+
return 'url(' + text.slice(4)
121115
}
122-
123-
return 'url(' + inner + CLOSE_PARENTHESES
116+
return text
124117
}
125118

126119
function print_operator(node: Operator, optional_space = SPACE): string {
@@ -138,7 +131,7 @@ function print_list(nodes: CSSNode[], optional_space = SPACE): string {
138131
let parts = []
139132
for (let node of nodes) {
140133
if (is_function(node)) {
141-
let fn = node.name.toLowerCase()
134+
let fn = print_identifier(node.name)
142135
parts.push(fn, OPEN_PARENTHESES, print_list(node.children, optional_space), CLOSE_PARENTHESES)
143136
} else if (is_dimension(node)) {
144137
parts.push(node.value, node.unit?.toLowerCase())
@@ -197,9 +190,7 @@ export function format_declaration(
197190
value += SPACE
198191
}
199192

200-
if (!property.startsWith('--')) {
201-
property = property.toLowerCase()
202-
}
193+
property = print_identifier(property)
203194
return property + COLON + optional_space + value + important
204195
}
205196

@@ -247,7 +238,7 @@ function print_combinator(node: Combinator, optional_space: string, is_first: bo
247238

248239
/** Prints an attribute selector, e.g. `[href^="https://" i]`. */
249240
function print_attribute_selector(node: AttributeSelector): string {
250-
let parts = [OPEN_BRACKET, node.name.toLowerCase()]
241+
let parts = [OPEN_BRACKET, print_identifier(node.name)]
251242

252243
if (node.attr_operator) {
253244
parts.push(node.attr_operator)
@@ -270,7 +261,7 @@ function print_pseudo_selector(
270261
optional_space = SPACE,
271262
): string {
272263
let parts = [COLON]
273-
let name = node.name.toLowerCase()
264+
let name = print_identifier(node.name)
274265

275266
// Legacy pseudo-elements or actual pseudo-elements use double colon
276267
if (name === 'before' || name === 'after' || is_pseudo_element_selector(node)) {
@@ -307,12 +298,12 @@ function print_selector_component(
307298
}
308299

309300
if (is_type_selector(node)) {
310-
let prefix = node.namespace === null ? '' : node.namespace.toLowerCase() + '|'
311-
return prefix + node.name.toLowerCase()
301+
let prefix = node.namespace === null ? '' : print_identifier(node.namespace) + '|'
302+
return prefix + print_identifier(node.name)
312303
}
313304

314305
if (is_universal_selector(node)) {
315-
let prefix = node.namespace === null ? '' : node.namespace.toLowerCase() + '|'
306+
let prefix = node.namespace === null ? '' : print_identifier(node.namespace) + '|'
316307
return prefix + '*'
317308
}
318309

@@ -425,51 +416,35 @@ export function format_atrule_prelude(
425416
}
426417

427418
/** Prints a two-sided (`200px < width < 1000px`) or one-sided (`width >
428-
* 1000px`) media-feature range. The feature name never appears as a child
429-
* node, so its printed position is found by comparing its offset in the
430-
* source against the sibling Dimension/PreludeOperator children. */
419+
* 1000px`) media-feature range, reordering by source offset since the
420+
* feature name isn't a child node. */
431421
function print_feature_range(node: FeatureRange, optional_space: string): string {
432422
let name_offset = node.start + node.text.indexOf(node.name, 1)
433423
let items: { offset: number; text: string }[] = [{ offset: name_offset, text: node.name }]
434424

435-
let children = [...(node as unknown as Iterable<CSSNode>)]
436-
for (let i = 0; i < children.length; i++) {
437-
let child = children[i]!
438-
if (is_prelude_operator(child)) {
439-
let text = child.text
440-
// Upstream tokenizer bug: `=>` comes through as two separate
441-
// single-char operator tokens ("=" then ">") instead of one, unlike
442-
// `>=`/`<=` which are correctly kept whole — merge them back
443-
// together when directly adjacent.
444-
let next = children[i + 1]
445-
if (next && is_prelude_operator(next) && next.start === child.end) {
446-
text += next.text
447-
i++
448-
}
449-
items.push({ offset: child.start, text: optional_space + text + optional_space })
450-
} else {
451-
items.push({ offset: child.start, text: child.text })
452-
}
425+
for (let child of node as unknown as Iterable<CSSNode>) {
426+
let text = is_prelude_operator(child)
427+
? optional_space + child.text + optional_space
428+
: child.text
429+
items.push({ offset: child.start, text })
453430
}
454431

455432
items.sort((a, b) => a.offset - b.offset)
456433
return OPEN_PARENTHESES + items.map((item) => item.text).join(EMPTY_STRING) + CLOSE_PARENTHESES
457434
}
458435

459436
/** Prints a single media/container feature, e.g. `(min-width: 768px)` or the
460-
* boolean form `(hover)`. `node.value` is a fully parsed value node (as of
461-
* @projectwallace/css-parser 0.18.0 — function calls like `calc()`/`env()`
462-
* used to be silently dropped from it), so it can go straight through
463-
* `print_list` like a declaration's value would. */
437+
* boolean form `(hover)`. */
464438
function print_media_feature(node: MediaFeature, minify: boolean): string {
439+
let property = print_identifier(node.property)
465440
if (node.value === null) {
466-
return OPEN_PARENTHESES + node.property + CLOSE_PARENTHESES
441+
return OPEN_PARENTHESES + property + CLOSE_PARENTHESES
467442
}
468443

469444
let optional_space = minify ? EMPTY_STRING : SPACE
470445
return (
471446
OPEN_PARENTHESES +
472-
node.property +
447+
property +
473448
COLON +
474449
optional_space +
475450
print_list([node.value], optional_space) +
@@ -481,28 +456,20 @@ function print_media_feature(node: MediaFeature, minify: boolean): string {
481456
* `and`/`or`/`not`-joined and nested-boolean-group forms that don't reduce to
482457
* a single declaration (e.g. `selector(:hover)`), which print as-is. */
483458
function print_supports_query(node: SupportsQuery, minify: boolean): string {
484-
// node.has_children means the prelude parser found a simple `prop: value`
485-
// declaration inside the parens (never true for @import's `supports(...)`,
486-
// which doesn't get a SupportsDeclaration child at all) — its value node is
487-
// fully parsed (see print_media_feature), so it can be printed directly
488-
// via format_declaration instead of re-parsing raw text.
459+
// has_children means a simple `prop: value` declaration was found inside.
489460
let condition = node.has_children
490461
? format_declaration(node.first_child.first_child, { minify })
491462
: format_atrule_prelude(node.value, { minify })
492-
// `@import url(...) supports(display: grid)` uses the functional notation
493-
// (keyword + parens around the condition) rather than the standalone
494-
// `@supports (...)` form's bare parenthesized condition — node.text still
495-
// carries the "supports(" prefix in that case.
463+
// @import's functional `supports(...)` notation needs the keyword;
464+
// standalone @supports's bare `(...)` form doesn't.
496465
let prefix = /^supports\(/i.test(node.text) ? 'supports' : EMPTY_STRING
497466
return prefix + OPEN_PARENTHESES + condition + CLOSE_PARENTHESES
498467
}
499468

500469
/** Prints a functional container-query condition, e.g. `style(--foo: bar)`. */
501470
function print_prelude_function(node: CSSFunction, minify: boolean): string {
502-
let name = node.name.toLowerCase()
503-
// `@supports selector(...)` takes a selector list, not a declaration —
504-
// deep-parsed as of @projectwallace/css-parser 0.18.1 (previously this
505-
// whole prelude came back empty).
471+
let name = print_identifier(node.name)
472+
// `selector(...)` takes a selector list, not a declaration.
506473
if (name === 'selector' && node.has_children && is_selector_list(node.first_child)) {
507474
return (
508475
name +
@@ -511,11 +478,9 @@ function print_prelude_function(node: CSSFunction, minify: boolean): string {
511478
CLOSE_PARENTHESES
512479
)
513480
}
514-
// A container style() condition is deep-parsed into a SupportsDeclaration
515-
// the same way @supports's own condition is (see print_supports_query),
516-
// as of @projectwallace/css-parser 0.18.1. Function's declared child type
517-
// doesn't include SupportsDeclaration (it's normally only used for
518-
// value-position functions like calc()), hence the cast.
481+
// style()'s condition is a SupportsDeclaration, same as @supports's own
482+
// (see print_supports_query); Function's declared child type doesn't
483+
// include it, hence the cast.
519484
if (node.has_children) {
520485
let declaration = (node.first_child as unknown as SupportsDeclaration).first_child
521486
return name + OPEN_PARENTHESES + format_declaration(declaration, { minify }) + CLOSE_PARENTHESES
@@ -524,24 +489,9 @@ function print_prelude_function(node: CSSFunction, minify: boolean): string {
524489
return name + OPEN_PARENTHESES + format_atrule_prelude(node.value, { minify }) + CLOSE_PARENTHESES
525490
}
526491

527-
/** Prints an `@import` URL: lowercases a leading `url(` keyword but never
528-
* touches quote style, matching how `format_atrule_prelude` already treats
529-
* quotes inside at-rule preludes (unlike value-position `url()`, which does
530-
* get its quotes normalized by `print_url`). */
531-
function print_prelude_url(node: CSSNode): string {
532-
let text = node.text
533-
if (/^url\(/i.test(text)) {
534-
return 'url(' + text.slice(4)
535-
}
536-
return text
537-
}
538-
539-
/**
540-
* Prints one child of an AtrulePrelude/MediaQuery/ContainerQuery: a nested
541-
* query, a feature/condition, an operator, or (for anything the prelude
542-
* parser doesn't specifically model — Identifier, String, LayerName from
543-
* `@import`'s `layer()`, Raw, ...) the node's raw text verbatim.
544-
*/
492+
/** Prints one child of an AtrulePrelude/MediaQuery/ContainerQuery. Falls back
493+
* to the node's raw text for anything the prelude parser doesn't specifically
494+
* model (Identifier, String, Raw, ...). */
545495
function print_prelude_component(node: CSSNode, optional_space: string, minify: boolean): string {
546496
if (is_media_query(node) || is_container_query(node)) {
547497
return print_prelude_children(node, optional_space, minify)
@@ -562,33 +512,20 @@ function print_prelude_component(node: CSSNode, optional_space: string, minify:
562512
return print_prelude_function(node, minify)
563513
}
564514
if (is_url(node)) {
565-
return print_prelude_url(node)
515+
return print_url(node)
566516
}
567517
if (is_layer_name(node)) {
568-
// Both the standalone `@layer name[, name2, ...];` statement form
569-
// (bare "name") and @import's embedded `layer(...)` reach this branch.
570-
// Only the latter has a "layer(" keyword to lowercase (matching
571-
// format_atrule_prelude); the former's node.text is just the name
572-
// itself, dots and all (@projectwallace/css-parser 0.17.0+).
518+
// @import's functional `layer(...)` notation has a keyword to
519+
// lowercase; the standalone `@layer name;` statement form doesn't.
573520
return /^layer\(/i.test(node.text) ? 'layer(' + node.text.slice(6) : node.text
574521
}
575-
if (is_prelude_operator(node)) {
576-
return node.text
577-
}
578522
return node.text
579523
}
580524

581-
/**
582-
* Prints a `,`-or-space joined sequence of at-rule prelude components: a
583-
* media/container query's own parts, or the top-level children of an
584-
* AtrulePrelude/MediaQuery/ContainerQuery.
585-
*
586-
* Same-type siblings back to back only ever happens for comma-separated
587-
* lists (e.g. multiple `MediaQuery`s in `screen, print`), so a comma is used
588-
* there; every other adjacent pair (`screen and (min-width: 100px)`, or
589-
* `url(...) layer(...) supports(...)` in `@import`) requires a real space
590-
* regardless of `minify` — CSS syntax doesn't allow gluing them together.
591-
*/
525+
/** Prints a `,`-or-space joined sequence of at-rule prelude components.
526+
* Same-type siblings (e.g. multiple `MediaQuery`s in `screen, print`) are
527+
* comma-separated; everything else gets a real space, always, since CSS
528+
* doesn't allow gluing them together. */
592529
function print_prelude_children(node: CSSNode, optional_space: string, minify: boolean): string {
593530
let parts: string[] = []
594531
for (let child of node as unknown as Iterable<CSSNode>) {
@@ -600,35 +537,13 @@ function print_prelude_children(node: CSSNode, optional_space: string, minify: b
600537
return parts.join(EMPTY_STRING)
601538
}
602539

603-
/**
604-
* Prints a structured at-rule prelude node (`Atrule.prelude` when
605-
* `parse_atrule_preludes: true`).
606-
*
607-
* Falls back to the existing regex-based `format_atrule_prelude` (still
608-
* correct, just not structure-aware), applied to the whole prelude's raw
609-
* text, when:
610-
* - the prelude parser doesn't recognize the at-rule at all, or the prelude
611-
* doesn't fit what it expects (`@page :first`, a quoted `@keyframes` name,
612-
* `@starting-style`, an unsupported `@supports selector(...)`/function
613-
* condition, ...) — it returns no children in that case, and nothing new
614-
* is invented for at-rules with no test coverage, just today's existing
615-
* (already tested) text-based formatting;
616-
* - the prelude contains a comment: the prelude parser has no equivalent of
617-
* the main parser's `on_comment` hook, so any comment gets silently
618-
* dropped while building the structured tree.
619-
*/
540+
/** Prints a structured at-rule prelude node. Falls back to the regex-based
541+
* `format_atrule_prelude` when the prelude parser doesn't recognize the
542+
* at-rule or its shape (`@page :first`, `@starting-style`, ...). */
620543
function print_atrule_prelude_node(node: AtrulePrelude | Raw, minify: boolean): string {
621544
if (is_raw(node) || !node.has_children) {
622545
return format_atrule_prelude(node.text, { minify })
623546
}
624-
// The prelude parser has no equivalent of the main parser's on_comment
625-
// hook, so any comment inside a prelude is silently dropped while
626-
// building the structured tree. Fall back to the regex formatter, which
627-
// operates on the raw text and never removes anything, whenever a
628-
// comment is present.
629-
if (node.text.includes('/*')) {
630-
return format_atrule_prelude(node.text, { minify })
631-
}
632547
let optional_space = minify ? EMPTY_STRING : SPACE
633548
return print_prelude_children(node, optional_space, minify)
634549
}
@@ -811,7 +726,7 @@ export function format(
811726
}
812727

813728
function print_atrule(node: Atrule): string {
814-
let name = '@' + node.name!.toLowerCase()
729+
let name = '@' + print_identifier(node.name!)
815730
if (node.prelude) {
816731
name += SPACE + print_atrule_prelude_node(node.prelude, minify)
817732
}

‎test/atrules.test.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -345,16 +345,13 @@ test('new-fangled comparators (width > 1000px)', () => {
345345
let actual = format(`
346346
@container (width>1000px) {}
347347
@media (width>1000px) {}
348-
@media (width=>1000px) {}
349348
@media (width<=1000px) {}
350349
@media (200px<width<1000px) {}
351350
`)
352351
let expected = `@container (width > 1000px) {}
353352
354353
@media (width > 1000px) {}
355354
356-
@media (width => 1000px) {}
357-
358355
@media (width <= 1000px) {}
359356
360357
@media (200px < width < 1000px) {}`
@@ -367,6 +364,18 @@ test('minify: new-fangled comparators (width > 1000px)', () => {
367364
expect(actual).toEqual(expected)
368365
})
369366

367+
test('lowercases the media/container feature name', () => {
368+
let actual = format(`@media (MIN-WIDTH: 100px) {}`)
369+
let expected = `@media (min-width: 100px) {}`
370+
expect(actual).toEqual(expected)
371+
})
372+
373+
test('preserves casing of a custom-property media feature name', () => {
374+
let actual = format(`@media (--Foo: 1) {}`)
375+
let expected = `@media (--Foo: 1) {}`
376+
expect(actual).toEqual(expected)
377+
})
378+
370379
test('minify: keeps necessary whitespace between keywords', () => {
371380
let actual = minify(`@media screen or print {}`)
372381
let expected = `@media screen or print{}`

‎test/comments.test.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -449,14 +449,19 @@ selector {}
449449
expect(actual).toEqual(expected)
450450
})
451451

452-
test('in @media prelude', () => {
452+
// Comments inside at-rule preludes are dropped by the structured prelude
453+
// printer (see print_atrule_prelude_node in src/lib/index.ts) — accepted
454+
// tradeoff, not a bug.
455+
// oxlint-disable-next-line vitest/no-disabled-tests
456+
test.skip('in @media prelude', () => {
453457
// from CSSTree https://github.com/csstree/csstree/blob/ba6dfd8bb0e33055c05f13803d04825d98dd2d8d/fixtures/ast/mediaQuery/MediaQuery.json#L147
454458
let actual = format('@media all /*0*/ (/*1*/foo/*2*/:/*3*/1/*4*/) {}')
455459
let expected = '@media all /*0*/ (/*1*/foo/*2*/: /*3*/1/*4*/) {}'
456460
expect(actual).toEqual(expected)
457461
})
458462

459-
test('in @supports prelude', () => {
463+
// oxlint-disable-next-line vitest/no-disabled-tests
464+
test.skip('in @supports prelude', () => {
460465
// from CSSTree https://github.com/csstree/csstree/blob/ba6dfd8bb0e33055c05f13803d04825d98dd2d8d/fixtures/ast/atrule/atrule/supports.json#L119
461466
let actual = format('@supports not /*0*/(/*1*/flex :/*3*/1/*4*/)/*5*/{}')
462467
let expected = '@supports not /*0*/(/*1*/flex: /*3*/1/*4*/) {}'

0 commit comments

Comments
 (0)