fix(compiler-ssr): add selected option attribute from select value #13539
Conversation
WalkthroughThis change refactors and extends the SSR handling of Changes
Sequence Diagram(s)sequenceDiagram
participant TemplateCompiler
participant ssrTransformElement
participant ssrVModel
participant Utils (processSelectChildren)
participant SSRRuntime
TemplateCompiler->>ssrTransformElement: Process <select> with value/v-bind
ssrTransformElement->>Utils (processSelectChildren): Delegate children processing
ssrVModel->>Utils (processSelectChildren): Delegate children processing (v-model)
Utils (processSelectChildren)->>Utils (processOption): For each <option>, determine selected
Utils (processOption)->>SSRRuntime: Emit SSR code for selected attribute
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/compiler-ssr/src/utils.ts (1)
68-80: Consider extracting createDynamicSelectExpression for performance.The function is defined inside
processOptionwhich could impact performance if this code path is executed frequently during compilation. Consider moving it to module scope.Extract the function to module scope:
+function createDynamicSelectExpression( + context: TransformContext, + selectValue: ExpressionNode, + value: ExpressionNode, +): ExpressionNode { + return createConditionalExpression( + createCallExpression(`Array.isArray`, [selectValue]), + createCallExpression(context.helper(SSR_LOOSE_CONTAIN), [ + selectValue, + value, + ]), + createCallExpression(context.helper(SSR_LOOSE_EQUAL), [ + selectValue, + value, + ]), + ) +} export function processOption( context: TransformContext, plainNode: PlainElementNode, selectValue: SelectValue, ): void { if (plainNode.tag === 'option') { if (plainNode.props.findIndex(p => p.name === 'selected') === -1) { const value = findValueBinding(plainNode) - function createDynamicSelectExpression(selectValue: ExpressionNode) { - return createConditionalExpression( - createCallExpression(`Array.isArray`, [selectValue]), - createCallExpression(context.helper(SSR_LOOSE_CONTAIN), [ - selectValue, - value, - ]), - createCallExpression(context.helper(SSR_LOOSE_EQUAL), [ - selectValue, - value, - ]), - ) - }Then update the calls to pass the required parameters:
- : selectValue.type === 'dynamicValue' - ? createDynamicSelectExpression(selectValue.value) + : selectValue.type === 'dynamicValue' + ? createDynamicSelectExpression(context, selectValue.value, value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/compiler-ssr/__tests__/ssrElement.spec.ts(2 hunks)packages/compiler-ssr/src/transforms/ssrTransformElement.ts(5 hunks)packages/compiler-ssr/src/transforms/ssrVModel.ts(2 hunks)packages/compiler-ssr/src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/compiler-ssr/src/transforms/ssrVModel.ts (1)
packages/compiler-ssr/src/utils.ts (1)
processSelectChildren(41-57)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (3)
packages/compiler-ssr/src/transforms/ssrVModel.ts (1)
21-21: Good refactoring to shared utilities.The extraction of
processSelectChildrenandfindValueBindingto a shared utilities module improves code reusability and maintainability.Also applies to: 130-133
packages/compiler-ssr/__tests__/ssrElement.spec.ts (1)
76-228: Excellent test coverage for select element SSR.The comprehensive test suite covers all binding scenarios (dynamic value, static value, v-bind combinations) and validates both the generated code and runtime HTML output. This ensures the SSR behavior matches expectations.
packages/compiler-ssr/src/transforms/ssrTransformElement.ts (1)
433-443: Smart generalization of the value binding check.The refactoring from
isTextareaWithValuetoisTagWithValueBindwith atargetTagparameter elegantly supports both<textarea>and<select>elements without code duplication.
| plainNode.ssrCodegenNode!.elements.push( | ||
| createConditionalExpression( | ||
| createCallExpression(context.helper(SSR_INCLUDE_BOOLEAN_ATTR), [ | ||
| selectValue.type === 'staticValue' | ||
| ? createCallExpression(context.helper(SSR_LOOSE_EQUAL), [ | ||
| createSimpleExpression(selectValue.value, true), | ||
| value, | ||
| ]) | ||
| : selectValue.type === 'dynamicValue' | ||
| ? createDynamicSelectExpression(selectValue.value) | ||
| : createConditionalExpression( | ||
| createSimpleExpression( | ||
| `"value" in ${selectValue.tempId}`, | ||
| false, | ||
| ), | ||
| createDynamicSelectExpression( | ||
| createSimpleExpression( | ||
| `${selectValue.tempId}.value`, | ||
| false, | ||
| ), | ||
| ), | ||
| createSimpleExpression('false', false), | ||
| ), | ||
| ]), | ||
| createSimpleExpression(' selected', true), | ||
| createSimpleExpression('', true), | ||
| false /* no newline */, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Add safety check for ssrCodegenNode.
The code assumes plainNode.ssrCodegenNode exists when pushing elements. Consider adding a safety check to prevent potential runtime errors.
Add a safety check:
+ if (plainNode.ssrCodegenNode) {
plainNode.ssrCodegenNode!.elements.push(
createConditionalExpression(
// ... rest of the code
),
)
+ }🤖 Prompt for AI Agents
In packages/compiler-ssr/src/utils.ts between lines 82 and 110, the code pushes
elements to plainNode.ssrCodegenNode without verifying its existence, which can
cause runtime errors if ssrCodegenNode is undefined. Add a safety check to
confirm plainNode.ssrCodegenNode is defined before pushing elements, and if it
is not, initialize it appropriately to ensure safe access.
There was a problem hiding this comment.
Note: the ! was not added with this change but was rather moved to this file from ssrVModel.ts
There was a problem hiding this comment.
@alex-snezhko Thank you for the clarification! You're absolutely right - since this code was moved from the existing ssrVModel.ts implementation where it was already working correctly, the non-null assertion was part of the established, tested logic. My suggestion for a safety check was based on treating this as new code, but given that it's a refactoring of existing functionality, the current approach is appropriate. I appreciate you providing that context!
edison1105
added
scope: ssr
has workaround
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
close #13528
Example:
Summary by CodeRabbit
<select>elements with dynamic and static value bindings, ensuring accurate rendering of theselectedoption.<select>elements in SSR output.<select>binding patterns to validate SSR correctness.<select>elements into shared utilities for better maintainability and code clarity.