Skip to content
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.

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

feat: CSS Nesting implementation #9549

Closed
wants to merge 78 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
4ded50f
Semi working
AlbertMarashi Nov 19, 2023
a3d4eaf
Working CSS nesting implementation
AlbertMarashi Nov 20, 2023
68713ee
Merge branches 'main' and 'main' of https://github.com/sveltejs/svelte
AlbertMarashi Nov 20, 2023
8756de3
Add changeset
AlbertMarashi Nov 20, 2023
2661f4e
Remove some unused testing code
AlbertMarashi Nov 20, 2023
0e67006
Remove unused testing code
AlbertMarashi Nov 20, 2023
d9174c4
Handle rule nesting, remove NestedSelector from SimpleSelector, impro…
AlbertMarashi Nov 20, 2023
b59387b
use for of
AlbertMarashi Nov 20, 2023
fcdc02e
Got CSS combinator prefixes working
AlbertMarashi Nov 27, 2023
dab01b1
closes #9320
AlbertMarashi Nov 27, 2023
05e959c
Implement full CSS nesting support
AlbertMarashi Nov 27, 2023
1ba667d
Update changeset message
AlbertMarashi Nov 27, 2023
b0397d1
Remove some unused code/comments
AlbertMarashi Nov 27, 2023
a37b975
Update .changeset/small-kids-switch.md
AlbertMarashi Nov 27, 2023
48c6cdf
Revert contributor change
AlbertMarashi Nov 29, 2023
2291dc3
Merge branch 'css-nesting' of https://github.com/AlbertMarashi/svelte…
AlbertMarashi Nov 29, 2023
2b4e730
Minor change
AlbertMarashi Nov 29, 2023
b6b872c
Modify comment
AlbertMarashi Nov 29, 2023
a8fb36d
Add additional test fixing bug with multiple nested layers
AlbertMarashi Nov 30, 2023
21d5daf
Merge branch 'main' into css-nesting
AlbertMarashi Jan 31, 2024
92f1e3a
fix syntax error
Rich-Harris Feb 1, 2024
6373f16
Handle CSS nesting by adding invisible selectors before nested rules
AlbertMarashi Feb 3, 2024
b5444cf
Correct comment
AlbertMarashi Feb 3, 2024
7e2875b
Added a failing test
AlbertMarashi Feb 3, 2024
7ea05ce
Merge branch 'main' into pr/9549
Rich-Harris Feb 6, 2024
61497b2
prettier
Rich-Harris Feb 6, 2024
751c73c
partial fix
Rich-Harris Feb 6, 2024
bdccdcb
remove unused export keyword
Rich-Harris Feb 6, 2024
633e05a
move type annotation
Rich-Harris Feb 6, 2024
4d4d973
use NestingSelector everywhere, include in SimpleSelector
Rich-Harris Feb 6, 2024
b884dae
Push some broken code, added block nested groupings
AlbertMarashi Feb 7, 2024
46efb43
Started working on encapsulation logic
AlbertMarashi Feb 7, 2024
0708f16
Wohooo. Almost working
AlbertMarashi Feb 7, 2024
eef0b3b
2 failing tests
AlbertMarashi Feb 7, 2024
8f347a9
Refactor a bunch of code
AlbertMarashi Feb 7, 2024
20629ca
Getting closer
AlbertMarashi Feb 7, 2024
862be02
Even closer
AlbertMarashi Feb 7, 2024
ef4b743
One test left!
AlbertMarashi Feb 7, 2024
1000cdf
Comment out some console.logs
AlbertMarashi Feb 7, 2024
89667b4
All tests passing & clean up a little bit of code
AlbertMarashi Feb 7, 2024
8c81aa7
Rever launch.json changes
AlbertMarashi Feb 8, 2024
51016b5
Add more tests
AlbertMarashi Feb 8, 2024
c1dfaaf
Enhance logic and add some more tests
AlbertMarashi Feb 8, 2024
015e695
Clean some logic
AlbertMarashi Feb 8, 2024
bd37582
Update deprecated function
AlbertMarashi Feb 8, 2024
4b4922e
prettier
Rich-Harris Feb 8, 2024
337b162
merge main
Rich-Harris Feb 9, 2024
055ec38
WIP Test
AlbertMarashi Feb 12, 2024
e075ad7
Merge branch 'css-nesting' of https://github.com/AlbertMarashi/svelte…
AlbertMarashi Feb 12, 2024
a421705
Add test expected result
AlbertMarashi Feb 12, 2024
b9b4cab
Pass all tests
AlbertMarashi Feb 12, 2024
f6a7796
Pass test for invalid characters and simplify block item parse code
AlbertMarashi Feb 12, 2024
fb09917
Add multiple nesting selector support
AlbertMarashi Feb 12, 2024
9c5c05e
Lol wot? Apparently I didn't need that logic
AlbertMarashi Feb 12, 2024
26ba130
Remove some unused logic
AlbertMarashi Feb 12, 2024
2c3cdab
avoid try-catch
Rich-Harris Feb 13, 2024
a528ee6
prettier
Rich-Harris Feb 13, 2024
bb8993b
we really don't need all these tests, it's overkill. one will do. sti…
Rich-Harris Feb 13, 2024
32da098
tweak
Rich-Harris Feb 13, 2024
a46d9fd
unused import
Rich-Harris Feb 13, 2024
1543993
comment out rules with unused/empty children
Rich-Harris Feb 13, 2024
4a7ac18
tweak
Rich-Harris Feb 13, 2024
f03e316
update test
Rich-Harris Feb 13, 2024
a53fa5a
generate wrappers during parse, allow rules to contain atrules
Rich-Harris Feb 13, 2024
67bf628
move grouping logic into Selector
Rich-Harris Feb 14, 2024
3d0a885
shrink things down a touch
Rich-Harris Feb 14, 2024
6e149f2
simplify
Rich-Harris Feb 14, 2024
353d697
simplify
Rich-Harris Feb 14, 2024
84b6f8b
actually we don't need that
Rich-Harris Feb 14, 2024
36123fc
shrink explanation
Rich-Harris Feb 14, 2024
ecff925
move
Rich-Harris Feb 14, 2024
1f23ce5
rename
Rich-Harris Feb 14, 2024
ec49db4
rename
Rich-Harris Feb 14, 2024
f6ba25c
That nested rule wasn't empty
AlbertMarashi Feb 14, 2024
0212055
Merge branch 'css-nesting' of github.com:AlbertMarashi/svelte into pr…
Rich-Harris Feb 14, 2024
4201cf7
revert
Rich-Harris Feb 14, 2024
e3b84d5
merge main
Rich-Harris Feb 14, 2024
d470e13
merge main
Rich-Harris Feb 14, 2024
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/small-kids-switch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

feat: CSS nesting support
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ Test samples are kept in `/test/xxx/samples` folder.
> PREREQUISITE: Install chromium via playwright by running `pnpm playwright install chromium`

1. To run test, run `pnpm test`.
1. To run test for a specific feature, you can use the `-g` (aka `--grep`) option. For example, to only run test involving transitions, run `pnpm test -- -g transition`.
1. To run test for a specific feature, you can use the `-t` (aka `--testNamePattern `) option. For example, to only run test involving transitions, run `pnpm test -- -t transistion`.
AlbertMarashi marked this conversation as resolved.
Show resolved Hide resolved

##### Running solo test

Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/1-parse/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ export class Parser {
* @param {string} str
* @param {boolean} [required]
*/
eat(str, required) {
eat(str, required = false) {
if (this.match(str)) {
this.index += str.length;
return true;
Expand Down
74 changes: 41 additions & 33 deletions packages/svelte/src/compiler/phases/1-parse/read/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const REGEX_PERCENTAGE = /^\d+(\.\d+)?%/;
const REGEX_WHITESPACE_OR_COLON = /[\s:]/;
const REGEX_BRACE_OR_SEMICOLON = /[{;]/;
const REGEX_LEADING_HYPHEN_OR_DIGIT = /-?\d/;
const REGEX_SEMICOLON_OR_OPEN_BRACE_OR_CLOSE_BRACE = /[;{}]/;
const REGEX_VALID_IDENTIFIER_CHAR = /[a-zA-Z0-9_-]/;
const REGEX_COMMENT_CLOSE = /\*\//;
const REGEX_HTML_COMMENT_CLOSE = /-->/;
Expand Down Expand Up @@ -83,37 +84,11 @@ function read_at_rule(parser) {
/** @type {import('#compiler').Css.Block | null} */
let block = null;

// eg: `@media (max-width: 600px) { ... }`
if (parser.match('{')) {
// if the parser could easily distinguish between rules and declarations, this wouldn't be necessary.
// but this approach is much simpler. in future, when we support CSS nesting, the parser _will_ need
// to be able to distinguish between them, but since we'll also need other changes to support that
// this remains a TODO
const contains_declarations = [
'color-profile',
'counter-style',
'font-face',
'font-palette-values',
'page',
'property'
].includes(name);

if (contains_declarations) {
block = read_block(parser);
} else {
const start = parser.index;

parser.eat('{', true);
const children = read_body(parser, '}');
parser.eat('}', true);

block = {
type: 'Block',
start,
end: parser.index,
children
};
}
block = read_block(parser);
} else {
// eg: `@import 'foo';`
parser.eat(';', true);
}

Expand Down Expand Up @@ -183,13 +158,20 @@ function read_selector_list(parser) {
function read_selector(parser) {
const list_start = parser.index;

/** @type {Array<import('#compiler').Css.SimpleSelector | import('#compiler').Css.Combinator>} */
/** @type {Array<import('#compiler').Css.SimpleSelector | import('#compiler').Css.Combinator | import('#compiler').Css.NestingSelector>} */
Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
const children = [];

while (parser.index < parser.template.length) {
const start = parser.index;

if (parser.eat('*')) {
if (parser.eat('&')){
children.push({
type: 'NestedSelector',
name: '&',
start,
end: parser.index
});
} else if (parser.eat('*')) {
children.push({
type: 'TypeSelector',
name: '*',
Expand Down Expand Up @@ -328,7 +310,7 @@ function read_block(parser) {

parser.eat('{', true);

/** @type {Array<import('#compiler').Css.Declaration | import('#compiler').Css.Rule>} */
/** @type {Array<import('#compiler').Css.Declaration | import('#compiler').Css.Rule | import('#compiler').Css.Atrule>} */
const children = [];

while (parser.index < parser.template.length) {
Expand All @@ -337,7 +319,7 @@ function read_block(parser) {
if (parser.match('}')) {
break;
} else {
children.push(read_declaration(parser));
children.push(read_block_item(parser));
}
}

Expand Down Expand Up @@ -380,6 +362,32 @@ function read_declaration(parser) {
};
}

/**
* Reads a declaration, rule or at-rule
*
* @param {import('../index.js').Parser} parser
* @returns {import('#compiler').Css.Declaration | import('#compiler').Css.Rule | import('#compiler').Css.Atrule}
*/
function read_block_item(parser) {
if (parser.match('@')) {
return read_at_rule(parser);
}

const start = parser.index;
parser.read_until(REGEX_SEMICOLON_OR_OPEN_BRACE_OR_CLOSE_BRACE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using regexes for this sort of thing always makes me a bit nervous — have we tested this against e.g. values containing strings that contain special characters? see e.g. #10221


// if we've run into a '{', it's a rule, otherwise we ran into
// a ';' or '}' so it's a declaration
if (parser.match('{')) {
// Rewind to the start of the rule
parser.index = start;
return read_rule(parser);
}
// Rewind to the start of the declaration
parser.index = start;
return read_declaration(parser);
}

/**
* @param {import('../index.js').Parser} parser
* @returns {string}
Expand Down
43 changes: 34 additions & 9 deletions packages/svelte/src/compiler/phases/2-analyze/css/Selector.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export default class Selector {
while (i--) {
const selector = block.selectors[i];
if (selector.type === 'PseudoElementSelector' || selector.type === 'PseudoClassSelector') {
if (selector.name !== 'root' && selector.name !== 'host') {
if (!block.root && !block.host && !block.nested) {
if (i === 0) code.prependRight(selector.start, attr);
}
continue;
Expand All @@ -117,13 +117,14 @@ export default class Selector {
if (block.global) {
remove_global_pseudo_class(block.selectors[0]);
}
if (block.should_encapsulate)
if (block.should_encapsulate) {
encapsulate_block(
block,
index === this.blocks.length - 1
index === this.blocks.length - 1 + (block.nested ? 1 : 0)
? attr.repeat(amount_class_specificity_to_increase + 1)
: attr
);
}
});
}

Expand Down Expand Up @@ -171,7 +172,7 @@ export default class Selector {
validate_invalid_combinator_without_selector(analysis) {
for (let i = 0; i < this.blocks.length; i++) {
const block = this.blocks[i];
if (block.selectors.length === 0) {
if (block.selectors.length === 0 && !block.nested) {
error(this.node, 'invalid-css-selector');
}
}
Expand Down Expand Up @@ -320,6 +321,16 @@ function block_might_apply_to_node(block, node) {
return NO_MATCH;
}

if (
block.nested &&
block.selectors.length === 1 &&
block.combinator !== null
) {
// nested selector with combinator, eg: `.foo { + .bar { ... } }`
// TODO: How to handle this?
return UNKNOWN_SELECTOR;
}

if (selector.type === 'PseudoClassSelector' || selector.type === 'PseudoElementSelector') {
continue;
}
Expand Down Expand Up @@ -797,6 +808,7 @@ class Block {
this.combinator = combinator;
this.host = false;
this.root = false;
this.nested = false;
this.selectors = [];
this.start = -1;
this.end = -1;
Expand Down Expand Up @@ -826,18 +838,31 @@ class Block {
}
}

/** @param {import('#compiler').Css.Selector} selector */
/**
* Groups selectors by combinator into blocks
* @param {import('#compiler').Css.Selector} selector
*/
function group_selectors(selector) {
let block = new Block(null);
const blocks = [block];

selector.children.forEach((child) => {
for (const child of selector.children) {
if (child.type === 'Combinator') {
block = new Block(child);
blocks.push(block);
// If we start with a combinator, that means
// we're dealing with a nested selector
AlbertMarashi marked this conversation as resolved.
Show resolved Hide resolved
if(block.selectors.length === 0 && !block.combinator) {
block.nested = true;
block.combinator = child;
} else {
block = new Block(child);
blocks.push(block);
}
} else if (child.type === "NestedSelector") {
block.nested = true;
} else {
block.add(child);
}
});
}

return blocks;
}
48 changes: 38 additions & 10 deletions packages/svelte/src/compiler/phases/2-analyze/css/Stylesheet.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,30 +58,40 @@ class Rule {
/** @type {import('#compiler').Css.Rule} */
node;

/** @type {Atrule | undefined} */
/** @type {Atrule | Rule | undefined} */
parent;

/** @type {Rule[]} */
nested_rules;

/**
* @param {import('#compiler').Css.Rule} node
* @param {any} stylesheet
* @param {Atrule | undefined} parent
* @param {Stylesheet} stylesheet
* @param {Atrule | Rule | undefined} parent
*/
constructor(node, stylesheet, parent) {
this.node = node;
this.parent = parent;
this.selectors = node.prelude.children.map((node) => new Selector(node, stylesheet));

this.declarations = /** @type {import('#compiler').Css.Declaration[]} */ (
node.block.children
).map((node) => new Declaration(node));
this.nested_rules = node.block.children
.filter((node) => node.type === 'Rule')
.map(node => new Rule(/** @type {import('#compiler').Css.Rule}*/ (node), stylesheet, this));
this.declarations = node.block.children
.filter((node) => node.type === 'Declaration')
.map((node) => new Declaration(/** @type {import('#compiler').Css.Declaration} */ (node)));
}

/** @param {import('#compiler').RegularElement | import('#compiler').SvelteElement} node */
apply(node) {
this.selectors.forEach((selector) => selector.apply(node)); // TODO move the logic in here?
this.nested_rules.forEach((rule) => rule.apply(node));
}

/** @param {boolean} dev */
/**
* @param {boolean} dev
* @returns {boolean}
*/
is_used(dev) {
if (this.parent && this.parent.node.type === 'Atrule' && is_keyframes_node(this.parent.node))
return true;
Expand All @@ -90,7 +100,7 @@ class Rule {
// see them in devtools
if (this.declarations.length === 0) return dev;

return this.selectors.some((s) => s.used);
return [this.selectors.some((s) => s.used), this.nested_rules.some(r => r.is_used(dev))].some(Boolean);
}

/**
Expand All @@ -109,27 +119,35 @@ class Rule {
selector.transform(code, attr, max_amount_class_specificity_increased)
);
this.declarations.forEach((declaration) => declaration.transform(code, keyframes));
this.nested_rules.forEach((rule) => rule.transform(code, id, keyframes, max_amount_class_specificity_increased));
}

/** @param {import('../../types.js').ComponentAnalysis} analysis */
validate(analysis) {
this.selectors.forEach((selector) => {
selector.validate(analysis);
});
this.nested_rules.forEach((rule) => {
rule.validate(analysis);
});
}

/** @param {(selector: import('./Selector.js').default) => void} handler */
warn_on_unused_selector(handler) {
this.selectors.forEach((selector) => {
if (!selector.used) handler(selector);
});
this.nested_rules.forEach((rule) => {
rule.warn_on_unused_selector(handler);
});
}

/** @returns number */
get_max_amount_class_specificity_increased() {
return Math.max(
...this.selectors.map((selector) => selector.get_amount_class_specificity_increased())
);
// do we need to check nested rules?
}

/**
Expand All @@ -143,7 +161,7 @@ class Rule {

// keep empty rules in dev, because it's convenient to
// see them in devtools
if (this.declarations.length === 0) {
if (this.declarations.length === 0 && this.nested_rules.length === 0) {
if (!dev) {
code.prependRight(this.node.start, '/* (empty) ');
code.appendLeft(this.node.end, '*/');
Expand Down Expand Up @@ -194,6 +212,10 @@ class Rule {
code.appendLeft(last, '*/');
}
}

this.nested_rules.forEach((rule) => {
rule.prune(code, dev);
});
}
}

Expand Down Expand Up @@ -381,11 +403,12 @@ export default class Stylesheet {
css: ast.content.styles,
hash
});

this.has_styles = true;

const state = {
/** @type {Atrule | undefined} */
atrule: undefined
atrule: undefined,
};

walk(/** @type {import('#compiler').Css.Node} */ (ast), state, {
Expand Down Expand Up @@ -424,12 +447,17 @@ export default class Stylesheet {
},
Rule: (node, context) => {
const rule = new Rule(node, this, context.state.atrule);

if (context.state.atrule) {
context.state.atrule.children.push(rule);
} else {
this.children.push(rule);
}

if (rule.nested_rules.length > 0) {
// Skip nested rules as they are instantiated in the Rule constructor
return node
}
context.next();
}
});
Expand Down
Loading