From 2aac02cf9835706b11ffbb6d69a4b06e4ae82c8a Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Thu, 7 Mar 2024 12:15:49 +0100 Subject: [PATCH 1/5] add `WalkAction` to indicate how the `walk` function should behave You can `Continue` its execution (the default behaviour), `Skip` walking the current node, or `Stop` walking entirely. --- packages/tailwindcss/src/ast.ts | 40 +++++++++++++++++++++---------- packages/tailwindcss/src/index.ts | 6 ++--- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/packages/tailwindcss/src/ast.ts b/packages/tailwindcss/src/ast.ts index 626c255371a7..d316c541196e 100644 --- a/packages/tailwindcss/src/ast.ts +++ b/packages/tailwindcss/src/ast.ts @@ -42,6 +42,17 @@ export function comment(value: string): Comment { } } +export enum WalkAction { + /** Continue walking, which is the default */ + Continue, + + /** Skip visiting the children of this node */ + Skip, + + /** Stop the walk entirely */ + Stop, +} + export function walk( ast: AstNode[], visit: ( @@ -49,21 +60,26 @@ export function walk( utils: { replaceWith(newNode: AstNode | AstNode[]): void }, - ) => void | false, + ) => void | WalkAction, ) { for (let i = 0; i < ast.length; i++) { let node = ast[i] - let shouldContinue = visit(node, { - replaceWith(newNode) { - ast.splice(i, 1, ...(Array.isArray(newNode) ? newNode : [newNode])) - // We want to visit the newly replaced node(s), which start at the current - // index (i). By decrementing the index here, the next loop will process - // this position (containing the replaced node) again. - i-- - }, - }) - - if (shouldContinue === false) return + let status = + visit(node, { + replaceWith(newNode) { + ast.splice(i, 1, ...(Array.isArray(newNode) ? newNode : [newNode])) + // We want to visit the newly replaced node(s), which start at the + // current index (i). By decrementing the index here, the next loop + // will process this position (containing the replaced node) again. + i-- + }, + }) ?? WalkAction.Continue + + // Stop the walk entirely + if (status === WalkAction.Stop) return + + // Skip visiting the children of this node + if (status === WalkAction.Skip) continue if (node.kind === 'rule') { walk(node.nodes, visit) diff --git a/packages/tailwindcss/src/index.ts b/packages/tailwindcss/src/index.ts index e0e3750ab978..9d4b49d77a48 100644 --- a/packages/tailwindcss/src/index.ts +++ b/packages/tailwindcss/src/index.ts @@ -1,6 +1,6 @@ import { Features, transform } from 'lightningcss' import { version } from '../package.json' -import { comment, decl, rule, toCss, walk, type AstNode, type Rule } from './ast' +import { WalkAction, comment, decl, rule, toCss, walk, type AstNode, type Rule } from './ast' import { compileCandidates } from './compile' import * as CSS from './css-parser' import { buildDesignSystem } from './design-system' @@ -29,7 +29,7 @@ export function compile(css: string, rawCandidates: string[]) { if (node.kind === 'rule' && node.selector.startsWith('@keyframes ')) { keyframesRules.push(node) replaceWith([]) - return + return WalkAction.Skip } if (node.kind !== 'declaration') return @@ -94,7 +94,7 @@ export function compile(css: string, rawCandidates: string[]) { // Stop walking after finding `@tailwind utilities` to avoid walking all // of the generated CSS. This means `@tailwind utilities` can only appear // once per file but that's the intended usage at this point in time. - return false + return WalkAction.Stop } }) From 02e6744db4d7aad46f8b2d0fd7c37c92f2ed99ec Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Thu, 7 Mar 2024 12:16:37 +0100 Subject: [PATCH 2/5] walk the nodes of `@theme` directly There is no need to walk the `@theme` itself or to create a temporary array here. --- packages/tailwindcss/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tailwindcss/src/index.ts b/packages/tailwindcss/src/index.ts index 9d4b49d77a48..c152aed3b3e4 100644 --- a/packages/tailwindcss/src/index.ts +++ b/packages/tailwindcss/src/index.ts @@ -23,7 +23,7 @@ export function compile(css: string, rawCandidates: string[]) { if (node.selector !== '@theme') return // Record all custom properties in the `@theme` declaration - walk([node], (node, { replaceWith }) => { + walk(node.nodes, (node, { replaceWith }) => { // Collect `@keyframes` rules to re-insert with theme variables later, // since the `@theme` rule itself will be removed. if (node.kind === 'rule' && node.selector.startsWith('@keyframes ')) { From 309bbf2777328a6fd50b1ef87c964090add6988b Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Thu, 7 Mar 2024 12:25:51 +0100 Subject: [PATCH 3/5] improvement: skip walking --- packages/tailwindcss/src/index.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/tailwindcss/src/index.ts b/packages/tailwindcss/src/index.ts index c152aed3b3e4..86c8dea3e957 100644 --- a/packages/tailwindcss/src/index.ts +++ b/packages/tailwindcss/src/index.ts @@ -45,6 +45,7 @@ export function compile(css: string, rawCandidates: string[]) { } else { replaceWith([]) } + return WalkAction.Skip }) // Output final set of theme variables at the position of the first `@theme` @@ -145,6 +146,7 @@ export function compile(css: string, rawCandidates: string[]) { walk(ast, (node, { replaceWith }) => { if (node.kind === 'rule' && node.selector === '@media reference') { replaceWith([]) + return WalkAction.Skip } }) } From 37d119803b8d02aabadb4abd2310a51028bb5638 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Thu, 7 Mar 2024 12:31:52 +0100 Subject: [PATCH 4/5] only allow CSS variables inside `@theme` --- packages/tailwindcss/src/index.test.ts | 27 ++++++++++++++++++++++++++ packages/tailwindcss/src/index.ts | 13 ++++++++++--- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/packages/tailwindcss/src/index.test.ts b/packages/tailwindcss/src/index.test.ts index 7457ac352450..110c04695b8c 100644 --- a/packages/tailwindcss/src/index.test.ts +++ b/packages/tailwindcss/src/index.test.ts @@ -51,6 +51,33 @@ describe('compiling CSS', () => { `) }) + test('that only CSS variables are allowed', () => { + expect(() => + compileCss( + css` + @theme { + --color-primary: red; + .foo { + --color-primary: blue; + } + } + @tailwind utilities; + `, + ['bg-primary'], + ), + ).toThrowErrorMatchingInlineSnapshot(` + [Error: Invalid \`@theme\` detected: + + @theme { + /* Only CSS variables are allowed, this is not allowed: */ + .foo { + --color-primary: blue; + } + } + ] + `) + }) + test('`@tailwind utilities` is only processed once', () => { expect( compileCss( diff --git a/packages/tailwindcss/src/index.ts b/packages/tailwindcss/src/index.ts index 86c8dea3e957..6f0341e1c4cc 100644 --- a/packages/tailwindcss/src/index.ts +++ b/packages/tailwindcss/src/index.ts @@ -32,10 +32,17 @@ export function compile(css: string, rawCandidates: string[]) { return WalkAction.Skip } - if (node.kind !== 'declaration') return - if (!node.property.startsWith('--')) return + if (node.kind === 'comment') return + if (node.kind === 'declaration' && node.property.startsWith('--')) { + theme.add(node.property, node.value) + return + } - theme.add(node.property, node.value) + let tree = rule('@theme', [ + comment(' Only CSS variables are allowed, this is not allowed: '), + node, + ]) + throw new Error(`Invalid \`@theme\` detected:\n\n${toCss([tree])}`) }) // Keep a reference to the first `@theme` rule to update with the full theme From 090727ed1d181c8cb365964d4cdb43aad50b95c6 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Thu, 7 Mar 2024 15:56:29 +0100 Subject: [PATCH 5/5] update error message --- packages/tailwindcss/src/index.test.ts | 13 ++++++------- packages/tailwindcss/src/index.ts | 13 ++++++++----- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/packages/tailwindcss/src/index.test.ts b/packages/tailwindcss/src/index.test.ts index 110c04695b8c..521841cbe5ce 100644 --- a/packages/tailwindcss/src/index.test.ts +++ b/packages/tailwindcss/src/index.test.ts @@ -66,15 +66,14 @@ describe('compiling CSS', () => { ['bg-primary'], ), ).toThrowErrorMatchingInlineSnapshot(` - [Error: Invalid \`@theme\` detected: + [Error: \`@theme\` blocks must only contain custom properties or \`@keyframes\`. - @theme { - /* Only CSS variables are allowed, this is not allowed: */ - .foo { - --color-primary: blue; + @theme { + > .foo { + > --color-primary: blue; + > } } - } - ] + ] `) }) diff --git a/packages/tailwindcss/src/index.ts b/packages/tailwindcss/src/index.ts index 6f0341e1c4cc..59fe7a21c870 100644 --- a/packages/tailwindcss/src/index.ts +++ b/packages/tailwindcss/src/index.ts @@ -38,11 +38,14 @@ export function compile(css: string, rawCandidates: string[]) { return } - let tree = rule('@theme', [ - comment(' Only CSS variables are allowed, this is not allowed: '), - node, - ]) - throw new Error(`Invalid \`@theme\` detected:\n\n${toCss([tree])}`) + let snippet = toCss([rule('@theme', [node])]) + .split('\n') + .map((line, idx, all) => `${idx === 0 || idx >= all.length - 2 ? ' ' : '>'} ${line}`) + .join('\n') + + throw new Error( + `\`@theme\` blocks must only contain custom properties or \`@keyframes\`.\n\n${snippet}`, + ) }) // Keep a reference to the first `@theme` rule to update with the full theme