fix(hmr): handle cached text node update #14134
Open
+84
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughCached text vnode handling was changed: during DEV/HMR updates, cached text children store a per-node index and are replaced with newly created text elements instead of reusing cached DOM nodes. A test was added to verify cached text updates across HMR rerenders. 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)
Runtime->>Runtime: create new text node (createText)
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.
Comment on lines
+503
to
+523
| // 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.
Critical: n2.el is not updated to reference the new text node.
After creating and inserting newChild (line 516-519), then removing oldChild (line 520), n2.el still points to the old element that was set on line 501. This means n2.el references a detached DOM node after the replacement.
Apply this diff to update n2.el to reference the new text node:
const newChild = hostCreateText(n2.children as string)
const oldChild =
childNodes[((n2 as any).__elIndex = (n1 as any).__elIndex)]
+ n2.el = newChild
hostInsert(newChild, container, oldChild)
hostRemove(oldChild)This follows the pattern used in patchStaticNode (lines 574-584), which properly updates n2.el and n2.anchor after replacing static content during HMR.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 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) | |
| } | |
| // 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)] | |
| n2.el = newChild | |
| hostInsert(newChild, container, oldChild) | |
| hostRemove(oldChild) | |
| } else { | |
| hostSetText(el, n2.children as string) | |
| } |
🤖 Prompt for AI Agents
In packages/runtime-core/src/renderer.ts around lines 503 to 523, after creating
and inserting newChild and removing oldChild during HMR replace logic, n2.el is
still pointing at the detached old element; update n2 to reference the newly
inserted node (assign n2.el = newChild) and also update n2.anchor if the node
uses an anchor (follow the same assignment pattern as patchStaticNode lines
~574-584) so the vnode now points to the live DOM node.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
close #14127
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.