Repository navigation
fix(compiler-ssr): textarea with v-text directive SSR - #13975
edison1105 merged 2 commits into
Conversation
WalkthroughAdds a helper to detect content override directives (v-text/v-html) in the SSR element transform and updates branching to account for overrides when merging props and deriving inner content (notably for textarea). Also adds SSR tests for v-html on span and v-text/v-html on textarea. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as SSR Compiler Transform
participant N as Element Node
participant M as MergeProps/Emit
participant R as SSR Output
C->>N: inspect tag, props, children
C->>N: call hasContentOverrideDirective(node)?
alt node is textarea
alt override present OR no interpolation
C-->>M: skip merging children into value
C->>R: set textContent/innerHTML from directive
else interpolation present and no override
C->>M: merge props and textarea value
M->>R: emit with interpolated value
end
else non-textarea
alt override present and no children
C-->>M: skip children merge
C->>R: set innerHTML/textContent from directive
else normal flow
C->>M: merge props and children
M->>R: emit with children
end
end
note right of C: Decision now uses hasContentOverrideDirective(node)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
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: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/compiler-ssr/src/transforms/ssrTransformElement.ts (1)
456-458: LGTM! Clean helper function.The helper function is clear and concise. It correctly identifies whether an element has a content override directive (v-text or v-html).
Optional micro-optimization: The function calls
findDirtwice. If performance becomes a concern, you could reduce this to a single call:function hasContentOverrideDirective(node: PlainElementNode): boolean { return !!node.props.some( p => p.type === NodeTypes.DIRECTIVE && (p.name === 'text' || p.name === 'html'), ) }However, given that the props array is typically small, the current implementation is fine and arguably more readable by leveraging the existing
findDirutility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/compiler-ssr/src/transforms/ssrTransformElement.ts(3 hunks)packages/server-renderer/__tests__/ssrDirectives.spec.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-ssr/src/transforms/ssrTransformElement.ts (2)
packages/compiler-core/src/ast.ts (1)
PlainElementNode(140-149)packages/compiler-core/src/utils.ts (1)
findDir(282-297)
⏰ 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). (4)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- GitHub Check: test / e2e-test
🔇 Additional comments (3)
packages/server-renderer/__tests__/ssrDirectives.spec.ts (1)
197-217: LGTM! Tests adequately cover the bug fix.The tests for v-text and v-html on textarea elements correctly validate the SSR output, ensuring the content override directives work as expected.
Note: These tests should be moved to a separate describe block as noted in the earlier comment.
packages/compiler-ssr/src/transforms/ssrTransformElement.ts (2)
125-131: LGTM! Condition correctly handles content override directives.The updated condition properly skips the merged props assignment logic when a content override directive (v-text/v-html) is present on a textarea element. This ensures that v-text/v-html directives take precedence over dynamic v-bind values, which aligns with the PR objective.
The logic flow is correct:
- Skip special handling if
hasContentOverrideDirective(node)returns true- OR skip if there's no existing text or the existing text is not an interpolation
184-185: LGTM! Generalized condition correctly covers both v-text and v-html.Replacing the explicit check with
hasContentOverrideDirective(node)is the right approach. This ensures that both v-text and v-html directives prevent the fallback textContent/innerHTML logic from being applied, since these directives will be handled separately later in the transform.
| } | ||
|
|
||
| function hasContentOverrideDirective(node: PlainElementNode): boolean { | ||
| return !!findDir(node, 'text') || !!findDir(node, 'html') |
There was a problem hiding this comment.
This existing line here: https://github.com/vuejs/core/pull/13975/files#diff-40b0d34e7fda1abbad1716852aa8a9db97fdbfce639710889dc62423f247a834R230 actually already handles v-html output being correct since it would result in rawChildrenMap always being updated. Because of this, the v-html lookup done here is not strictly necessary for v-html SSR output to work but including it here would at least short-circuit past the _ssrInterpolate expression node creation which is desired in this case; I also think including it here makes the logic more expressive.
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
Fix SSR on
textareaelements with av-textdirective valuePlayground
Related to #12311 but slightly different as this bug would be triggered for
textareaeven without custom user directives.Summary by CodeRabbit
Bug Fixes
Tests