Skip to content

fix: preserve CSS comments when printing an AST #18475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
manuel3108 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: preserve CSS comments when printing an AST #18475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/curly-dingos-smile.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: preserve CSS comments when printing an AST
67 changes: 48 additions & 19 deletions packages/svelte/src/compiler/phases/1-parse/read/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,17 @@ export default function read_style(parser, start, attributes) {
/**
* @param {Parser} parser
* @param {(parser: Parser) => boolean} finished
* @returns {Array<AST.CSS.Rule | AST.CSS.Atrule>}
* @returns {Array<AST.CSS.Rule | AST.CSS.Atrule | AST.CSS.CSSComment>}
*/
function read_body(parser, finished) {
/** @type {Array<AST.CSS.Rule | AST.CSS.Atrule>} */
/** @type {Array<AST.CSS.Rule | AST.CSS.Atrule | AST.CSS.CSSComment>} */
const children = [];

while ((allow_comment_or_whitespace(parser), !finished(parser))) {
while (parser.index < parser.template.length) {
children.push(...read_comments_and_whitespace(parser));

if (finished(parser)) break;

if (parser.match('@')) {
children.push(read_at_rule(parser));
} else {
Expand Down Expand Up @@ -93,7 +97,8 @@ function read_at_rule(parser) {
start,
end: parser.index,
name,
prelude,
prelude: prelude.value,
...(prelude.raw && { raw: prelude.raw }),
block
};
}
Expand Down Expand Up @@ -126,10 +131,10 @@ function read_rule(parser) {
* @returns {AST.CSS.SelectorList}
*/
function read_selector_list(parser, inside_pseudo_class = false) {
/** @type {AST.CSS.ComplexSelector[]} */
/** @type {Array<AST.CSS.ComplexSelector | AST.CSS.CSSComment>} */
const children = [];

allow_comment_or_whitespace(parser);
children.push(...read_comments_and_whitespace(parser));

const start = parser.index;

Expand All @@ -138,7 +143,7 @@ function read_selector_list(parser, inside_pseudo_class = false) {

const end = parser.index;

allow_comment_or_whitespace(parser);
children.push(...read_comments_and_whitespace(parser));

if (inside_pseudo_class ? parser.match(')') : parser.match('{')) {
return {
Expand All @@ -149,7 +154,7 @@ function read_selector_list(parser, inside_pseudo_class = false) {
};
} else {
parser.eat(',', true);
allow_comment_or_whitespace(parser);
children.push(...read_comments_and_whitespace(parser));
}
}

Expand Down Expand Up @@ -323,7 +328,7 @@ function read_selector(parser, inside_pseudo_class = false) {
}

const index = parser.index;
allow_comment_or_whitespace(parser);
read_comments_and_whitespace(parser);

if (parser.match(',') || (inside_pseudo_class ? parser.match(')') : parser.match('{'))) {
// rewind, so we know whether to continue building the selector list
Expand Down Expand Up @@ -412,11 +417,11 @@ function read_block(parser) {

parser.eat('{', true);

/** @type {Array<AST.CSS.Declaration | AST.CSS.Rule | AST.CSS.Atrule>} */
/** @type {Array<AST.CSS.Declaration | AST.CSS.Rule | AST.CSS.Atrule | AST.CSS.CSSComment>} */
const children = [];

while (parser.index < parser.template.length) {
allow_comment_or_whitespace(parser);
children.push(...read_comments_and_whitespace(parser));

if (parser.match('}')) {
break;
Expand Down Expand Up @@ -471,7 +476,7 @@ function read_declaration(parser) {

const value = read_value(parser);

if (!value && !property.startsWith('--')) {
if (!value.value && !property.startsWith('--')) {
e.css_empty_declaration({ start, end: index });
}

Expand All @@ -486,16 +491,19 @@ function read_declaration(parser) {
start,
end,
property,
value
value: value.value,
...(value.raw && { raw: value.raw })
};
}

/**
* @param {Parser} parser
* @returns {string}
* @returns {{ value: string; raw: string | null }}
*/
function read_value(parser) {
const start = parser.index;
let value = '';
let has_comment = false;
let escaped = false;
let in_url = false;

Expand Down Expand Up @@ -523,13 +531,19 @@ function read_value(parser) {
} else if (char === '(' && value.slice(-3) === 'url') {
in_url = true;
} else if ((char === ';' || char === '{' || char === '}') && !in_url && !quote_mark) {
return value.trim();
const normalized = value.trim();

return {
value: normalized,
raw: has_comment ? parser.template.slice(start, parser.index).trim() : null
};
} else if (
char === '/' &&
!in_url &&
!quote_mark &&
parser.template[parser.index + 1] === '*'
) {
has_comment = true;
parser.index += 2;
while (parser.index < parser.template.length) {
if (parser.template[parser.index] === '*' && parser.template[parser.index + 1] === '/') {
Expand Down Expand Up @@ -624,13 +638,26 @@ function read_identifier(parser) {
return identifier;
}

/** @param {Parser} parser */
function allow_comment_or_whitespace(parser) {
/**
* @param {Parser} parser
* @returns {AST.CSS.CSSComment[]}
*/
function read_comments_and_whitespace(parser) {
/** @type {AST.CSS.CSSComment[]} */
const comments = [];

parser.allow_whitespace();
while (parser.match('/*') || parser.match('<!--')) {
if (parser.eat('/*')) {
parser.read_until(REGEX_COMMENT_CLOSE);
const start = parser.index - 2;
const data = parser.read_until(REGEX_COMMENT_CLOSE);
parser.eat('*/', true);
comments.push({
type: 'CSSComment',
start,
end: parser.index,
data
});
}

if (parser.eat('<!--')) {
Expand All @@ -640,12 +667,14 @@ function allow_comment_or_whitespace(parser) {

parser.allow_whitespace();
}

return comments;
}

/**
* Parse standalone CSS content (not wrapped in `<style>`).
* @param {Parser} parser
* @returns {Array<AST.CSS.Rule | AST.CSS.Atrule>}
* @returns {Array<AST.CSS.Rule | AST.CSS.Atrule | AST.CSS.CSSComment>}
*/
export function parse_stylesheet(parser) {
return read_body(parser, (p) => p.index >= p.template.length);
Expand Down
31 changes: 17 additions & 14 deletions packages/svelte/src/compiler/phases/2-analyze/css/css-analyze.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
/** @import { Visitors } from 'zimmerframe' */
import { walk } from 'zimmerframe';
import * as e from '../../../errors.js';
import { is_keyframes_node } from '../../css.js';
import { is_keyframes_node, without_css_comments } from '../../css.js';
import { is_global, is_unscoped_pseudo_class } from './utils.js';

/**
Expand Down Expand Up @@ -91,7 +91,8 @@ const css_visitors = {
const selector = relative_selector.selectors[i];

if (selector.type === 'PseudoClassSelector' && selector.name === 'global') {
const child = selector.args?.children[0].children[0];
const child =
selector.args && without_css_comments(selector.args.children)[0]?.children[0];
// ensure `:global(element)` to be at the first position in a compound selector
if (child?.selectors[0].type === 'TypeSelector' && i !== 0) {
e.css_global_invalid_selector_list(selector);
Expand All @@ -106,7 +107,7 @@ const css_visitors = {
// (standalone :global() with multiple selectors is OK)
if (
selector.args !== null &&
selector.args.children.length > 1 &&
without_css_comments(selector.args.children).length > 1 &&
(node.children.length > 1 || relative_selector.selectors.length > 1)
) {
e.css_global_invalid_selector(selector);
Expand All @@ -130,9 +131,9 @@ const css_visitors = {
const first = node.children[0]?.selectors[1];
const no_nesting_scope =
first?.type !== 'PseudoClassSelector' || is_unscoped_pseudo_class(first);
const parent_is_global = node.metadata.rule.metadata.parent_rule.prelude.children.some(
(child) => child.children.length === 1 && child.children[0].metadata.is_global
);
const parent_is_global = without_css_comments(
node.metadata.rule.metadata.parent_rule.prelude.children
).some((child) => child.children.length === 1 && child.children[0].metadata.is_global);
// mark `&:hover` in `:global(.foo) { &:hover { color: green }}` as used
if (no_nesting_scope && parent_is_global) {
node.metadata.used = true;
Expand Down Expand Up @@ -196,9 +197,10 @@ const css_visitors = {
},
Rule(node, context) {
node.metadata.parent_rule = context.state.rule;
const complex_selectors = without_css_comments(node.prelude.children);

// We gotta allow :global x, :global y because CSS preprocessors might generate that from :global { x, y {...} }
for (const complex_selector of node.prelude.children) {
for (const complex_selector of complex_selectors) {
let is_global_block = false;

for (let selector_idx = 0; selector_idx < complex_selector.children.length; selector_idx++) {
Expand Down Expand Up @@ -238,7 +240,7 @@ const css_visitors = {
complex_selector.children.length === 1 &&
complex_selector.children[0].selectors.length === 1; // just `:global`, not e.g. `:global x`

if (is_lone_global && node.prelude.children.length > 1) {
if (is_lone_global && complex_selectors.length > 1) {
// `:global, :global x { z { ... } }` would become `x { z { ... } }` which means `z` is always
// constrained by `x`, which is not what the user intended
e.css_global_block_invalid_list(node.prelude);
Expand All @@ -247,7 +249,7 @@ const css_visitors = {
if (
declaration &&
// :global { color: red; } is invalid, but foo :global { color: red; } is valid
node.prelude.children.length === 1 &&
complex_selectors.length === 1 &&
is_lone_global
) {
e.css_global_block_invalid_declaration(declaration);
Expand All @@ -268,7 +270,7 @@ const css_visitors = {
// visit selector list first, to populate child selector metadata
context.visit(node.prelude, state);

for (const selector of node.prelude.children) {
for (const selector of complex_selectors) {
node.metadata.has_global_selectors ||= selector.metadata.is_global;
node.metadata.has_local_selectors ||= !selector.metadata.is_global;
}
Expand All @@ -290,23 +292,24 @@ const css_visitors = {

if (!parent_rule) {
// https://developer.mozilla.org/en-US/docs/Web/CSS/Nesting_selector#using_outside_nested_rule
const children = rule.prelude.children;
const children = without_css_comments(rule.prelude.children);
const selectors = children[0].children[0].selectors;
if (
children.length > 1 ||
selectors.length > 1 ||
selectors[0].type !== 'PseudoClassSelector' ||
selectors[0].name !== 'global' ||
selectors[0].args?.children[0]?.children[0].selectors[0] !== node
(selectors[0].args &&
without_css_comments(selectors[0].args.children)[0]?.children[0].selectors[0]) !== node
) {
e.css_nesting_selector_invalid_placement(node);
}
} else if (
// :global { &.foo { ... } } is invalid
parent_rule.metadata.is_global_block &&
!parent_rule.metadata.parent_rule &&
parent_rule.prelude.children[0].children.length === 1 &&
parent_rule.prelude.children[0].children[0].selectors.length === 1
without_css_comments(parent_rule.prelude.children)[0].children.length === 1 &&
without_css_comments(parent_rule.prelude.children)[0].children[0].selectors.length === 1
) {
e.css_global_block_invalid_modifier_start(node);
}
Expand Down
30 changes: 17 additions & 13 deletions packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
is_unscoped_pseudo_class
} from './utils.js';
import { regex_ends_with_whitespace, regex_starts_with_whitespace } from '../../patterns.js';
import { without_css_comments } from '../../css.js';
import { get_attribute_chunks, is_text_attribute } from '../../../utils/ast.js';

/** @typedef {typeof NODE_PROBABLY_EXISTS | typeof NODE_DEFINITELY_EXISTS} NodeExistsValue */
Expand Down Expand Up @@ -407,11 +408,13 @@ function is_global(selector, rule) {
selector_list = owner.prelude;
}

const has_global_selectors = !!selector_list?.children.some((complex_selector) => {
return complex_selector.children.every((relative_selector) =>
is_global(relative_selector, owner)
);
});
const has_global_selectors =
!!selector_list &&
without_css_comments(selector_list.children).some((complex_selector) => {
return complex_selector.children.every((relative_selector) =>
is_global(relative_selector, owner)
);
});
explicitly_global ||= has_global_selectors;

if (!has_global_selectors && !can_be_global) {
Expand Down Expand Up @@ -448,9 +451,11 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element,
const rules = get_parent_rules(rule);
include_self =
rules.some((r) =>
r.prelude.children.some((c) => c.children.some((s) => is_global(s, r)))
without_css_comments(r.prelude.children).some((c) =>
c.children.some((s) => is_global(s, r))
)
) ||
rules[rules.length - 1].prelude.children.some((c) =>
without_css_comments(rules[rules.length - 1].prelude.children).some((c) =>
c.children.some((r) =>
r.selectors.some(
(s) =>
Expand All @@ -464,8 +469,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element,
// :has(...) is special in that it means "look downwards in the CSS tree". Since our matching algorithm goes
// upwards and back-to-front, we need to first check the selectors inside :has(...), then check the rest of the
// selector in a way that is similar to ancestor matching. In a sense, we're treating `.x:has(.y)` as `.x .y`.
const complex_selectors = /** @type {Compiler.AST.CSS.SelectorList} */ (selector.args)
.children;
const complex_selectors = without_css_comments(selector.args.children);
let matched = false;

for (const complex_selector of complex_selectors) {
Expand Down Expand Up @@ -522,7 +526,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element,
relative_selector.selectors.length === 1
) {
const args = selector.args;
const complex_selector = args.children[0];
const complex_selector = without_css_comments(args.children)[0];
return apply_selector(complex_selector.children, rule, element, BACKWARD);
}

Expand All @@ -533,7 +537,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element,
// because they are then _more_ likely to bleed out of the component. The exception is complex selectors
// with descendants, in which case we scope them all.
if (name === 'not' && selector.args) {
for (const complex_selector of selector.args.children) {
for (const complex_selector of without_css_comments(selector.args.children)) {
walk(complex_selector, null, {
ComplexSelector(node, context) {
node.metadata.used = true;
Expand Down Expand Up @@ -565,7 +569,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element,
if ((name === 'is' || name === 'where') && selector.args) {
let matched = false;

for (const complex_selector of selector.args.children) {
for (const complex_selector of without_css_comments(selector.args.children)) {
const relative = truncate(complex_selector);
const is_global = relative.length === 0;

Expand Down Expand Up @@ -651,7 +655,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element,

const parent = /** @type {Compiler.AST.CSS.Rule} */ (rule.metadata.parent_rule);

for (const complex_selector of parent.prelude.children) {
for (const complex_selector of without_css_comments(parent.prelude.children)) {
if (
apply_selector(get_relative_selectors(complex_selector), parent, element, direction) ||
complex_selector.children.every((s) => is_global(s, parent))
Expand Down
Loading
Loading