-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
v4: Allow @apply to be used with CSS mixins
#19427
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
base: main
Are you sure you want to change the base?
Conversation
@apply with CSS mixins@apply to be used with CSS mixins
WalkthroughAdds support for using Pre-merge checks✅ Passed checks (4 passed)
Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/tailwindcss/src/apply.ts (1)
204-215: Stale comment should be updated.The comment at lines 204-208 references "dashed ident" but the actual validation checks for a body combined with normal idents (utilities). This appears to be a copy-paste artifact from the earlier dashed ident check.
- // If we find a dashed ident *here* it means that someone is trying - // to use mixins and our `@apply` behavior together. - // - // This is considered invalid usage and we want to inform the user - // as such. + // If we find a body with normal idents, we want to inform the user + // that `@apply` with utilities must not have a body. let hasBody = child.nodes.length > 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/tailwindcss/src/apply.ts(1 hunks)packages/tailwindcss/src/ast.ts(1 hunks)packages/tailwindcss/src/index.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/tailwindcss/src/apply.ts (1)
packages/tailwindcss/src/walk.ts (1)
WalkAction(10-20)
packages/tailwindcss/src/index.test.ts (2)
packages/tailwindcss/src/index.ts (1)
compile(819-855)integrations/utils.ts (1)
css(519-519)
🔇 Additional comments (5)
packages/tailwindcss/src/apply.ts (2)
166-182: LGTM - Dashed ident categorization logic is correct.The detection of CSS custom property syntax (
--) for mixins is appropriate, and usingidx % 2 === 0correctly filters out whitespace parts from the split result.
184-202: LGTM - Mixin passthrough and mixing validation.The logic correctly:
- Preserves pure CSS mixin
@applyrules by returningWalkAction.Skip- Errors when mixing mixins with utilities, forcing users to separate them
This aligns well with the CSS Custom Functions and Mixins spec behavior.
packages/tailwindcss/src/ast.ts (1)
397-408: LGTM - Necessary for CSS mixin passthrough.Adding
@applyto the retention list ensures that CSS mixin@applyrules (those containing only dashed idents) are preserved in the final output after being skipped during the substitution phase. This correctly complements theWalkAction.Skipreturn inapply.ts.packages/tailwindcss/src/index.test.ts (2)
859-889: LGTM - Comprehensive CSS mixin test coverage.The test effectively validates all key mixin scenarios:
- Single and multiple dashed idents
- Mixins with and without parentheses
- Mixins with a body block
- Coexistence with utility processing (
underlineis compiled while mixins pass through)The snapshot correctly shows mixins preserved verbatim in output.
891-925: LGTM - Error case coverage is thorough.Both orderings of the mixed syntax (
underline --mixinand--mixin underline) are tested, ensuring the validation isn't order-dependent. The body error test completes the validation coverage.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/tailwindcss/src/apply.ts (1)
170-182: Consider guarding against empty parts.If
child.paramsever contains leading/trailing whitespace,split(/(\s+)/g)produces empty strings at even indices (e.g.," --mixin "→["", " ", "--mixin", " ", ""]). Empty strings would be pushed tonormalIdentsand passed tocompileCandidates, likely causing a confusing error.A simple guard avoids this edge case:
for (let [idx, part] of parts.entries()) { if (idx % 2 === 0) { + if (!part) continue + if (part[0] === '-' && part[1] === '-') { dashedIdents.push(part) } else { normalIdents.push(part) } candidateOffsets[part] = offset } offset += part.length }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tailwindcss/src/apply.ts(1 hunks)
🔇 Additional comments (3)
packages/tailwindcss/src/apply.ts (3)
184-202: LGTM!The logic correctly distinguishes between pure CSS mixin usage (passed through) and mixed usage (error thrown). The error message is clear and actionable, guiding users to split their
@applyrules.
204-210: LGTM!The body validation correctly ensures that utility-based
@applyrules don't have a body, while allowing CSS mixin@applyrules (handled earlier via early return) to include a body for the@slotsyntax per the CSS Mixins spec.
164-182: Control flow is sound.Verified the logic flow:
- Pure dashed idents → early return at line 190,
@applypreserved- Mixed idents → error thrown at line 199
- Pure normal idents →
dashedIdents.lengthis 0, block skipped, proceeds to utility replacementWhen
compileCandidatesis reached at line 217,candidateOffsetscontains only utility candidates as expected.Also applies to: 184-210
The CSS Custom Functions and Mixins spec is using
@applywith dashed idents for native mixin support. We shouldn't attempt to treat these as utilities and try to compile them since they'll fail.There one intentional limitation with regards to mixins and our use of
@apply: We do not allow users to mix utilities and mixins in the same at-rule. In other words, all of the following@applyrules are invalid:Aside: Lightning CSS does not yet support this syntax so the results of a production build won't produce the correct code but we'll at least handle these correctly in Tailwind CSS itself.
Fixes #19422