fix(compiler-core): prevent root comments from blocking static node hoisting #13345
Conversation
…ild with certain structure (fix vuejs#13344)
WalkthroughThe changes update the logic determining whether a root node has a single element child by introducing a new function that ignores comment nodes. This ensures static prop hoisting works correctly even when comments are present. A corresponding test case is added to verify this behavior, and related internal function usage is updated. Changes
Sequence Diagram(s)sequenceDiagram
participant RootNode
participant CacheStaticTransform
participant getSingleElementRoot
participant Codegen
RootNode->>CacheStaticTransform: Initiate static prop hoisting
CacheStaticTransform->>getSingleElementRoot: Check for single element root (ignoring comments)
getSingleElementRoot-->>CacheStaticTransform: Return single element node or null
CacheStaticTransform->>Codegen: Hoist props if single element node found
Codegen-->>RootNode: Generate code with hoisted props (comments preserved)
Assessment against linked issues
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/compiler-core/__tests__/transforms/cacheStatic.spec.ts (1)
546-571: Good test case for verifying the comment node exclusion behavior.This test verifies that static props hoisting works correctly when the root has both a comment node and a single element. It properly tests that:
- A single cache entry is created
- The root element's static attributes are hoisted
- The comment node is preserved in the output children
- The single element is correctly handled despite the comment node's presence
Consider removing the
console.logstatement on line 569 since it was likely added for debugging purposes and isn't necessary for the test.- console.log(generate(root).code)packages/compiler-core/src/transform.ts (1)
359-374: Logic improved to handle comment nodes in single element roots.The implementation now correctly retrieves the single element child while excluding comment nodes, which fixes the issue described in #13344 where fallthrough attributes were not being properly applied when comments were present in the root node.
Consider using optional chaining as suggested by the static analysis tool:
- if (singleElementRootChild && singleElementRootChild.codegenNode) { + if (singleElementRootChild?.codegenNode) {This would reduce code verbosity while maintaining the same functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 361-361: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
packages/compiler-core/__tests__/transforms/__snapshots__/cacheStatic.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
packages/compiler-core/__tests__/transforms/cacheStatic.spec.ts(1 hunks)packages/compiler-core/src/transform.ts(3 hunks)packages/compiler-core/src/transforms/cacheStatic.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/compiler-core/src/transform.ts (1)
packages/compiler-core/src/transforms/cacheStatic.ts (1)
getSingleElementRoot(48-57)
packages/compiler-core/src/transforms/cacheStatic.ts (2)
packages/compiler-core/src/ast.ts (4)
RootNode(104-121)PlainElementNode(140-149)ComponentNode(151-159)TemplateNode(170-174)packages/compiler-core/src/utils.ts (1)
isSlotOutlet(358-362)
🪛 Biome (1.9.4)
packages/compiler-core/src/transform.ts
[error] 361-361: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (2)
packages/compiler-core/src/transform.ts (1)
40-40: Updated import aligns with the implementation change.The import now correctly references the new
getSingleElementRootfunction instead of the oldisSingleElementRootfunction.packages/compiler-core/src/transforms/cacheStatic.ts (1)
44-44: Updated usage of the new function with proper boolean conversion.The double negation (
!!) properly converts the return value ofgetSingleElementRoot(which can be an element node ornull) to a boolean value, maintaining the expected behavior while using the new implementation.
| export function getSingleElementRoot( | ||
| root: RootNode, | ||
| child: TemplateChildNode, | ||
| ): child is PlainElementNode | ComponentNode | TemplateNode { | ||
| const { children } = root | ||
| return ( | ||
| children.length === 1 && | ||
| child.type === NodeTypes.ELEMENT && | ||
| !isSlotOutlet(child) | ||
| ) | ||
| ): PlainElementNode | ComponentNode | TemplateNode | null { | ||
| const children = root.children.filter(x => x.type !== NodeTypes.COMMENT) | ||
| return children.length === 1 && | ||
| children[0].type === NodeTypes.ELEMENT && | ||
| !isSlotOutlet(children[0]) | ||
| ? children[0] | ||
| : null | ||
| } |
There was a problem hiding this comment.
Key implementation change to filter out comment nodes.
This is the core change that fixes issue #13344. The new getSingleElementRoot function:
- Filters out comment nodes from the children array
- Checks if there's exactly one element child that's not a slot outlet
- Returns the actual element node or null (instead of a boolean)
This ensures that static prop hoisting works correctly even when comment nodes are present, which addresses the fallthrough attributes issue in the PR description.
While thoroughly filtering out all comment nodes is correct for this specific use case (hoisting static props), ensure that this doesn't break any other functionality that might rely on comments being processed. The test case appropriately verifies that the comment node is still present in the final output.
The implementation is clean and well-focused on the specific issue being fixed.
🤖 Prompt for AI Agents
In packages/compiler-core/src/transforms/cacheStatic.ts around lines 48 to 57,
update the getSingleElementRoot function to filter out comment nodes from the
root's children before checking if there is exactly one element child that is
not a slot outlet. Return the element node if conditions are met, otherwise
return null. This change ensures static prop hoisting works correctly when
comment nodes are present without affecting other functionality that relies on
comments.
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-ssr
@vue/compiler-sfc
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
Fixes #13344
Filter out the comment children for the hoisting logic of the root template node, resulting in the non-comment child receiving the hoisted properties.
8000Summary by CodeRabbit
Tests
Refactor