fix(ssr): handle v-bind modifiers during render attrs #14263
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughHandles render-time prop modifiers during SSR: strips a leading Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Pull request overview
This PR adds support for props modifiers (^ and .) in render functions for server-side rendering (SSR), addressing issue #14262. These modifiers allow developers to explicitly control whether props are set as HTML attributes or DOM properties when using render functions with the h() API.
- Implements
applyModifiersfunction to handle^(force as attribute) and.(force as property/skip) modifiers - Integrates modifier handling into the
ssrRenderAttrsfunction - Adds integration test to verify the modifier behavior in SSR context
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/server-renderer/src/helpers/ssrRenderAttrs.ts | Adds applyModifiers function and integrates it into ssrRenderAttrs to handle ^ and . modifiers, stripping the prefix for attribute rendering or skipping props marked with . |
| packages/server-renderer/tests/render.spec.ts | Adds integration test verifying that ^attr renders as an attribute and .prop is correctly ignored in SSR output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/server-renderer/__tests__/render.spec.ts (1)
1232-1248: Test correctly validates the core functionality.The test case properly verifies that:
^attrmodifier strips the^prefix and renders as an attribute.propmodifier causes the prop to be ignored in SSR outputOptional: Consider expanding test coverage
Additional test cases could improve confidence:
test('props modifiers in render functions', async () => { const app = createApp({ setup() { return () => h( 'div', { '^attr': 'attr', '.prop': 'prop', }, 'Functional Component', ) }, }) const html = await render(app) expect(html).toBe(`<div attr="attr">Functional Component</div>`) }) + +test('props modifiers with normal props', async () => { + const app = createApp({ + setup() { + return () => + h( + 'div', + { + id: 'test', + '^data-attr': 'value', + '.domProp': 'ignored', + class: 'foo', + }, + 'Mixed', + ) + }, + }) + const html = await render(app) + expect(html).toBe(`<div id="test" class="foo" data-attr="value">Mixed</div>`) +})packages/server-renderer/src/helpers/ssrRenderAttrs.ts (1)
27-35: Implementation correctly handles SSR modifier prefixes.The logic properly transforms prop keys:
^prefix → strips prefix for attribute binding.prefix → returns null to skip DOM properties in SSR- No prefix → passes through unchanged
Optional: Add defensive checks for edge cases
Consider handling edge cases where the key is only a prefix or has multiple prefixes:
const applyModifiers = (prop: string): string | null => { if (prop.startsWith('^')) { - return prop.slice(1) // attribute binding + const stripped = prop.slice(1) + // Avoid empty keys or nested modifiers + return stripped && !stripped.startsWith('^') && !stripped.startsWith('.') + ? stripped + : prop.slice(1) // attribute binding } if (prop.startsWith('.')) { return null // prop should be ignored } return prop // normal prop }However, these are unlikely edge cases in practice and the current implementation is sufficient for typical usage.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server-renderer/__tests__/render.spec.tspackages/server-renderer/src/helpers/ssrRenderAttrs.ts
🔇 Additional comments (1)
packages/server-renderer/src/helpers/ssrRenderAttrs.ts (1)
58-62: Modifier application is correctly integrated with Vue's documented behavior.The implementation properly applies modifiers and skips props that return null. Props prefixed with
.(DOM properties) are correctly skipped in SSR since DOM properties don't exist on the server—only HTML attributes are serialized. Props prefixed with^are rendered as HTML attributes with the prefix removed. This aligns with Vue 3's documented render-function prop modifier semantics:
.→ DOM property (skipped in SSR)^→ HTML attribute (rendered in SSR)The placement in the else branch ensures modifiers apply to all dynamic props, excluding class, style, className, and ignored props.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
KazariEX
added
scope: ssr
🍰 p2-nice-to-have
|
Thanks for the PR, and I've made small tweaks. |
edison1105
changed the title
edison1105
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server-renderer/__tests__/render.spec.ts (1)
1233-1249: Test validates basic modifier behavior.The test correctly verifies that:
'^attr'renders asattr="attr"(attribute modifier)'.prop'is omitted from output (property modifier)This covers the primary use case from issue #14262.
Optional: Consider edge case coverage
To increase confidence, consider adding test cases for:
- Special keys with modifiers:
'^class': 'foo','.style': {...}- Event handlers:
'^onClick': handler,.onClick: handler(to confirm they're properly handled/skipped)- Boolean attributes:
'^disabled': true(to verify correct rendering)Example:
test('props modifiers with special keys', async () => { const html = await render( h('input', { '^class': 'btn', '^disabled': true, '.prop': 'x' }) ) expect(html).toBe('<input class="btn" disabled>') })However, these tests can be deferred if the current coverage is deemed sufficient.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server-renderer/__tests__/render.spec.tspackages/server-renderer/src/helpers/ssrRenderAttrs.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server-renderer/src/helpers/ssrRenderAttrs.ts (1)
packages/shared/src/general.ts (1)
isOn(15-19)
🔇 Additional comments (3)
packages/server-renderer/src/helpers/ssrRenderAttrs.ts (3)
32-32: LGTM: Loop variable mutation enabled.Changing from
consttoletis necessary to allow the key reassignment on line 44 when stripping the^modifier.
36-40: LGTM: Property modifier handling.The logic correctly skips keys starting with
.since DOM properties cannot be set during SSR. The comment clearly explains the behavior.
43-44: LGTM: Attribute modifier handling.The
^prefix is correctly stripped to force the key to be rendered as an attribute. The comment matches the terminology used in the render function documentation.
3 participants
close #14262.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.