Fix RouterView instance registration under HMR (Options API beforeRouteUpdate)
#2545
Conversation
…Update work after HMR (vuejs#2500)
WalkthroughAdds HMR-aware instance registration in RouterView by injecting an onVnodeMounted hook when running in HMR runtime, and adds a Vitest spec that simulates blocked post-flush watchers to verify that beforeRouteUpdate still fires after an HMR-like remount. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant RouterView
participant RouteRecord
participant Router
participant Comp
App->>RouterView: render RouterView (depth/name)
note over RouterView: detect HMR runtime (dev && import.meta.hot)
RouterView->>Comp: mount vnode with onVnodeMounted
Comp-->>RouterView: vnode mounted (onVnodeMounted)
RouterView->>RouteRecord: instances[name] = vnode.component.proxy
App->>Router: router.push(/temp/2)
Router-->>Comp: call beforeRouteUpdate(to, from)
note right of Comp: beforeRouteUpdate runs after instance re-registration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
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
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for vue-router canceled.
|
![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
🧹 Nitpick comments (5)
packages/router/src/RouterView.ts (2)
168-172: Minor: make HMR detection a bit more resilient and compute once.Two small tweaks improve robustness and avoid repeated evaluation on each render:
- Use optional chaining on
import.metaaccess to be future-proof in non-Vite environments.- Hoist the
isHMRRuntimecomputation once insetup()(or even module scope) instead of per render.Apply:
- const isHMRRuntime = - (__DEV__ || __FEATURE_PROD_DEVTOOLS__) && - isBrowser && - (import.meta as any).hot + // compute once in setup (above return) and reuse here: + const isHMRRuntime = + (__DEV__ || __FEATURE_PROD_DEVTOOLS__) && + isBrowser && + ((import.meta as any)?.hot)
188-197: Preserve/compose any user-provided onVnodeMounted handler.If users pass
onVnodeMountedviaattrsor route-props, the current code replaces it in HMR mode. Compose both to avoid breaking user hooks.- const component = h( - ViewComponent, - assign( - {}, - routeProps, - attrs, - onVnodeMounted ? { onVnodeMounted } : null, - { - onVnodeUnmounted, - ref: viewRef, - } - ) - ) + // Compose user and internal vnode-mounted hooks if both exist + const userOnVnodeMounted = + (attrs as any)?.onVnodeMounted as VNodeProps['onVnodeMounted'] | undefined + const mergedOnVnodeMounted = + onVnodeMounted && userOnVnodeMounted + ? ((v: VNode) => { + userOnVnodeMounted!(v) + onVnodeMounted!(v) + }) + : onVnodeMounted ?? userOnVnodeMounted + + const component = h( + ViewComponent, + assign( + {}, + routeProps, + attrs, + mergedOnVnodeMounted ? { onVnodeMounted: mergedOnVnodeMounted } : null, + { + onVnodeUnmounted, + ref: viewRef, + } + ) + )If you’d like, I can scan the repo for any existing
onVnodeMountedusage against<router-view>to validate that this change won’t alter current behavior.packages/router/__tests__/RouterView.hmr.spec.ts (3)
32-36: Isolate and restore globals used to emulate HMR.Mutating
globalThis.__DEV__andimport.meta.hotis fine for this spec, but it should be restored to avoid cross-test bleed. Wrap in try/finally and restore previous values.Example:
const prevDEV = (globalThis as any).__DEV__ const prevHot = (import.meta as any).hot try { ;(globalThis as any).__DEV__ = true ;(import.meta as any).hot = {} // ... test body ... } finally { ;(globalThis as any).__DEV__ = prevDEV ;(import.meta as any).hot = prevHot }I can push a follow-up that wraps the whole test body into a
try/finallyand handles cleanup automatically.
59-62: Avoid DOM/resource leaks: keep the wrapper and unmount in finally.Capture the wrapper and unmount it in a
finallyblock so this spec leaves no retained DOM nodes or Vue instances.- mount(App, { global: { plugins: [router] }, attachTo: document.body }) + const wrapper = mount(App, { global: { plugins: [router] }, attachTo: document.body }) await router.push('/temp/1') await router.isReady() @@ - vi.doUnmock('vue') + vi.doUnmock('vue') + wrapper.unmount()Also applies to: 117-122
10-11: Simplify: hoisting isn’t necessary here.Because the mock factory is declared within the same test and closes over local variables,
vi.hoistedis not required. A plain locallet blockedPostWatchCalls = 0is sufficient and reduces cognitive overhead.If you keep
vi.hoisted, consider moving it to module top-level (recommended usage) so it’s guaranteed to initialize before any mocks are evaluated.If you want, I can provide two minimal patch variants: (1) convert to a local counter, or (2) move
vi.hoistedto the top-level of the spec module.Also applies to: 13-25
📜 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/router/__tests__/RouterView.hmr.spec.ts(1 hunks)packages/router/src/RouterView.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/router/__tests__/RouterView.hmr.spec.ts (1)
packages/router/src/RouterView.ts (1)
RouterView(244-261)
packages/router/src/RouterView.ts (2)
packages/router/src/utils/env.ts (1)
isBrowser(1-1)packages/router/src/utils/index.ts (1)
assign(38-38)
🔇 Additional comments (2)
packages/router/src/RouterView.ts (1)
173-183: Good, targeted fix: re-register instance on mount in HMR.Registering
matchedRoute.instances[currentName] = vnode.component.proxyinsideonVnodeMountedand guarding it with the currentmatchedRoute/depthis the right way to ensure Options API update guards are collected after HMR re-mounts, even if the post-flush watcher is blocked. This addresses the root cause without affecting prod.packages/router/__tests__/RouterView.hmr.spec.ts (1)
98-116: Nice assertions — they validate the intended sequencing.
- Verifying pre-registration after
nextTick()and zero post-writes after navigation accurately models the blocked post-flush path.- The
beforeRouteUpdatespy confirms the fix works end-to-end.No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/router/__tests__/RouterView.hmr.spec.ts (3)
33-37: Snapshot and restore HMR globals to avoid test pollution.The test mutates DEV and import.meta.hot but doesn’t restore them. This can leak into other tests. Snapshot, then restore at the end (ideally in a finally block).
Apply this diff to capture and later restore:
-;(globalThis as any).__DEV__ = true -;(import.meta as any).hot = {} +const prevDev = (globalThis as any).__DEV__ +const prevHot = (import.meta as any).hot +;(globalThis as any).__DEV__ = true +;(import.meta as any).hot = {}And near the end of the test (see also cleanup changes below):
+ // Restore globals modified for HMR + ;(globalThis as any).__DEV__ = prevDev + if (prevHot === undefined) delete (import.meta as any).hot + else (import.meta as any).hot = prevHot
60-63: Unmount the mounted wrapper to prevent DOM leaks across tests.attachTo: document.body without unmount can leak nodes and side effects to subsequent tests.
- mount(App, { global: { plugins: [router] }, attachTo: document.body }) + const wrapper = mount(App, { + global: { plugins: [router] }, + attachTo: document.body, + })Then near the end of the test, add:
+ // Ensure DOM cleanup + wrapper.unmount()
119-122: Restore rec.instances.default via descriptor without redundant assignment.Defining the original descriptor and then assigning stored can throw if the original descriptor is non-writable. Prefer restoring the descriptor with the intended value in one step; fall back to defining a fresh, writable data property if none existed.
- if (desc) Object.defineProperty(rec.instances, 'default', desc) - else delete (rec.instances as any).default - ;(rec.instances as any).default = stored - vi.doUnmock('vue') + if (desc) { + // If it was a data descriptor, restore it with the stored value + if ('value' in desc || 'writable' in desc) { + Object.defineProperty(rec.instances, 'default', { + ...desc, + value: stored, + }) + } else { + // Accessor descriptor: restore as-is, then set via setter if available + Object.defineProperty(rec.instances, 'default', desc) + try { + ;(rec.instances as any).default = stored + } catch {} + } + } else { + Object.defineProperty(rec.instances, 'default', { + configurable: true, + enumerable: true, + writable: true, + value: stored, + }) + } + vi.doUnmock('vue')
📜 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 (1)
packages/router/__tests__/RouterView.hmr.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/router/__tests__/RouterView.hmr.spec.ts (1)
packages/router/src/RouterView.ts (1)
RouterView(244-261)
🔇 Additional comments (2)
packages/router/__tests__/RouterView.hmr.spec.ts (2)
14-26: Mocking watch(post) correctly and returning a proper WatchStopHandle — nice.Good use of vi.importActual to delegate non-post watchers. Returning a no-op function fixes the earlier issue flagged in past reviews and matches Vue’s WatchStopHandle contract.
98-116: Assertions convincingly validate the HMR path — LGTM.The preWrites/postWrites split, blocked post-flush watch counting, and beforeRouteUpdate assertion together give strong coverage for the regression. No changes requested.
Fix Options API
beforeRouteUpdateafter HMR (#2500)Summary
This PR fixes #2500.
When using the Options API with HMR enabled, the
beforeRouteUpdatehook was not called after a hot reload.The issue occurred because the route component instance was not properly re-registered after re-mount under HMR.
Fix
beforeRouteUpdateis correctly triggered after hot reloads.Impact
beforeRouteUpdatefire as expected after HMR updates.Before/After
Before: After HMR, beforeRouteUpdate could be skipped due to missing instance at guard extraction.
After: Instance is registered on mount in HMR, so Options API update guards are reliably collected.
Related
Summary by CodeRabbit