fix(hmr): prevent updating unmounting component during HMR rerender #13775
Conversation
WalkthroughAdds an HMR rerender guard to skip updates for disposed component jobs and introduces a test verifying nested component rerenders propagate through forwarded slots during HMR. Changes
Sequence Diagram(s)sequenceDiagram
participant DevServer as HMR Runtime
participant Comp as Component Instance
participant Sched as Scheduler
DevServer->>Comp: rerender(id, newRender?)
Comp->>Sched: check job flags
alt Job not DISPOSED
Sched-->>Comp: allowed to update
Comp->>Comp: instance.update()
else Job DISPOSED
Sched-->>Comp: skip update
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
edison1105
changed the title
edison1105
changed the title
![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 (4)
packages/runtime-core/src/hmr.ts (2)
99-103: Nit: prefer explicit, null-safe flag handling for readability.Relying on JS bitwise coercion of
undefinedto0works but is implicit. Reading flags via?.and?? 0makes the intent clearer and safer ifjoborflagsare absent.Apply this diff for clarity:
- // #13771 don't update if the job is already disposed - if (!(instance.job.flags! & SchedulerJobFlags.DISPOSED)) { - instance.update() - } + // #13771 don't update if the job is already disposed + const flags = instance.job?.flags ?? 0 + if ((flags & SchedulerJobFlags.DISPOSED) === 0) { + instance.update() + }
98-104: Optional: ensureisHmrUpdatingis always reset withtry/finally.If an exception occurs during
instance.update(),isHmrUpdatingmay remaintrueuntil the wrapper logs and returns, which could have side effects on subsequent updates. Usingtry/finallylocalizes the guarantee.Apply this diff:
- // this flag forces child components with slot content to update - isHmrUpdating = true - // #13771 don't update if the job is already disposed - const flags = instance.job?.flags ?? 0 - if ((flags & SchedulerJobFlags.DISPOSED) === 0) { - instance.update() - } - isHmrUpdating = false + // this flag forces child components with slot content to update + isHmrUpdating = true + try { + // #13771 don't update if the job is already disposed + const flags = instance.job?.flags ?? 0 + if ((flags & SchedulerJobFlags.DISPOSED) === 0) { + instance.update() + } + } finally { + isHmrUpdating = false + }packages/runtime-core/__tests__/hmr.spec.ts (2)
931-931: Fix record registration id for App to match its__hmrId.
Appdeclares__hmrId = appIdbut is registered underparentId. While not breaking this test, it’s inconsistent and can confuse HMR bookkeeping.Apply this diff:
- createRecord(parentId, App) + createRecord(appId, App)
898-939: Consider adding a regression test for nested Teleport + conditional + HMR.The root cause in #13771 involved a nested Teleport with a conditional (v-if) where HMR triggered an update as the component was unmounting, leading to DOM anchor lookup failures. The disposed-job guard should prevent that. A focused test will lock the fix in.
You can add a test like this (positions near the Teleport test block):
test('rerender does not update disposed job in nested Teleport with conditional', async () => { const root = nodeOps.createElement('div') const target = nodeOps.createElement('div') const childId = 'teleport-nested-conditional-child' const Child: ComponentOptions = { __hmrId: childId, render: () => h('div', 'child') } createRecord(childId, Child) const parentId = 'teleport-nested-conditional-parent' const Parent: ComponentOptions = { __hmrId: parentId, data() { return { show: true, target } }, components: { Child }, render: compileToFunction(` <div> <teleport :to="target"> <Child v-if="show" /> </teleport> </div> `) } createRecord(parentId, Parent) render(h(Parent), root) expect(serializeInner(root)).toBe(`<div><!--teleport start--><!--teleport end--></div>`) expect(serializeInner(target)).toBe(`<div>child</div>`) // start unmount // toggle off, then trigger HMR on Child in the same flush window ;(root.children[0] as TestElement) // no-op to keep types happy // mutate state by re-rendering Parent rerender( parentId, compileToFunction(` <div> <teleport :to="target"> <!-- v-if false --> </teleport> </div> `) ) // HMR update on child while it's being unmounted rerender(childId, () => h('div', 'updated')) // expect no crash and teleported content empty expect(serializeInner(root)).toBe(`<div><!--teleport start--><!--teleport end--></div>`) expect(serializeInner(target)).toBe(``) })If you’d like, I can adapt this to match the test harness’ preferred patterns (e.g., using nextTick/triggerEvent as needed).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/runtime-core/__tests__/hmr.spec.ts(1 hunks)packages/runtime-core/src/hmr.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-core/__tests__/hmr.spec.ts (2)
packages/runtime-core/src/component.ts (1)
ComponentOptions(276-276)packages/runtime-test/src/serialize.ts (1)
serializeInner(22-33)
⏰ 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 (3)
packages/runtime-core/src/hmr.ts (2)
10-10: ImportingSchedulerJobFlagsis appropriate for the disposed-job guard.The added import from
./scheduleris correct and scoped to the new check.
99-103: Good fix: skip HMR updates for disposed jobs to avoid crashes during unmount.Conditionally calling
instance.update()based onSchedulerJobFlags.DISPOSEDaddresses the race where an unmounting instance was being updated, fixing #13771.packages/runtime-core/__tests__/hmr.spec.ts (1)
898-939: Test adds valuable coverage for HMR rerender propagation through forwarded slots.This verifies that rerendering the child propagates correctly through a parent that forwards its slot, which is a realistic nesting scenario.
edison1105
added
ready to merge
@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: |
Size ReportBundles
Usages
|
close #13771
close #13772
Summary by CodeRabbit
New Features
Bug Fixes
Tests