fix(ssr): filter out transition-specific props to avoid invalid HTML attributes #13894
Conversation
…nvalid HTML attributes - Add props filtering logic to exclude transition-specific and TransitionGroup-specific props - Prevents invalid HTML attributes like 'name' from being applied to DOM elements - Fixes vuejs#13037 where TransitionGroup with tag='ul' was generating invalid HTML The filtering ensures only valid HTML attributes are passed to the rendered element, resolving W3C validation errors when using TransitionGroup with specific tags.
WalkthroughReintroduces TransitionGroup prop filtering on the SSR side by updating the SSR compiler transform to mark transition contexts and pass an isTransition flag to ssrRenderAttrs, and by extending the server renderer's ssrRenderAttrs to skip transition-related props (kebab and camel forms) when isTransition is true. Tests updated/added accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Template as SFC Template
participant Compiler as compiler-ssr (ssrTransformTransitionGroup)
participant Renderer as server-renderer (ssrRenderAttrs)
participant Output as SSR Output
Template->>Compiler: compile <TransitionGroup> (props, tag)
Note over Compiler: Phase 1 - collect tag, filter attrs (kebab & camel), set isTransition
Compiler->>Compiler: store WIP (tag, propsExp)
Compiler->>Renderer: call _ssrRenderAttrs(_attrs, tagOrExpr, true)
Note over Renderer: if isTransition=true -> skip Transition props (camel+kebab) & moveClass
Renderer-->>Compiler: filtered attributes string
Compiler-->>Output: emit HTML fragment with filtered attributes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/runtime-dom/src/components/TransitionGroup.ts (2)
133-145: Prop sanitization is correct; considerhasOwnand add a quick guardrail.
- Use
hasOwninstead ofinto avoid prototype-chain hits and micro‑optimize.- Minor: precompute a blocked Set if this runs hot (optional).
Apply:
- import { extend } from '@vue/shared' + import { extend, hasOwn } from '@vue/shared' ... - if ( - !(key in TransitionPropsValidators) && + if ( + !hasOwn(TransitionPropsValidators, key) && key !== 'tag' && key !== 'moveClass' ) {Please verify that standard fallthrough attrs (class, style, id, aria-, data-, DOM events) still land on the root element across: tag='ul', tag='div', and default Fragment (should warn/ignore as before).
183-183: Avoid passing props when tag is Fragment to prevent dev warnings.Passing attrs to a Fragment yields extraneous-attrs warnings in dev. Skip props in that case:
- return createVNode(tag, filteredProps, children) + return createVNode(tag, tag === Fragment ? null : filteredProps, children)Confirm no new warnings are emitted for
<TransitionGroup name="x">(no tag) while attributes on tagged roots (e.g.,<TransitionGroup tag="ul" id="list">) still render correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-dom/src/components/TransitionGroup.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-dom/src/components/TransitionGroup.ts (1)
packages/runtime-dom/src/components/Transition.ts (1)
TransitionPropsValidators(64-68)
⏰ 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). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- 在 TransitionGroup 中引入 hasOwn 函数，替代原有的属性检查方式 - 确保仅有效的 HTML 属性被传递到渲染的元素中，进一步避免无效的 HTML 属性问题
….com/Husky-Yellow/core into fix/transition-group-props-filtering
There was a problem hiding this comment.
I don't think this change has fixed the problem.
Here's a Playground using this PR. The name attribute is still present in the generated HTML:
As far as I'm aware, the original issue only occurs when SSR is enabled. The extra props are already removed during client-side rendering.
Perhaps I'm missing something. Are you able to provide a Playground (using the Netlify preview of this PR), or even better include some test cases that demonstrate the fix is working correctly?
| } | ||
|
|
||
| return createVNode(tag, null, children) | ||
| return createVNode(tag, tag === Fragment ? null : filteredProps, children) |
There was a problem hiding this comment.
I'm confused. This seems to change the props from null to filteredProps. If the old value was null then it doesn't seem this is where the spurious props were being applied originally. I'm not sure how passing extra props here would help.
There was a problem hiding this comment.
So the fix isn't changing from null to filteredProps - it's changing
from passing all props (including invalid HTML attributes like
transition props) to passing only valid HTML attributes.
The condition tag === Fragment ? null : filteredProps means:
- If rendering a Fragment: pass null (no props needed)
- If rendering an actual HTML element: pass only the filtered, valid
HTML props
This prevents invalid HTML attributes like name="fade" or
duration="300" from appearing on the DOM element.
There was a problem hiding this comment.
But the original code is this:
return createVNode(tag, null, children)That isn't passing all props, it's passing null. The new code passes more props, not fewer.
I believe the changes to this file are incorrect and should be reverted.
There was a problem hiding this comment.
But the original code is this:
return createVNode(tag, null, children)That isn't passing all props, it's passing
null. The new code passes more props, not fewer.I believe the changes to this file are incorrect and should be reverted.
Thank you for your guidance, and this line of code has awakened me to the issue — I've identified a fundamental flaw in my previous approach to fixing the issue. The core problem is that Vue's automatic fallthrough mechanism fails to properly handle the declared props of TransitionGroup, causing properties that should be filtered by the component to erroneously appear in the final HTML. The runtime fallthrough mechanism malfunctions, resulting in transition-related attributes (such as name="fade") being incorrectly rendered into the HTML. The same issue occurs in SSR environments, generating HTML with invalid attributes. Therefore, my previous method of manually filtering attributes within each component was incorrect.
There is a critical flaw in how TransitionGroup handles props:
- A dynamic deletion operation
delete t.props.modeis executed in the decorate function - This breaks the fallthrough mechanism: Vue's setFullProps function relies on
hasOwn(options, camelKey)to determine which properties are declared props - The end result: The deleted mode and all other transition properties fail to be correctly identified as declared props, causing them to erroneously enter the attrs object
The correct architecture-level fix should be:
- Rebuild the props definition for TransitionGroup
- Use extend instead of object spreading (to meet ESBuild requirements)
- Exclude mode during the definition phase to avoid subsequent deletion operations
It's important to note that during SSR compilation, all attributes are directly compiled into the generated code. Since SSR is processed at compile time rather than runtime, the runtime fallthrough mechanism does not take effect during SSR compilation.
There was a problem hiding this comment.
But the original code is this:
return createVNode(tag, null, children)That isn't passing all props, it's passing . The new code passes more props, not fewer.
nullI believe the changes to this file are incorrect and should be reverted.
I double-checked and realized I had indeed misidentified the root cause of the issue. This led me to fix a file that didn’t need fixing, but I have now reverted that change. 😣
Sorry, this does affect SSR after all. I had a misunderstanding of the code, and I'll fix this part again. |
During my local testing, I noticed that the actual effect is inconsistent with the display in the Vue SFC Playground. Currently, I’m not quite clear on how to present local effects through the Playground. However, I have integrated the build artifacts from Playground into the project for server-side rendering, and no issues occurred during this process. What confuses me now is: since the same files are used, why are there discrepancies in the final build results? |
|
The SSR example you linked appears to be using a render function, so it won't match what happens in the Playground. For components that use templates, Vue compiles a separate SSR version that bypasses VNodes and just generates the HTML string. You can see that version in the Playground by clicking on the SSR tab. 
It would appear that this is where the spurious |
…ibutes - Add filtering for `name` prop in SSR transform logic - Prevents `name` from being rendered as DOM attribute (e.g. `<ul name="list">`) - Fixes vuejs#13037 (invalid HTML from TransitionGroup with custom `tag`) Ensures only valid HTML attributes are passed to the rendered element, resolving W3C validation issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (1)
35-35: Use English for inline comments to match repo conventions.Replace the Chinese comment with an English equivalent for consistency:
- // 在处理 TransitionGroup 的属性时，过滤掉 name/tag 等私有 props + // Filter out TransitionGroup-only props (e.g. name/tag) so they are not emitted as HTML attrs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.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). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (1)
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (1)
36-45: Exclude TransitionGroup-only props from SSR-emitted attrs (static + bound)File: packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts
Short: Filter all Transition/TransitionGroup props (static and v-bind with a static arg) so they are not emitted as HTML attributes in SSR.Apply this change (broadens the original
name-only exclusion and also dropsv-bindwith a static arg that matches an excluded key):- const otherProps = node.props.filter(p => { - // 排除 tag（已单独处理）和 name（私有 props，不该透传） - if ( - p === tag || - (p.type === NodeTypes.ATTRIBUTE && p.name === 'name') - ) { - return false - } - return true - }) + const otherProps = node.props.filter(p => { + if (p === tag) return false + // Exclude Transition/TransitionGroup-only props from SSR-emitted attrs + if (p.type === NodeTypes.ATTRIBUTE) { + return !TRANSITION_GROUP_SSR_EXCLUDE_PROPS.has(p.name) + } + // Exclude v-bind with a static arg that matches excluded keys + if ( + p.type === NodeTypes.DIRECTIVE && + p.name === 'bind' && + p.arg && + p.arg.type === NodeTypes.SIMPLE_EXPRESSION && + p.arg.isStatic && + TRANSITION_GROUP_SSR_EXCLUDE_PROPS.has(p.arg.content) + ) { + return false + } + return true + })Add this helper set near the top of the file (outside this hunk):
// Props that belong to Transition/TransitionGroup and must not be emitted as HTML attrs const TRANSITION_GROUP_SSR_EXCLUDE_PROPS = new Set<string>([ 'name', 'css', 'type', 'duration', 'appear', 'moveClass', 'move-class', 'enter-from-class', 'enter-active-class', 'enter-to-class', 'leave-from-class', 'leave-active-class', 'leave-to-class', 'appear-from-class', 'appear-active-class', 'appear-to-class', // 'tag' is handled separately via `findProp`/`p === tag` ])Note: object spreads (
v-bind="obj") with transition keys cannot be filtered statically; consider an SSR helper to strip these keys at render time if parity with runtime is required.Tests to add:
- Static props:
name,appear,css,duration,move-class, and representative*-classprops.- Bound with static arg:
:name,:move-class.- Spread:
v-bind="{ name: 'x', appear: true }"(document current behavior or add helper).
| const otherProps = node.props.filter(p => p !== tag) | ||
| // 在处理 TransitionGroup 的属性时，过滤掉 name/tag 等私有 props | ||
| const otherProps = node.props.filter(p => { | ||
| // 排除 tag（已单独处理）和 name（私有 props，不该透传） |
There was a problem hiding this comment.
sorry 这边发现还有其他属性没有处理
我需要晚上回家处理下。
There was a problem hiding this comment.
中文注释不太好，其他贡献者可能看不懂。
谢谢您的督促，已经调整好了。
edison1105
added
scope: transition
🍰 p2-nice-to-have
| // 排除 tag（已单独处理）和 name（私有 props，不该透传） | ||
| if ( | ||
| p === tag || | ||
| (p.type === NodeTypes.ATTRIBUTE && p.name === 'name') |
There was a problem hiding this comment.
This seems to only fix the problem for name, not for other props such as duration.
Also, checking for a type of ATTRIBUTE will only find static attributes. Attributes bound with v-bind, e.g. :name="n", will still be included.
There was a problem hiding this comment.
This seems to only fix the problem for
name, not for other props such asduration.Also, checking for a
typeofATTRIBUTEwill only find static attributes. Attributes bound withv-bind, e.g.:name="n", will still be included.
You're absolutely right. I only noticed this issue after fixing the previous one. I'll figure out how to address it—it seems like this isn't the right place to handle it, so I'll try another approach. Thank you for your oversight.
…tionGroup SSR transform This change improves the SSR transform for TransitionGroup by properly filtering out all transition-specific props that should not be passed through to the rendered element. The implementation: 1. Re-creates the transition props validators structure to mirror runtime logic 2. Filters out both static and dynamic transition-specific props 3. Handles both camelCase and kebab-case prop names 4. Excludes TransitionGroup-specific props like moveClass/move-class 5. Adds comprehensive test coverage for prop filtering This ensures that only relevant props are passed to the rendered element in SSR, matching the behavior of client-side rendering.
- Replace TransitionPropsValidators with direct props definition to exclude 'mode' - Combine BaseTransition props with DOM-specific transition props explicitly - Remove manual props filtering logic by properly defining allowed props - Clean up unnecessary hasOwn import since it's no longer used - Simplify vnode creation by removing filteredProps
edison1105
changed the title
| `) | ||
| }) | ||
|
|
||
| test('filters out transition event handlers', () => { |
There was a problem hiding this comment.
Event handlers are not included in the SSR output, so this test is redundant.
see ssr output in Playground
|
Also need to consider the usage of |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (2)
64-67: Use sharedcamelizeinstead of rolling your own kebab-to-camel.Reuse
camelizefrom@vue/sharedand remove the local helper.Apply these diffs:
- import { hasOwn } from '@vue/shared' + import { hasOwn, camelize } from '@vue/shared'-// Helper function to convert kebab-case to camelCase -function kebabToCamel(str: string): string { - return str.replace(/-([a-z])/g, (_, char) => char.toUpperCase()) -} +// Use shared camelize for kebab-case -> camelCase- const camelCaseName = kebabToCamel(propName) + const camelCaseName = camelize(propName)- const camelCaseArgName = kebabToCamel(argName) + const camelCaseArgName = camelize(argName)Also applies to: 12-12, 97-97, 112-112
20-62: Avoid duplicating runtime validators; centralize or reduce drift risk.Mirroring
TransitionPropsValidatorshere risks divergence. Prefer:
- Centralizing the keys list in a shared module consumed by both runtime and compiler-ssr, or
- Defining a local Set of keys (camelCase) plus
moveClass, and relying oncamelizechecks, with a prominent “keep-in-sync” comment referencing the runtime source.Non-blocking but recommended to prevent subtle mismatches in future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts(1 hunks)packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/compiler-ssr/tests/ssrTransitionGroup.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (2)
packages/runtime-dom/src/components/Transition.ts (1)
TransitionPropsValidators(64-68)packages/shared/src/general.ts (1)
hasOwn(34-37)
🔇 Additional comments (2)
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (2)
99-103: BlocklistingmoveClass/move-classexplicitly is good.Covers the TransitionGroup-only prop that isn’t in the validators object.
104-118: Static-argv-bindfiltering looks solid.Correctly removes transition-related keys for
:name="...",:duration="...", etc. Once runtime filtering is added for no-argv-bind, dynamic/computed keys will also be covered.
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts
Outdated
Show resolved
Hide resolved
| argName === 'moveClass' || | ||
| argName === 'move-class' | ||
| return !shouldFilter | ||
| } else if (!p.arg) { |
There was a problem hiding this comment.
This would be a breaking change, considering this scenario where the class was removed.
There was a problem hiding this comment.
This filtering should be done at ssr runtime, for example, by adding an extra parameter to _ssrRenderAttrs: _ssrRenderAttrs(attrs, tag, isTransition). If isTransition = true, filter out the transition-specific props.
There was a problem hiding this comment.
This filtering should be done at ssr runtime, for example, by adding an extra parameter to
_ssrRenderAttrs:_ssrRenderAttrs(attrs, tag, isTransition). IfisTransition = true, filter out the transition-specific props.
I've dealt with it.
…p SSR Replace compile-time removal of v-bind objects with runtime filtering to preserve valid attributes (id, data-*, etc.) while filtering transition-specific props. - Add SSR_FILTER_TRANSITION_PROPS runtime helper - Detect user-written v-bind objects (exclude compiler-generated _attrs) - Apply runtime filtering only when object v-bind is present - Preserve existing behavior for single prop bindings - Add comprehensive SSR tests for mixed object/prop scenarios
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts (1)
258-307: Consider adding a case fortaginside object v-bindTo guard against regressions, add a test ensuring
tagfromv-binddoesn’t appear as an HTML attribute when object v-bind is present. Note: SSR compile doesn’t infer wrapper tag from object v-bind’stag, so this only asserts attribute filtering (not dynamic tag selection).Proposed test to append near similar v-bind cases:
+ test('object v-bind: filters out tag prop (no leak into attrs)', () => { + expect( + compile( + `<transition-group tag="div" v-bind="{ tag: 'ul', id: 'ok', name: 'fade' }"></transition-group>` + ).code, + ).toMatchInlineSnapshot(` + "const { mergeProps: _mergeProps } = require("vue") + const { ssrRenderAttrs: _ssrRenderAttrs, ssrFilterTransitionProps: _ssrFilterTransitionProps } = require("vue/server-renderer") + + return function ssrRender(_ctx, _push, _parent, _attrs) { + _push(\`<div\${_ssrRenderAttrs(_ssrFilterTransitionProps(_mergeProps({ tag: 'ul', id: 'ok', name: 'fade' }, _attrs)))}></div>\`) + }" + `) + })Explanation:
- Uses static
tag="div"so SSR chooses a known wrapper.- Supplies
tagvia object v-bind to verify it’s filtered from attrs (should not rendertag="ul").- Keeps
nameto confirm transition props are filtered too.packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (2)
72-80: Unused state in WIP entry
sawObjectVBindis stored inWIPEntrybut not used in phase 2. Safe to drop from the entry to reduce footprint.interface WIPEntry { tag: AttributeNode | DirectiveNode propsExp: string | JSChildNode | null scopeId: string | null - sawObjectVBind: boolean }And remove the field where set in
wipMap.set(...).
23-71: Validator duplication risk (informational)Recreating
TransitionPropsValidatorshere mirrors runtime but can drift. If feasible later, consider a shared source of truth or a generated list to reduce maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts(1 hunks)packages/compiler-ssr/src/runtimeHelpers.ts(2 hunks)packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts(3 hunks)packages/server-renderer/src/helpers/ssrRenderAttrs.ts(1 hunks)packages/server-renderer/src/internal.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (5)
packages/runtime-dom/src/components/Transition.ts (1)
TransitionPropsValidators(64-68)packages/compiler-core/src/ast.ts (5)
ComponentNode(151-159)AttributeNode(186-191)DirectiveNode(193-211)JSChildNode(349-359)createCallExpression(728-739)packages/shared/src/general.ts (1)
hasOwn(34-37)packages/compiler-ssr/src/transforms/ssrTransformElement.ts (1)
buildSSRProps(359-387)packages/compiler-ssr/src/runtimeHelpers.ts (2)
SSR_RENDER_ATTRS(10-10)SSR_FILTER_TRANSITION_PROPS(30-32)
packages/server-renderer/src/helpers/ssrRenderAttrs.ts (2)
packages/shared/src/makeMap.ts (1)
makeMap(10-14)packages/server-renderer/src/internal.ts (1)
ssrFilterTransitionProps(12-12)
⏰ 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). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (5)
packages/compiler-ssr/src/runtimeHelpers.ts (1)
30-32: Helper symbol and mapping look correct
SSR_FILTER_TRANSITION_PROPSis declared and registered tossrFilterTransitionProps, matching the server-renderer export.Also applies to: 54-55
packages/server-renderer/src/internal.ts (1)
12-13: Publicly re-exportingssrFilterTransitionPropsis aligned with compiler usageThis ensures the compiler-emitted helper resolves via
vue/server-renderer. Good.packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts (1)
226-240: Redundant test: event handlers are already excluded by SSR pathSSR skips event listeners during props building, so this test doesn’t exercise the new filtering. Consider removing to reduce noise.
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (2)
92-141: Dynamic prop filtering looks solid; ensuretagvia object v-bind is handled at runtimeStatic and
v-bind:tagare excluded viap === tag. However,taginside object v-bind isn’t filtered here and must be handled by the runtime filter. Please pair with addingtagtotransitionPropsToFilter(server-renderer) as suggested.
151-159: Correct conditional wrapping withSSR_FILTER_TRANSITION_PROPSOnly wrapping when an object v-bind is present avoids unnecessary runtime work and preserves attrs in other cases. Good.
… SSR Add isTransition parameter to ssrRenderAttrs to filter transition-specific props at runtime, preserving valid attributes (id, data-*, etc.) while removing transition props (name, moveClass, appear, etc.). - Add isTransition parameter to ssrRenderAttrs function - Update TransitionGroup SSR transform to pass isTransition=true flag - Handle both cases: with component props and _attrs-only - Filter transition props in runtime for both camelCase and kebab-case - Add comprehensive tests for transition prop filtering - Update all existing TransitionGroup SSR test snapshots
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts (2)
258-272: Object v-bind with mixed props defers to runtime filteringConsider renaming the test to clarify that filtering happens at render time, not compile time.
- test('object v-bind with mixed valid and transition props', () => { + test('object v-bind passes mixed props; filtering happens at SSR render time', () => {
274-288: Object v-bind literal props: snapshot correctly shows unfiltered inputs flowing to _ssrRenderAttrs(..., true)Same minor naming suggestion for clarity (runtime filtering).
- test('object v-bind filters runtime computed transition props', () => { + test('object v-bind (runtime-computed): relies on SSR render-time filtering', () => {packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (2)
20-62: Validator duplication risk (drift vs runtime)Recreating TransitionPropsValidators here works but can drift from runtime. If feasible, centralize the prop-name list (not the validators) in a shared module to avoid divergence. If not, add a comment linking to runtime source and keep tests that would fail on drift.
85-123: Compile-time filtering logic looks correct; minor DRY improvementYou replicate filtering logic for both static attrs and static-arg v-bind. Consider extracting a helper to avoid divergence.
+// above: near kebabToCamel +function isTransitionKey(name: string): boolean { + const camel = kebabToCamel(name) + return ( + hasOwn(TransitionPropsValidators, name) || + hasOwn(TransitionPropsValidators, camel) || + name === 'moveClass' || + name === 'move-class' + ) +} @@ - const shouldFilter = - hasOwn(TransitionPropsValidators, propName) || - hasOwn(TransitionPropsValidators, camelCaseName) || - propName === 'moveClass' || - propName === 'move-class' - return !shouldFilter + return !isTransitionKey(propName) @@ - const shouldFilter = - hasOwn(TransitionPropsValidators, argName) || - hasOwn(TransitionPropsValidators, camelCaseArgName) || - argName === 'moveClass' || - argName === 'move-class' - return !shouldFilter + return !isTransitionKey(argName)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts(5 hunks)packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts(6 hunks)packages/server-renderer/__tests__/ssrRenderAttrs.spec.ts(1 hunks)packages/server-renderer/src/helpers/ssrRenderAttrs.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server-renderer/src/helpers/ssrRenderAttrs.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/server-renderer/__tests__/ssrRenderAttrs.spec.ts (1)
packages/server-renderer/src/helpers/ssrRenderAttrs.ts (1)
ssrRenderAttrs(27-55)
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (2)
packages/runtime-dom/src/components/Transition.ts (1)
TransitionPropsValidators(64-68)packages/shared/src/general.ts (1)
hasOwn(34-37)
🔇 Additional comments (20)
packages/server-renderer/__tests__/ssrRenderAttrs.spec.ts (3)
10-27: Good coverage for isTransition=true filteringCovers camelCase and kebab-case (enter-from-class). Output matches expectations.
29-45: Back-compat validated for isTransition=falseConfirms previous behavior remains when flag is false.
47-59: Default (flag omitted) behavior preservedNo regression when the new parameter is not provided.
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts (12)
32-33: Correctly passes tag and transition flag to _ssrRenderAttrs (static tag)Matches the new SSR path.
51-52: Static tag + v-if: correct transition-aware attrs callSnapshot looks right.
73-74: Static tag + comment: transition-aware attrs preservedAll good.
94-95: Dynamic tag: passes dynamic tag + isTransition=true to _ssrRenderAttrsInline snapshot aligns with transform changes.
146-148: Attribute fallthrough: mergeProps + transition-aware attrsGood to see fallthrough going through the filtering path.
151-170: Filters out transition props from explicit attributes (no fallthrough)Ensures SSR output stays clean while runtime handles fallthrough.
172-189: Comprehensive filtering of transition propsCovers most of the Transition props set.
191-205: Explicitly covers moveClass filteringGood to assert the special-case.
207-224: Dynamic transition props: compile defers to runtime filtering via flagSnapshot is correct.
226-240: Event handlers aren’t included in SSR attrs; this test is redundantSSR skips listeners already; keeping the test is harmless but unnecessary.
242-256: Covers empty/boolean-like values for transition propsGood edge-case coverage.
290-305: Mixed single-prop bindings and object v-bind path looks correctFlag is passed; merge order is preserved.
packages/compiler-ssr/src/transforms/ssrTransformTransitionGroup.ts (5)
64-67: kebabToCamel helper is fineCovers required conversions for filtering parity with runtime.
136-140: Passing tag + isTransition=true into _ssrRenderAttrs is the right hookEnables runtime filtering regardless of compile-time pruning.
165-170: Dynamic tag fallback handles _attrs with filteringCovers the no-props case; prevents leaking transition props from fallthrough.
204-207: Static tag fallback handles _attrs with filteringGood parity with the dynamic branch.
217-218: Heads-up: tag from no-arg v-bind is not detected by the transformIf users provide tag only via v-bind="obj", SSR will render a fragment (no wrapper), while runtime may render an element. This is pre-existing and out of scope, but worth documenting or warning in docs/tests.
… SSR Add isTransition parameter to ssrRenderAttrs to filter transition-specific props at runtime, preserving valid attributes (id, data-*, etc.) while removing transition props (name, moveClass, appear, etc.). - Add isTransition parameter to ssrRenderAttrs function - Update TransitionGroup SSR transform to pass isTransition=true flag - Handle both cases: with component props and _attrs-only - Filter transition props in runtime for both camelCase and kebab-case - Add comprehensive tests for transition prop filtering - Update all existing TransitionGroup SSR test snapshots
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts (2)
151-170: Rename test to reflect compile-time stripping of static transition propsThis test asserts compile-time filtering, not runtime fallthrough. Suggest clarifying the name/comment.
- test('transition props should NOT fallthrough (runtime should handle this)', () => { - // This test verifies that if runtime fallthrough is working correctly, - // SSR should still filter out transition props for clean HTML + test('filters out static transition props (SSR compile-time)', () => { + // Static transition props are stripped by the SSR compiler for clean HTML.
260-274: Consider dropping or making the prop shape explicitThis test doesn’t demonstrate mixed transition props unless transitionProps is known. The next test (literal object) already covers this path.
- test('object v-bind with mixed valid and transition props', () => { - expect( - compile( - `<transition-group tag="ul" v-bind="transitionProps" class="container"> - </transition-group>`, - ).code, - ).toMatchInlineSnapshot(` - "const { mergeProps: _mergeProps } = require("vue") - const { ssrRenderAttrs: _ssrRenderAttrs } = require("vue/server-renderer") - - return function ssrRender(_ctx, _push, _parent, _attrs) { - _push(\`<ul\${_ssrRenderAttrs(_mergeProps(_ctx.transitionProps, { class: "container" }, _attrs), "ul", true)}></ul>\`) - }" - `) - })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts(5 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). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (12)
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts (12)
32-32: Pass-through with isTransition=true for static tag looks correctSignature and args order (_attrs, "ul", true) are consistent with the new helper.
51-51: Consistent isTransition flag for static tag + v-if pathGood coverage for the v-if branch; args look right.
73-73: Consistent isTransition flag for static tag + comment pathArgs and import usage are correct.
94-95: Dynamic tag path correctly passes tag expr + isTransitionUsing _ctx.someTag and true is correct for filtering at render time.
146-146: Fallthrough merge order + isTransition param are correctProps merged before _attrs; helper called with ("ul", true). LGTM.
172-189: Good breadth: static filtering of many transition propsCovers kebab-case variants well; snapshot matches expected helper usage.
191-205: Explicit coverage for moveClassCorrect to exclude it; args to helper are consistent.
207-224: Dynamic transition props path is exercised correctlyDynamic props preserved in merge and delegated to _ssrRenderAttrs with isTransition=true.
226-242: Redundant: event listeners are already omitted by SSR compilerMatches prior feedback; this adds no transition-specific signal. Recommend removing to keep the suite lean.
- test('event handlers are omitted in SSR (not transition-specific)', () => { - // This test verifies that event handlers are filtered out during SSR compilation, - // not because of transition filtering but because SSR skips event listeners entirely - expect( - compile( - `<transition-group tag="div" @before-enter="onBeforeEnter" @enter="onEnter" @after-enter="onAfterEnter" @enter-cancelled="onEnterCancelled" @before-leave="onBeforeLeave" @leave="onLeave" @after-leave="onAfterLeave" @leave-cancelled="onLeaveCancelled" @before-appear="onBeforeAppear" @appear="onAppear" @after-appear="onAfterAppear" @appear-cancelled="onAppearCancelled" @click="onClick" class="events"> - </transition-group>`, - ).code, - ).toMatchInlineSnapshot(` - "const { mergeProps: _mergeProps } = require("vue") - const { ssrRenderAttrs: _ssrRenderAttrs } = require("vue/server-renderer") - - return function ssrRender(_ctx, _push, _parent, _attrs) { - _push(\`<div\${_ssrRenderAttrs(_mergeProps({ class: "events" }, _attrs), "div", true)}></div>\`) - }" - `) - })
244-258: Good: empty/boolean-ish values filteredCovers appear="", persisted="", css, type, duration. Looks solid.
276-290: Good: exercises runtime filtering via object literalKeeping name/moveClass inside the merged object is correct; filtering happens in _ssrRenderAttrs with isTransition=true.
303-307: Mixed single-prop and object v-bind path looks correctMerge order and isTransition flag are right.
Summary:
Changes:
Filtered props before passing to createVNode, excluding transition props and tag/moveClass while retaining valid HTML attributes.
Fixes: #13037, W3C validation errors. No breaking changes.
Summary by CodeRabbit
Bug Fixes
Tests