From 578f0ca3c40e1c27e9d7a660a5b20fe58a4515b9 Mon Sep 17 00:00:00 2001 From: Rahim Date: Tue, 16 Jun 2026 18:40:00 -0700 Subject: [PATCH] =?UTF-8?q?fix(compiler):=20vanilla-css=20correctness=20?= =?UTF-8?q?=E2=80=94=20markers,=20collisions,=20theme=20vars?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three correctness fixes for the `vanilla-css` Tailwind target, each previously untested: 1. Preserve marker utilities. Utilities that produce no declarations (`group`, `peer`, `group/`) were silently dropped when rewriting `className` to the semantic name, breaking every descendant `group-*` / `peer-*` variant. They're now kept on the element (`"play-button group"`), alongside any other class Tailwind doesn't recognize. 2. Flag class-name collisions. Two distinct elements that derive the same class name but resolve to different utilities would have their conflicting declarations silently merged into one rule (e.g. the forward/back `seek-icon`, buffering vs preview `spinner-icon`, current/duration `time-value`). The plugin now tracks a per-name utility signature and throws a fixable DiagnosticError on mismatch, while identical recurrences (repeated `Tooltip.Popup`) pass cleanly. 3. Emit referenced theme variables. The generated CSS referenced `var(--spacing)`, `var(--color-*)`, `var(--ease-*)` etc. with nothing defining them. `emitCss` now accepts a `resolveThemeVar` resolver (backed by `design.resolveThemeVar`) and emits a leading, transitively resolved theme block — scoped to the skin's hoist root when set — so the output resolves without a separate Tailwind theme on the page. Adds 11 tests (markers, collisions, theme emission incl. transitive, already-declared, scoped, and back-compat). Compiler suite: 169 passing. Note: `@property`-registered `--tw-*` slots (e.g. `--tw-content`) resolve to undefined and are left for a follow-up. --- .../compiler/src/tailwind/design-system.ts | 19 ++++ packages/compiler/src/tailwind/emit.ts | 83 +++++++++++++++++- packages/compiler/src/tailwind/plugin.ts | 86 ++++++++++++++++--- .../compiler/src/tailwind/tests/emit.test.ts | 57 ++++++++++++ .../src/tailwind/tests/plugin.test.ts | 83 ++++++++++++++++++ 5 files changed, 315 insertions(+), 13 deletions(-) diff --git a/packages/compiler/src/tailwind/design-system.ts b/packages/compiler/src/tailwind/design-system.ts index f51bd2f5..6756732b 100644 --- a/packages/compiler/src/tailwind/design-system.ts +++ b/packages/compiler/src/tailwind/design-system.ts @@ -19,6 +19,13 @@ export interface DesignSystem { readonly cssPath: string; /** Compile a single utility class to CSS. Returns `null` for unknown candidates. */ compileUtility(utility: string): string | null; + /** + * Resolve a `@theme` variable (e.g. `--spacing`, `--color-white`) to its + * value, or `undefined` if the theme doesn't define it. Used to emit a + * self-contained theme block for the variables compiled rules reference. + * Returns `undefined` for `@property`-registered slots like `--tw-*`. + */ + resolveThemeVar(name: string): string | undefined; } /** @@ -43,6 +50,7 @@ export async function loadDesignSystem(cssPath: string): Promise { }); const cache = new Map(); + const themeCache = new Map(); return { cssPath: absolute, @@ -54,6 +62,17 @@ export async function loadDesignSystem(cssPath: string): Promise { cache.set(utility, value); return value; }, + resolveThemeVar(name: string): string | undefined { + if (themeCache.has(name)) return themeCache.get(name); + let value: string | undefined; + try { + value = ds.resolveThemeValue?.(name); + } catch { + value = undefined; + } + themeCache.set(name, value); + return value; + }, }; } diff --git a/packages/compiler/src/tailwind/emit.ts b/packages/compiler/src/tailwind/emit.ts index f424665f..27ee968a 100644 --- a/packages/compiler/src/tailwind/emit.ts +++ b/packages/compiler/src/tailwind/emit.ts @@ -75,6 +75,20 @@ export interface EmitCssOptions { * fallback if present, otherwise they're left alone. */ inlineVars?: true | RegExp; + /** + * Resolve a referenced `@theme` variable (e.g. `--spacing`) to its value. + * When set, `emitCss` emits a leading rule defining every theme variable the + * output references but doesn't itself declare, so the CSS resolves without a + * separate Tailwind theme/preflight on the page. Typically + * `design.resolveThemeVar`. Returns `undefined` to leave a variable alone + * (e.g. `@property`-registered `--tw-*` slots). + */ + resolveThemeVar?: (name: string) => string | undefined; + /** + * Selector the emitted theme block attaches to. Defaults to `:root`. Pass a + * skin selector (e.g. `[data-skin="default-video"]`) to scope the variables. + */ + themeSelector?: string; } /** @@ -97,7 +111,8 @@ export async function emitCss(opts: EmitCssOptions): Promise { if (mode === 'merged') { const base = await bundleBaseCss(opts.baseCss ?? [], configDir); const body = composeRules(opts.rules, hoist, inlineVars); - return { kind: 'merged', css: joinSections(base, body) }; + const theme = buildThemeBlock(body, opts.resolveThemeVar, opts.themeSelector); + return { kind: 'merged', css: joinSections(base, theme, body) }; } // Split mode: group rules by `bag`. @@ -120,10 +135,74 @@ export async function emitCss(opts: EmitCssOptions): Promise { } const base = await bundleBaseCss(opts.baseCss ?? [], configDir); - const index = joinSections(base, importLines.join('\n')); + // Theme variables go in `index` (it loads first), resolved against every bag. + const theme = buildThemeBlock([...bags.values()].join('\n'), opts.resolveThemeVar, opts.themeSelector); + const index = joinSections(base, theme, importLines.join('\n')); return { kind: 'split', index, bags }; } +/** + * Build a leading rule that defines every `@theme` variable the emitted CSS + * references but doesn't itself declare. Resolves transitively (a theme value + * may reference further variables). Returns `''` when there's nothing to emit + * or no resolver was supplied. + */ +function buildThemeBlock( + css: string, + resolveThemeVar: ((name: string) => string | undefined) | undefined, + themeSelector: string | undefined +): string { + if (!resolveThemeVar) return ''; + + const defined = collectDefinedVars(css); + const resolved = new Map(); + const queue = [...collectReferencedVars(css)].filter((name) => !defined.has(name)); + + while (queue.length > 0) { + const name = queue.shift()!; + if (resolved.has(name) || defined.has(name)) continue; + const value = resolveThemeVar(name); + if (value === undefined) continue; + resolved.set(name, value); + for (const ref of collectReferencedVars(value)) { + if (!resolved.has(ref) && !defined.has(ref)) queue.push(ref); + } + } + + if (resolved.size === 0) return ''; + + const selector = themeSelector ?? ':root'; + const decls = [...resolved.entries()] + .sort((a, b) => a[0].localeCompare(b[0])) + .map(([name, value]) => ` ${name}: ${value};`) + .join('\n'); + return `${selector} {\n${decls}\n}`; +} + +/** Collect `var(--name)` references in a CSS string. */ +function collectReferencedVars(css: string): Set { + const out = new Set(); + const re = /var\(\s*(--[A-Za-z0-9_-]+)/g; + let m: RegExpExecArray | null = re.exec(css); + while (m !== null) { + out.add(m[1]!); + m = re.exec(css); + } + return out; +} + +/** Collect custom properties *declared* (`--name:`) in a CSS string. */ +function collectDefinedVars(css: string): Set { + const out = new Set(); + const re = /(?:^|[{;\s])(--[A-Za-z0-9_-]+)\s*:/g; + let m: RegExpExecArray | null = re.exec(css); + while (m !== null) { + out.add(m[1]!); + m = re.exec(css); + } + return out; +} + /** Internal: read each `baseCss` file via Lightning CSS, return concatenated string. */ async function bundleBaseCss(paths: readonly string[], configDir: string): Promise { if (paths.length === 0) return ''; diff --git a/packages/compiler/src/tailwind/plugin.ts b/packages/compiler/src/tailwind/plugin.ts index 186d93e4..a767f20d 100644 --- a/packages/compiler/src/tailwind/plugin.ts +++ b/packages/compiler/src/tailwind/plugin.ts @@ -1,12 +1,13 @@ import { existsSync, readFileSync } from 'node:fs'; import { dirname, isAbsolute, resolve as resolvePath } from 'node:path'; import ts from 'typescript'; +import { tagName } from '../matchers'; import { analyzeStyles, type StyleSegment, type StyleVisitor } from '../styles'; import { decompose, type UtilityCss } from './decompose'; import type { DesignSystem } from './design-system'; import { type CompiledRule, type EmittedCss, emitCss, type HoistOptions } from './emit'; import { EvaluationError, loadTokenModule, type TokenValue } from './evaluator'; -import { type DeriveClassNameOptions, deriveClassName, type NameTransform } from './naming'; +import { type DeriveClassNameOptions, DiagnosticError, deriveClassName, type NameTransform } from './naming'; /** Output target for `tailwindPlugin`. */ export type TailwindTarget = @@ -133,13 +134,20 @@ function vanillaCssPlugin(options: TailwindPluginOptions): ts.TransformerFactory return (transformContext) => { return (sourceFile) => { const rules: CompiledRule[] = []; + // Per derived class name, the sorted utility signature of the first + // element that produced it. Lets us detect when two *different* source + // elements collapse onto the same class name with *different* styles — + // emitCss would silently merge their declarations into one rule. + const signatures = new Map(); const visit: StyleVisitor = (info, factory) => { if (info.kind !== 'segments' || !info.segments) return undefined; + // Capture the narrowed segments so the closures below keep the type. + const segments = info.segments; const naming: DeriveClassNameOptions = { element: info.element, - segments: info.segments, + segments, ...(transformName ? { transformName } : {}), ...(overrides ? { overrides } : {}), }; @@ -152,12 +160,30 @@ function vanillaCssPlugin(options: TailwindPluginOptions): ts.TransformerFactory // classname to the derived semantic name and wrap any pass-throughs in // a `cn(...)` call so composition is preserved. const passThrough: ts.Expression[] = []; - for (const seg of info.segments) { + const preserved: string[] = []; + // Every utility this element resolves to (rule-producing or preserved), + // for the collision signature below. + const utilities: string[] = []; + + // Compile one utility: emit a rule when it produces declarations, + // otherwise *preserve* it as a literal class. Utilities that yield no + // declarations are markers (`group`, `peer`, `group/`) or classes + // Tailwind doesn't recognize — dropping them would silently break every + // descendant `group-*` / `peer-*` variant that targets the marker. + const handleUtility = (utility: string): void => { + utilities.push(utility); + const css = decompose(utility, design); + if (css && css.declarations.length > 0) { + rules.push(buildCompiledRule(derived.className, css, segments, bagFor)); + return; + } + if (!preserved.includes(utility)) preserved.push(utility); + }; + + for (const seg of segments) { if (seg.kind === 'literal') { for (const utility of seg.value.split(/\s+/)) { - if (!utility) continue; - const css = decompose(utility, design); - if (css) rules.push(buildCompiledRule(derived.className, css, info.segments, bagFor)); + if (utility) handleUtility(utility); } continue; } @@ -165,9 +191,7 @@ function vanillaCssPlugin(options: TailwindPluginOptions): ts.TransformerFactory const literal = resolveTokenPath(seg.path, env); if (literal !== null) { for (const utility of literal.split(/\s+/)) { - if (!utility) continue; - const css = decompose(utility, design); - if (css) rules.push(buildCompiledRule(derived.className, css, info.segments, bagFor)); + if (utility) handleUtility(utility); } continue; } @@ -176,11 +200,28 @@ function vanillaCssPlugin(options: TailwindPluginOptions): ts.TransformerFactory passThrough.push(seg.node); } + // Collision guard: two distinct elements that derive the same class + // name must resolve to the same utilities. Identical recurrences (e.g. + // many `` with the same token) share a signature and are + // fine; differing ones would merge conflicting declarations into one + // rule, so we fail loudly with a fixable diagnostic. + if (utilities.length > 0) { + const signature = [...utilities].sort().join(' '); + const previous = signatures.get(derived.className); + if (previous === undefined) { + signatures.set(derived.className, signature); + } else if (previous !== signature) { + throw collisionError(info.element, derived.className, previous, signature); + } + } + + const baseName = preserved.length > 0 ? `${derived.className} ${preserved.join(' ')}` : derived.className; + if (passThrough.length === 0) { - return factory.createStringLiteral(derived.className); + return factory.createStringLiteral(baseName); } return factory.createCallExpression(factory.createIdentifier('cn'), undefined, [ - factory.createStringLiteral(derived.className), + factory.createStringLiteral(baseName), ...passThrough, ]); }; @@ -192,11 +233,16 @@ function vanillaCssPlugin(options: TailwindPluginOptions): ts.TransformerFactory onRules?.(rules); if (onCss) { + // Scope the emitted theme variables to the skin's hoist root when one + // is configured, so they don't leak to a global `:root`. + const themeSelector = hoistVars ? hoistVars.rootSelector : undefined; emitCss({ rules, ...(emit ?? {}), ...(hoistVars !== undefined ? { hoist: hoistVars } : {}), ...(inlineVars !== undefined ? { inlineVars } : {}), + resolveThemeVar: (name) => design.resolveThemeVar(name), + ...(themeSelector ? { themeSelector } : {}), }) .then(onCss) .catch(() => { @@ -406,3 +452,21 @@ function buildCompiledRule( const bag = bagFor?.({ className, segments }); return bag === undefined ? { className, utility } : { className, utility, bag }; } + +function collisionError(element: ts.Node, className: string, first: string, next: string): DiagnosticError { + const tag = tagName(element as Parameters[0]); + const sourceFile = element.getSourceFile?.(); + const loc = sourceFile ? sourceFile.getLineAndCharacterOfPosition(element.pos) : undefined; + const fileName = sourceFile?.fileName; + const line = loc ? loc.line + 1 : undefined; + return new DiagnosticError( + `vanilla-css: class name '${className}' is derived from elements with different styles` + + `${fileName ? ` (this one at ${fileName}:${line})` : ''}.\n` + + ` <${tag}> resolves to: ${next}\n` + + ` an earlier element resolved to: ${first}\n` + + `Merging these would put conflicting declarations in a single '.${className}' rule. ` + + `Disambiguate with a distinct token, a distinct component, or an \`overrides\` entry.`, + fileName, + line + ); +} diff --git a/packages/compiler/src/tailwind/tests/emit.test.ts b/packages/compiler/src/tailwind/tests/emit.test.ts index 9fc3be4c..cbbf95b9 100644 --- a/packages/compiler/src/tailwind/tests/emit.test.ts +++ b/packages/compiler/src/tailwind/tests/emit.test.ts @@ -475,3 +475,60 @@ describe('emitCss — inlineVars', () => { expect(out.css).toMatch(/\.a\s*{[^}]*color:\s*var\(--media-color\)/); }); }); + +describe('emitCss — theme variables', () => { + it('emits a :root block defining referenced theme variables', async () => { + const out = await emitCss({ + rules: [rule('box', [{ property: 'padding', value: 'calc(var(--spacing) * 1)' }])], + resolveThemeVar: (name) => (name === '--spacing' ? '0.25rem' : undefined), + }); + if (out.kind !== 'merged') throw new Error('expected merged'); + expect(collapse(out.css)).toContain(collapse(':root {\n --spacing: 0.25rem;\n}')); + // Theme block precedes the rules that consume it. + expect(out.css.indexOf('--spacing: 0.25rem')).toBeLessThan(out.css.indexOf('.box')); + }); + + it('resolves theme variables transitively', async () => { + const out = await emitCss({ + rules: [rule('box', [{ property: 'color', value: 'var(--brand)' }])], + resolveThemeVar: (name) => + name === '--brand' ? 'var(--brand-500)' : name === '--brand-500' ? '#09f' : undefined, + }); + if (out.kind !== 'merged') throw new Error('expected merged'); + expect(collapse(out.css)).toContain(collapse('--brand: var(--brand-500);')); + expect(collapse(out.css)).toContain(collapse('--brand-500: #09f;')); + }); + + it('does not redeclare variables the rules already define', async () => { + const out = await emitCss({ + rules: [ + rule('box', [ + { property: '--spacing', value: '1rem' }, + { property: 'padding', value: 'var(--spacing)' }, + ]), + ], + resolveThemeVar: () => '0.25rem', + }); + if (out.kind !== 'merged') throw new Error('expected merged'); + // The locally-declared --spacing wins; no :root override is emitted. + expect(out.css).not.toMatch(/:root/); + }); + + it('scopes the theme block to a custom selector', async () => { + const out = await emitCss({ + rules: [rule('box', [{ property: 'gap', value: 'var(--spacing)' }])], + resolveThemeVar: (name) => (name === '--spacing' ? '0.25rem' : undefined), + themeSelector: '[data-skin="x"]', + }); + if (out.kind !== 'merged') throw new Error('expected merged'); + expect(collapse(out.css)).toContain(collapse('[data-skin="x"] {\n --spacing: 0.25rem;\n}')); + }); + + it('omits the theme block when no resolver is provided (back-compat)', async () => { + const out = await emitCss({ + rules: [rule('box', [{ property: 'gap', value: 'var(--spacing)' }])], + }); + if (out.kind !== 'merged') throw new Error('expected merged'); + expect(out.css).not.toMatch(/:root/); + }); +}); diff --git a/packages/compiler/src/tailwind/tests/plugin.test.ts b/packages/compiler/src/tailwind/tests/plugin.test.ts index fd948478..04b38f4b 100644 --- a/packages/compiler/src/tailwind/tests/plugin.test.ts +++ b/packages/compiler/src/tailwind/tests/plugin.test.ts @@ -146,6 +146,66 @@ describe('tailwindPlugin — target: vanilla-css', () => { expect(code).not.toContain('"flex items-center"'); }); + it('preserves marker utilities (group/peer) alongside the derived name', () => { + const source = `function App(){ return ; }`; + const { code } = compile(source, { + target: 'react', + plugins: [tailwindPlugin({ design, target: 'vanilla-css' })], + }); + // `group` produces no declarations but is required by descendant + // `group-*` variants, so it must survive on the element. + expect(code).toContain('"play-button group"'); + }); + + it('keeps markers and still emits rules for declaration-producing utilities', () => { + const source = `function App(){ return ; }`; + let captured: readonly CompiledRule[] | undefined; + const { code } = compile(source, { + target: 'react', + plugins: [ + tailwindPlugin({ + design, + target: 'vanilla-css', + onRules: (rules) => { + captured = rules; + }, + }), + ], + }); + expect(code).toContain('"play-button group"'); + expect(captured).toBeDefined(); + expect(captured!.map((r) => r.className)).toContain('play-button'); + expect(captured!.flatMap((r) => r.utility.declarations)).toContainEqual({ property: 'display', value: 'flex' }); + }); + + it('preserves a marker while wrapping pass-through expressions in cn()', () => { + const source = `function App(){ return ; }`; + const { code } = compile(source, { + target: 'react', + plugins: [tailwindPlugin({ design, target: 'vanilla-css' })], + }); + expect(code).toMatch(/cn\("play-button group",\s*extra\)/); + }); + + it('throws a diagnostic when two elements derive the same name with different styles', () => { + const source = `function App(){ return
; }`; + expect(() => + compile(source, { + target: 'react', + plugins: [tailwindPlugin({ design, target: 'vanilla-css' })], + }) + ).toThrow(/class name 'seek-icon' is derived from elements with different styles/); + }); + + it('does not flag identical recurrences of the same derived name', () => { + const source = `function App(){ return
; }`; + const { code } = compile(source, { + target: 'react', + plugins: [tailwindPlugin({ design, target: 'vanilla-css' })], + }); + expect(code).toContain('"play-button"'); + }); + it('rewrites className to a token-path-derived name on a bare HTML element', () => { const source = `function App(){ return
; }`; const { code } = compile(source, { @@ -381,4 +441,27 @@ function App(){ return ; }`; const css = await cssPromise; expect(collapse(css)).toContain(collapse('.foo{display:flex;}')); }); + + it('emits referenced theme variables in the onCss output', async () => { + // `p-4` lowers to `padding: calc(var(--spacing) * 4)` — the output must + // define `--spacing` so it resolves without a separate Tailwind theme. + const source = `function App(){ return ; }`; + const cssPromise = new Promise((resolve) => { + compile(source, { + target: 'react', + plugins: [ + tailwindPlugin({ + design, + target: 'vanilla-css', + hoistVars: { rootSelector: '[data-skin="x"]' }, + onCss: (out) => { + if (out.kind === 'merged') resolve(out.css); + }, + }), + ], + }); + }); + const css = await cssPromise; + expect(css).toMatch(/\[data-skin="x"\]\s*{[^}]*--spacing:/); + }); });