-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
fix(compiler-ssr): ensure v-show has a higher priority in SSR #12171
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
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
WalkthroughSSR transform now reorders v-show to be processed last and updates SSR codegen/tests to merge _attrs directly via _mergeProps. A new test covers v-show combined with inline display style to validate composed style generation with _ssrRenderStyle. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Template Compiler (SSR)
participant N as AST Node (Element)
participant P as Props Processor
participant G as SSR Codegen
T->>N: Parse element with props (incl. v-show)
N->>P: Provide props array
P->>P: Reorder props (move v-show to end)
P->>G: Emit merged props via _mergeProps({... , ...})
G->>G: Compose style via _ssrRenderStyle(display + v-show toggle)
G-->>T: Return SSR render output
note over P,G: v-show handled last to finalize display style and merging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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 (2)
packages/compiler-ssr/src/transforms/ssrTransformElement.ts (1)
92-101
: Reordering v-show last is correct; add a small guard to avoid no-op mutation.This achieves the intended precedence. Minor polish: skip splice/push when v-show is already last; it avoids unnecessary array churn. Optionally, in dev builds, warn on duplicate v-show.
- const vShowPropIndex = node.props.findIndex( + const vShowPropIndex = node.props.findIndex( i => i.type === NodeTypes.DIRECTIVE && i.name === 'show', ) - if (vShowPropIndex !== -1) { - const vShowProp = node.props[vShowPropIndex] - node.props.splice(vShowPropIndex, 1) - node.props.push(vShowProp) - } + const lastIndex = node.props.length - 1 + if (vShowPropIndex > -1 && vShowPropIndex !== lastIndex) { + const [vShowProp] = node.props.splice(vShowPropIndex, 1) + node.props.push(vShowProp) + }packages/compiler-ssr/__tests__/ssrVShow.spec.ts (1)
95-111
: Good added case; consider two more to harden coverage.Add:
- dynamic display in bound style to ensure v-show still wins,
- root element variant (no wrapper) with inline display.
+ test('with dynamic style + display (v-show overrides)', () => { + const code = compileWithWrapper(`<div :style="{ display: 'flex' }" v-show="foo"/>`).code + expect(code).toMatch( + /_ssrRenderStyle\(\s*\[\s*\{ display: 'flex' \s*\},\s*\(_ctx\.foo\)\s*\?\s*null\s*:\s*\{ display: "none" \}\s*\]\s*\)/ + ) + }) + + test('root: with style + display', () => { + const code = compile(`<div v-show="foo" style="display:flex"/>`).code + expect(code).toMatch(/_mergeProps\(_attrs,\s*\{\s*style:\s*\[?\s*[\s\S]*display["']?\s*:\s*["']?flex["']?[\s\S]*\(_ctx\.foo\)\s*\?\s*null\s*:\s*\{ display: "none" \}/) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/compiler-ssr/__tests__/ssrVShow.spec.ts
(2 hunks)packages/compiler-ssr/src/transforms/ssrTransformElement.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test / e2e-test
🔇 Additional comments (1)
packages/compiler-ssr/__tests__/ssrVShow.spec.ts (1)
14-16
: Snapshot change looks good.Merging
_attrs
as the first arg to_mergeProps
is consistent with current SSR attr precedence.
close #12162
Summary by CodeRabbit