-
-
Notifications
You must be signed in to change notification settings - Fork 9k
fix(runtime-core): pass component instance to flushPreFlushCbs on unmount #14221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(runtime-core): pass component instance to flushPreFlushCbs on unmount #14221
Conversation
WalkthroughPasses the previous vnode's component instance into pre-flush callbacks and adds a test that unmounting a dynamically mounted app does not trigger watchers in another app. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/runtime-core/src/renderer.ts (1)
2391-2397: Fix correctly prevents cross-app watcher triggering, but simplify the expression.The fix is correct: by passing the previous component instance to
flushPreFlushCbs, only callbacks associated with that specific component are flushed, preventing watchers in other apps from being inadvertently triggered during unmount.However, the nested ternary expression on lines 2395-2397 is overly verbose.
🔎 Simplify with optional chaining:
const prev = container._vnode container._vnode = vnode if (!isFlushing) { isFlushing = true - flushPreFlushCbs( - prev ? (prev.component ? prev.component : undefined) : undefined, - ) + flushPreFlushCbs(prev?.component) flushPostFlushCbs() isFlushing = false }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime-core/__tests__/apiCreateApp.spec.ts(1 hunks)packages/runtime-core/src/renderer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/runtime-core/src/renderer.ts (1)
packages/runtime-core/src/scheduler.ts (1)
flushPreFlushCbs(142-171)
packages/runtime-core/__tests__/apiCreateApp.spec.ts (1)
packages/runtime-core/src/apiCreateApp.ts (1)
App(33-119)
🔇 Additional comments (1)
packages/runtime-core/__tests__/apiCreateApp.spec.ts (1)
650-686: Well-structured test that properly validates the fix.The test correctly verifies that unmounting one app doesn't trigger watchers in another independent app. The setup with separate apps and the spy function effectively demonstrates the bug is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/runtime-core/src/renderer.ts (1)
2376-2380: Instance capture logic is correct for standard Vue app usage; consider documenting the component vnode assumption.The code captures
instance = container._vnode.componentto scope pre-flush callbacks during unmount, preventing watchers from other apps from executing. This works correctly for standard Vue applications where the root vnode is always a component. While accessing.componenton a non-component vnode (edge case) would result inundefined, this is unlikely in practice and the fix still achieves its primary goal for typical usage.Consider adding a comment for maintainability:
if (vnode == null) { if (container._vnode) { unmount(container._vnode, null, null, true) + // Capture component instance to scope pre-flush callbacks to this + // component only, preventing watchers in other apps from triggering instance = container._vnode.component }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-core/src/renderer.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-core/src/renderer.ts (1)
packages/runtime-core/src/scheduler.ts (1)
flushPreFlushCbs(142-171)
🔇 Additional comments (1)
packages/runtime-core/src/renderer.ts (1)
2396-2396: Well-targeted fix for scoping pre-flush callbacks.Passing the captured
instancetoflushPreFlushCbs()correctly scopes the pre-flush callbacks to only the component being unmounted. According to the relevant code snippet, when an instance is provided,flushPreFlushCbsfilters callbacks bycb.id === instance.uid, which prevents watchers from unrelated apps/components from being triggered.The behavior is appropriately differentiated:
- Unmount path:
instanceis defined → only matching callbacks are flushed (the fix)- Mount/patch path:
instanceis undefined → all callbacks are flushed (existing behavior)This targeted approach addresses issue #14215 without affecting other code paths.
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
close #14215
Summary by CodeRabbit
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.