fix(hmr): handle cached text node update #14134
Conversation
WalkthroughDuring DEV/HMR updates, cached Text vnodes are given a per-node index and, when updated, are replaced by newly created text nodes instead of reusing the cached DOM node; traversal logic accounts for Fragment offsets. A test verifying cached text HMR updates was added. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Dev (HMR)
participant Runtime as Runtime Core
participant DOM as Host DOM
Note over Dev,Runtime: Normal runtime update (non-cached or no HMR)
Dev->>Runtime: trigger update
Runtime->>DOM: hostSetText(existingEl, newText)
Note over Dev,Runtime: DEV/HMR update for cached Text vnode
Dev->>Runtime: HMR rerender with PatchFlags.CACHED Text vnode
Runtime->>Runtime: detect cached text & HMR active
Runtime->>Runtime: compute per-node index (__elIndex) (i + FragmentOffset)
Runtime->>Runtime: create new text node
Runtime->>DOM: replace old element with new text node
Runtime->>DOM: continue patching dynamic children
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-core/src/renderer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-core/src/renderer.ts (1)
packages/runtime-core/src/hmr.ts (1)
isHmrUpdating(15-15)
🔇 Additional comments (1)
packages/runtime-core/src/renderer.ts (1)
501-501: LGTM: const to let change enables reassignment.The change from
consttoletis necessary to allow reassignment ofelin the HMR conditional branch below.
edison1105
force-pushed
the
edison/fix/14127
branch
from
a223e68 to
7e08418
Compare
edison1105
force-pushed
the
edison/fix/14127
branch
from
97861cd to
6c11c4d
Compare
edison1105
changed the title
edison1105
added
scope: hmr
🔨 p3-minor-bug
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime-core/__tests__/hmr.spec.ts(1 hunks)packages/runtime-core/src/renderer.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-core/src/renderer.ts (3)
packages/runtime-core/src/hmr.ts (1)
isHmrUpdating(15-15)packages/runtime-core/src/vnode.ts (2)
Text(69-69)Fragment(63-68)packages/runtime-core/src/index.ts (2)
Text(113-113)Fragment(113-113)
🔇 Additional comments (2)
packages/runtime-core/__tests__/hmr.spec.ts (1)
1044-1095: LGTM! Comprehensive test coverage for cached text node HMR updates.The test effectively validates the fix for issue #14127 by:
- Verifying initial render and state updates work correctly
- Performing two consecutive HMR updates to ensure static text changes are reflected
- Confirming dynamic state (count) is preserved across HMR updates
The test structure follows existing patterns and thoroughly exercises the interaction between HMR and cached text nodes.
packages/runtime-core/src/renderer.ts (1)
2519-2530: Well-designed approach to prevent detached DOM node references.The logic correctly addresses the issue:
- Non-cached text nodes inherit
elfrom the previous node (preserving existing behavior)- Cached text nodes store
__elIndexinstead to avoid retaining references to detached DOM nodes- The Fragment offset (
+1) correctly accounts for the fragment start anchor at index 0This index-based approach elegantly avoids stale DOM references while enabling HMR to locate and replace cached text nodes by their position in the parent's child list.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-core/src/renderer.ts(2 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). (1)
- GitHub Check: Agent
🔇 Additional comments (1)
packages/runtime-core/src/renderer.ts (1)
2519-2530: Logic for cached text node index storage looks correct.The implementation properly distinguishes between cached and non-cached text nodes:
- Non-cached text nodes inherit
elfrom the previous vnode (line 2522)- Cached text nodes store
__elIndexfor HMR replacement (lines 2525-2528)The Fragment offset calculation (
n1.type === Fragment ? 1 : 0) correctly accounts for the fragment's start anchor node, ensuring the index maps to the correct DOM child position when the parent is a Fragment.
| // We don't inherit el for cached text nodes in `traverseStaticChildren` | ||
| // to avoid retaining detached DOM nodes. However, the text node may be | ||
| // changed during HMR. In this case we need to replace the old text node | ||
| // with the new one. | ||
| if ( | ||
| __DEV__ && | ||
| isHmrUpdating && | ||
| n2.patchFlag === PatchFlags.CACHED && | ||
| '__elIndex' in n1 | ||
| ) { | ||
| const childNodes = __TEST__ | ||
| ? container.children | ||
| : container.childNodes | ||
| const newChild = hostCreateText(n2.children as string) | ||
| const oldChild = | ||
| childNodes[((n2 as any).__elIndex = (n1 as any).__elIndex)] | ||
| hostInsert(newChild, container, oldChild) | ||
| hostRemove(oldChild) | ||
| } else { | ||
| hostSetText(el, n2.children as string) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the target code in renderer.ts around lines 503-523
head -n 530 packages/runtime-core/src/renderer.ts | tail -n 40Repository: vuejs/core
Length of output: 1304
🏁 Script executed:
#!/bin/bash
# Search for traverseStaticChildren function and __TEST__ usage with container.children
rg -n "traverseStaticChildren|container\.children" packages/runtime-core/src/renderer.ts -A 5 -B 2Repository: vuejs/core
Length of output: 2469
🏁 Script executed:
#!/bin/bash
# Search for processText function to see full context
rg -n "const processText" packages/runtime-core/src/renderer.ts -A 30Repository: vuejs/core
Length of output: 1333
Critical: n2.el is not updated to reference the new text node.
After creating newChild (line 516) and removing oldChild (line 520), n2.el still points to the old, now-detached DOM node assigned on line 501. This means the vnode holds a stale reference to a removed element, which breaks subsequent updates and operations on this text node. Add n2.el = newChild after creating the new child.
Additionally, the __TEST__ path on line 514 uses container.children, which only returns element nodes, not text nodes. Since this branch handles text nodes, use container.childNodes instead to match the production behavior.
🤖 Prompt for AI Agents
In packages/runtime-core/src/renderer.ts around lines 503 to 523, the HMR path
replaces the old text node but fails to update the vnode reference and uses
container.children in the test branch; after creating newChild (line ~516) set
n2.el = newChild so the vnode points to the new DOM node, and change the
__TEST__ branch to use container.childNodes (not container.children) so text
nodes are found in tests the same as production; then proceed to insert and
remove oldChild as existing code does.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where cached static text nodes were not being correctly updated during Hot Module Replacement (HMR). The fix introduces a mechanism to track the DOM index of cached text nodes and replace them during HMR updates, rather than attempting to update them in place (which fails because their el reference is intentionally not inherited to avoid retaining detached DOM nodes).
Key Changes
- Introduced
__elIndexproperty on cached text VNodes to track their position in the parent's child nodes - Added special handling in
processTextto replace (rather than update) cached text nodes during HMR - Added comprehensive test coverage for cached text node updates across multiple HMR cycles
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/runtime-core/src/renderer.ts |
Modified processText to handle cached text node replacement during HMR, and updated traverseStaticChildren to cache the DOM index for cached text nodes |
packages/runtime-core/__tests__/hmr.spec.ts |
Added test case verifying cached text nodes update correctly during HMR while preserving dynamic content and handling consecutive HMR updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
close #14127
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.