fix: ensure tracking is paused when emit() calls handler so it can safely be called in effects (fix #6669) #6688
Conversation
…r so it can safely be called in effects fix: #6669
LinusBorg
force-pushed
the
linusborg/pause-tracking-in-emit-6669
branch
from
09f17b4 to
1c02e5a
Compare
|
I feel this PR is necessary though ##6669 is closed. |
|
@yyx990803 You closed #6669 as "not a bug", so I assume my PR can be closed? |
|
Just in case this does get merged, I think the same change would need to be applied to the As for whether it should be merged, I'm very much on the fence. I think it's an easy problem for people to fall into without really understanding what's going on. The exact timing of operations in Vue is not generally well understood by users. Various things are sensitive to timing and it usually 'just works' without needing to think about it. I don't think it's necessarily obvious that Even if someone isn't thinking about it in quite those terms, I still think events are perceived as somehow different from normal function calls, like throwing a parcel over a wall and not having to worry about what's on the other side. But, I can also see Evan's perspective for closing the original issue. Update: A few days after I posted this, we had another conversation about the synchronicity of |
edison1105
added
the
🔨 p3-minor-bug
WalkthroughThe changes introduce reactivity tracking control in the Vue emit function by pausing and resetting tracking around event handler invocations. A new test verifies that emitting events inside a reactive effect does not cause infinite loops or repeated triggers due to unintended dependency tracking during event listener execution. Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant EmitFunction
participant EventHandler
participant Reactivity
Component->>EmitFunction: emit('event', ...)
EmitFunction->>Reactivity: pauseTracking()
EmitFunction->>EventHandler: callWithAsyncErrorHandling()
EventHandler-->>EmitFunction: (event handled)
EmitFunction->>Reactivity: resetTracking()
EmitFunction-->>Component: (done)
Assessment against linked issues
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
packages/runtime-core/__tests__/componentEmits.spec.ts(2 hunks)packages/runtime-core/src/componentEmits.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-core/__tests__/componentEmits.spec.ts (1)
packages/runtime-core/src/componentEmits.ts (1)
emit(73-195)
🔇 Additional comments (3)
packages/runtime-core/src/componentEmits.ts (1)
30-30: Appropriate import for tracking control.The added import brings in reactivity tracking control methods, which will be used to pause dependency tracking during event handler execution.
packages/runtime-core/__tests__/componentEmits.spec.ts (2)
10-12: Added necessary imports for reactive testing.These imports are required for the new test case that verifies the reactivity tracking behavior.
437-467: Well-designed test for the reactivity tracking fix.This test effectively verifies that event handlers aren't being tracked as dependencies of reactive effects. By using a
watchEffectthat emits an event, and an event handler that modifies reactive state and triggers multiple ticks, it confirms that the counter only increments once instead of causing an infinite loop.The test design aligns perfectly with the fix implemented in
componentEmits.tsby proving that reactive tracking is properly paused during handler execution.
| pauseTracking() | ||
| callWithAsyncErrorHandling( | ||
| handler, | ||
| instance, | ||
| ErrorCodes.COMPONENT_EVENT_HANDLER, | ||
| args | ||
| ) | ||
| resetTracking() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good fix: Pausing reactivity tracking during event handler execution.
This change prevents reactive dependencies from being tracked while executing event handlers called through emit(). This fixes issue #6669 where emitting events inside an effect could cause infinite loops when reactive state was accessed inside the event handlers.
However, the same fix should be applied to the onceHandler code block (lines 175-189) to maintain consistency.
if (onceHandler) {
if (!instance.emitted) {
instance.emitted = {} as Record<any, boolean>
} else if (instance.emitted[handlerName]) {
return
}
instance.emitted[handlerName] = true
+ pauseTracking()
callWithAsyncErrorHandling(
onceHandler,
instance,
ErrorCodes.COMPONENT_EVENT_HANDLER,
args
)
+ resetTracking()
}🤖 Prompt for AI Agents
In packages/runtime-core/src/componentEmits.ts between lines 175 and 189, apply
the same pauseTracking and resetTracking calls around the onceHandler invocation
as done for the regular handler at lines 165-172. This ensures reactive
dependencies are not tracked during the execution of onceHandler event
callbacks, preventing infinite loops and maintaining consistency in event
handling.
fix: #13338
fix: #6669
If emit() is called in an effect (i.e. the callback of a watchEffect), it will collect any reactive values accessed in that parent handle as dependencies of the current component.
This can lead to a recursive loop crashing the app.
Summary by CodeRabbit
Bug Fixes
Tests