-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Handle @variant inside @custom-variant
#18885
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
Changes from all commits
f628ea8
0dc5ab5
19949cc
e3b4c4c
5f52530
8ef8419
6636897
866ab4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ import { substituteAtImports } from './at-import' | |
| import { applyCompatibilityHooks } from './compat/apply-compat-hooks' | ||
| import type { UserConfig } from './compat/config/types' | ||
| import { type Plugin } from './compat/plugin-api' | ||
| import { applyVariant, compileCandidates } from './compile' | ||
| import { compileCandidates } from './compile' | ||
| import { substituteFunctions } from './css-functions' | ||
| import * as CSS from './css-parser' | ||
| import { buildDesignSystem, type DesignSystem } from './design-system' | ||
|
|
@@ -32,7 +32,8 @@ import { createCssUtility } from './utilities' | |
| import { expand } from './utils/brace-expansion' | ||
| import { escape, unescape } from './utils/escape' | ||
| import { segment } from './utils/segment' | ||
| import { compoundsForSelectors, IS_VALID_VARIANT_NAME } from './variants' | ||
| import { topologicalSort } from './utils/topological-sort' | ||
| import { compoundsForSelectors, IS_VALID_VARIANT_NAME, substituteAtVariant } from './variants' | ||
| export type Config = UserConfig | ||
|
|
||
| const IS_VALID_PREFIX = /^[a-z]+$/ | ||
|
|
@@ -150,7 +151,8 @@ async function parseCss( | |
|
|
||
| let important = null as boolean | null | ||
| let theme = new Theme() | ||
| let customVariants: ((designSystem: DesignSystem) => void)[] = [] | ||
| let customVariants = new Map<string, (designSystem: DesignSystem) => void>() | ||
| let customVariantDependencies = new Map<string, Set<string>>() | ||
| let customUtilities: ((designSystem: DesignSystem) => void)[] = [] | ||
| let firstThemeRule = null as StyleRule | null | ||
| let utilitiesNode = null as AtRule | null | ||
|
|
@@ -390,7 +392,7 @@ async function parseCss( | |
| } | ||
| } | ||
|
|
||
| customVariants.push((designSystem) => { | ||
| customVariants.set(name, (designSystem) => { | ||
| designSystem.variants.static( | ||
| name, | ||
| (r) => { | ||
|
|
@@ -411,6 +413,7 @@ async function parseCss( | |
| }, | ||
| ) | ||
| }) | ||
| customVariantDependencies.set(name, new Set<string>()) | ||
|
|
||
| return | ||
| } | ||
|
|
@@ -431,9 +434,17 @@ async function parseCss( | |
| // } | ||
| // ``` | ||
| else { | ||
| customVariants.push((designSystem) => { | ||
| designSystem.variants.fromAst(name, node.nodes) | ||
| let dependencies = new Set<string>() | ||
| walk(node.nodes, (child) => { | ||
| if (child.kind === 'at-rule' && child.name === '@variant') { | ||
| dependencies.add(child.params) | ||
| } | ||
| }) | ||
|
|
||
| customVariants.set(name, (designSystem) => { | ||
| designSystem.variants.fromAst(name, node.nodes, designSystem) | ||
| }) | ||
| customVariantDependencies.set(name, dependencies) | ||
|
|
||
| return | ||
| } | ||
|
|
@@ -605,8 +616,27 @@ async function parseCss( | |
| sources, | ||
| }) | ||
|
|
||
| for (let customVariant of customVariants) { | ||
| customVariant(designSystem) | ||
| for (let name of customVariants.keys()) { | ||
| // Pre-register the variant to ensure its position in the variant list is | ||
| // based on the order we see them in the CSS. | ||
| designSystem.variants.static(name, () => {}) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Open for suggestions here. Like maybe a But my idea was to have the same behavior as-if you are overwriting internal variants that should maintain the sort order. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's an argument for keeping the position of the lastly defined There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this is fine. Keeping the last defined one would probably be more CSS-y but it would make overriding builtin variants different from custom ones and I'd prefer that they act the same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep agree |
||
| } | ||
|
|
||
| // Register custom variants in order | ||
| for (let variant of topologicalSort(customVariantDependencies, { | ||
| onCircularDependency(path, start) { | ||
| let output = toCss( | ||
| path.map((name, idx) => { | ||
| return atRule('@custom-variant', name, [atRule('@variant', path[idx + 1] ?? start, [])]) | ||
| }), | ||
| ) | ||
| .replaceAll(';', ' { … }') | ||
| .replace(`@custom-variant ${start} {`, `@custom-variant ${start} { /* ← */`) | ||
|
|
||
| throw new Error(`Circular dependency detected in custom variants:\n\n${output}`) | ||
| }, | ||
| })) { | ||
| customVariants.get(variant)?.(designSystem) | ||
| } | ||
|
|
||
| for (let customUtility of customUtilities) { | ||
|
|
@@ -636,30 +666,7 @@ async function parseCss( | |
| firstThemeRule.nodes = [context({ theme: true }, nodes)] | ||
| } | ||
|
|
||
| // Replace the `@variant` at-rules with the actual variant rules. | ||
| if (variantNodes.length > 0) { | ||
| for (let variantNode of variantNodes) { | ||
| // Starting with the `&` rule node | ||
| let node = styleRule('&', variantNode.nodes) | ||
|
|
||
| let variant = variantNode.params | ||
|
|
||
| let variantAst = designSystem.parseVariant(variant) | ||
| if (variantAst === null) { | ||
| throw new Error(`Cannot use \`@variant\` with unknown variant: ${variant}`) | ||
| } | ||
|
|
||
| let result = applyVariant(node, variantAst, designSystem.variants) | ||
| if (result === null) { | ||
| throw new Error(`Cannot use \`@variant\` with variant: ${variant}`) | ||
| } | ||
|
|
||
| // Update the variant at-rule node, to be the `&` rule node | ||
| Object.assign(variantNode, node) | ||
| } | ||
| features |= Features.Variants | ||
| } | ||
|
|
||
| features |= substituteAtVariant(ast, designSystem) | ||
| features |= substituteFunctions(ast, designSystem) | ||
| features |= substituteAtApply(ast, designSystem) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| export function topologicalSort<Key>( | ||
| graph: Map<Key, Set<Key>>, | ||
| options: { onCircularDependency: (path: Key[], start: Key) => void }, | ||
| ): Key[] { | ||
| let seen = new Set<Key>() | ||
| let wip = new Set<Key>() | ||
|
|
||
| let sorted: Key[] = [] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tested this with a very simple graph: let graph = new Map<string, Set<string>>([
['A', new Set(['B', 'C'])],
['B', new Set(['D'])],
['C', new Set(['D'])],
['D', new Set(['E'])],
['E', new Set()],
])But I don't think the real graphs will be much more complex anyway... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I'd keep the array in this case. The perf tradeoff makes sense when the memory hit is possibly unbounded but you're likely to hit other perf and memory problems with large CSS files first. |
||
|
|
||
| function visit(node: Key, path: Key[] = []) { | ||
| if (!graph.has(node)) return | ||
| if (seen.has(node)) return | ||
|
|
||
| // Circular dependency detected | ||
| if (wip.has(node)) options.onCircularDependency?.(path, node) | ||
|
|
||
| wip.add(node) | ||
|
|
||
| for (let dependency of graph.get(node) ?? []) { | ||
| path.push(node) | ||
| visit(dependency, path) | ||
| path.pop() | ||
| } | ||
|
|
||
| seen.add(node) | ||
| wip.delete(node) | ||
|
|
||
| sorted.push(node) | ||
| } | ||
|
|
||
| for (let node of graph.keys()) { | ||
| visit(node) | ||
| } | ||
|
|
||
| return sorted | ||
| } | ||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kinda wish we didn't have to pass the design system into something hanging off the design system :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, one way of solving this is that we put a
variantFromAstonto theDesignSystemitself 🤔or, we can pass in a callback if we want to do something with the
ast(body in this case) so the logic lives in the callsite.I think for now, this is fine because it's all private anyway.