fix(compiler-sfc): preserve selectors after global pseudo#14763
fix(compiler-sfc): preserve selectors after global pseudo#14763kirchoni wants to merge 1 commit intovuejs:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR fixes incorrect CSS selector compilation where Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/compiler-sfc/__tests__/compileStyle.spec.ts (1)
243-252: Consider also covering the combinator case from issue#12404.The linked issue explicitly mentions that descendant and combinator selectors (e.g.
>) are affected. The two new assertions only exercise the descendant (whitespace) combinator. Adding one combinator case would lock in coverage of the full reported bug surface:✅ Suggested extra assertion
expect(compileScoped(`:global(html.dark) .foo { color: red; }`)) .toMatchInlineSnapshot(` "html.dark .foo[data-v-test] { color: red; }" `) + expect(compileScoped(`:global(html) > body .foo { color: red; }`)) + .toMatchInlineSnapshot()(Run vitest with
-uto populate the snapshot.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/compiler-sfc/__tests__/compileStyle.spec.ts` around lines 243 - 252, Add a test case to cover the combinator selector scenario (e.g. the child combinator `>`) in the same test file exercising the compileScoped helper; the issue shows both descendant and combinator selectors can break, so add an assertion calling compileScoped with a selector like `:global(body) > h1 { color: red; }` (or similar) and verify the snapshot matches the expected transformed output (run vitest -u to update snapshot). Locate the existing assertions that call compileScoped in this spec (the two lines asserting `:global(body) h1` and `:global(html.dark) .foo`) and add the new combinator assertion nearby so the combinator case is covered.packages/compiler-sfc/src/style/pluginScoped.ts (2)
346-372: Helper is clean; minor return-value semantics suggestion.
return !hasTrailingNodescouples two ideas (whether unwrapping happened, and whether the caller should skip). Consider returninghasTrailingNodes(orunwrapped+ a separate flag) and letting the caller invert, or renaming to make the contract self-documenting, e.g.:♻️ Optional clarity tweak
-function rewriteGlobalSelector(selector: selectorParser.Selector): boolean { +/** + * Unwraps a top-level `:global(...)` / `::v-global(...)` pseudo in `selector`. + * Returns true when the selector consists solely of the global portion and + * therefore should skip the normal scoped-attribute injection. + */ +function rewriteGlobalSelector(selector: selectorParser.Selector): boolean {Behavior is fine as-is; purely a readability nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/compiler-sfc/src/style/pluginScoped.ts` around lines 346 - 372, Summary: The return value of rewriteGlobalSelector is confusing because it returns !hasTrailingNodes which couples unwrapping and caller-skip semantics; change it to return hasTrailingNodes (or an explicit flag) and update callers accordingly. Fix: modify rewriteGlobalSelector to return hasTrailingNodes (or an object like {unwrapped: true, hasTrailingNodes}) instead of !hasTrailingNodes, and then update all call sites that rely on the old boolean to invert the check (or destructure the result if using an object); reference the function rewriteGlobalSelector and the local variable hasTrailingNodes to locate and change the logic and caller handling.
250-255: Legacy:globalbranch is now mostly dead code.Since
rewriteGlobalSelectorruns beforeselector.eachand unwraps the first top-level:global/::v-global(early-returning when there are no trailing nodes), this branch is only reachable if a second:globalappears in the same compound/descendant chain (e.g.:global(.a) > :global(.b)), in which case it still does the old wholesaleselector.replaceWith(n.nodes[0])and silently drops.a >.Consider either removing this branch (and letting
rewriteGlobalSelectorloop over all:globaloccurrences) or keeping it but documenting the two-:globallimitation. Not a regression, but worth a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/compiler-sfc/src/style/pluginScoped.ts` around lines 250 - 255, The branch handling value === ':global' || value === '::v-global' in pluginScoped.ts is legacy dead code that can drop preceding selector parts when multiple :global occur; remove this branch (the selector.replaceWith(n.nodes[0]) early-return) so rewriteGlobalSelector and the surrounding selector.each loop handle all :global/::v-global occurrences consistently, then run/update tests for rewriteGlobalSelector and selector.each to ensure multiple :global() in a compound/descendant chain are unwrapped correctly and no parts are lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/compiler-sfc/__tests__/compileStyle.spec.ts`:
- Around line 243-252: Add a test case to cover the combinator selector scenario
(e.g. the child combinator `>`) in the same test file exercising the
compileScoped helper; the issue shows both descendant and combinator selectors
can break, so add an assertion calling compileScoped with a selector like
`:global(body) > h1 { color: red; }` (or similar) and verify the snapshot
matches the expected transformed output (run vitest -u to update snapshot).
Locate the existing assertions that call compileScoped in this spec (the two
lines asserting `:global(body) h1` and `:global(html.dark) .foo`) and add the
new combinator assertion nearby so the combinator case is covered.
In `@packages/compiler-sfc/src/style/pluginScoped.ts`:
- Around line 346-372: Summary: The return value of rewriteGlobalSelector is
confusing because it returns !hasTrailingNodes which couples unwrapping and
caller-skip semantics; change it to return hasTrailingNodes (or an explicit
flag) and update callers accordingly. Fix: modify rewriteGlobalSelector to
return hasTrailingNodes (or an object like {unwrapped: true, hasTrailingNodes})
instead of !hasTrailingNodes, and then update all call sites that rely on the
old boolean to invert the check (or destructure the result if using an object);
reference the function rewriteGlobalSelector and the local variable
hasTrailingNodes to locate and change the logic and caller handling.
- Around line 250-255: The branch handling value === ':global' || value ===
'::v-global' in pluginScoped.ts is legacy dead code that can drop preceding
selector parts when multiple :global occur; remove this branch (the
selector.replaceWith(n.nodes[0]) early-return) so rewriteGlobalSelector and the
surrounding selector.each loop handle all :global/::v-global occurrences
consistently, then run/update tests for rewriteGlobalSelector and selector.each
to ensure multiple :global() in a compound/descendant chain are unwrapped
correctly and no parts are lost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91800e4d-7c01-4714-b70e-207fff89d129
📒 Files selected for processing (2)
packages/compiler-sfc/__tests__/compileStyle.spec.tspackages/compiler-sfc/src/style/pluginScoped.ts
|
duplicate of #12416 |
2 participants
fix #12404
👨: Hey, real human speaking here. Not sure what's your AI code policy, but I encountered this issue while trying to implement a css-vars based theme/switcher in a sli.dev project, and dedicated a couple of tokens to an agent to try and fix it.
🤖: When a scoped selector started with :global(...) and then continued with a descendant or combinator selector, the scoped CSS transform replaced the entire selector with the :global() inner selector. That dropped the trailing local selector and compiled :global(body) h1 to body.
This updates the :global()/::v-global rewrite to replace only the pseudo node, preserve trailing selector nodes for normal scoped processing, and keep standalone global selectors unscoped.
Added regression coverage for both :global(body) h1 and :global(html.dark) .foo.
Validation: