Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions packages/reactivity/src/watch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export interface WatchOptions<Immediate = boolean> extends DebuggerOptions {
immediate?: Immediate
deep?: boolean | number
once?: boolean
equals?: (a: any, b: any) => boolean
scheduler?: WatchScheduler
onWarn?: (msg: string, ...args: any[]) => void
/**
Expand Down Expand Up @@ -240,14 +241,31 @@ export function watch(
if (cb) {
// watch(source, cb)
const newValue = effect.run()
if (
deep ||
forceTrigger ||
(isMultiSource
? (newValue as any[]).some((v, i) => hasChanged(v, oldValue[i]))
: hasChanged(newValue, oldValue))
) {
// cleanup before running cb again
const areEqual = (a: any, b: any): boolean => {
if (options.equals) {
try {
return !!options.equals(a, b)
} catch (e) {
if (call) {
call(() => {
throw e
}, WatchErrorCodes.WATCH_CALLBACK)
}
return false
}
}
return !hasChanged(a, b)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const isChanged = isMultiSource
? (newValue as any[]).some((v, i) => !areEqual(v, oldValue[i]))
: !areEqual(newValue, oldValue)
// If equals is provided, it fully controls the trigger decision,
// bypassing deep and forceTrigger logic.
const shouldTrigger = options.equals
? isChanged
: deep || forceTrigger || isChanged
Comment on lines +261 to +268
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add error handling around user-provided equals function calls.

If the user-provided equals function throws an exception (e.g., due to accessing properties on null/undefined, or internal logic errors), the entire watcher job will fail, potentially breaking reactive updates and leaving the component in an inconsistent state.

Wrap the equals invocations in try-catch blocks and handle errors gracefully, similar to how other user callbacks are handled via the call helper throughout this codebase.

🔎 Example approach for error handling

Consider wrapping equals calls:

 const isChanged = isMultiSource
   ? (newValue as any[]).some((v, i) =>
       options.equals
-        ? !options.equals(v, oldValue[i])
+        ? !safeEquals(options.equals, v, oldValue[i])
         : hasChanged(v, oldValue[i]),
     )
   : options.equals
-    ? !options.equals(newValue, oldValue)
+    ? !safeEquals(options.equals, newValue, oldValue)
     : hasChanged(newValue, oldValue)

Where safeEquals is a helper that wraps the call in try-catch and uses the call helper for error handling, or falls back to hasChanged on error.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In packages/reactivity/src/watch.ts around lines 244 to 257, user-supplied
equals calls are invoked directly and can throw, which would crash the watcher;
wrap every equals invocation (both in the multi-source some(...) comparator and
the single-source branch) in a small try-catch wrapper (or create a safeEquals
helper) that calls the equals using the existing call helper for consistent
error handling, and on exception fall back to using hasChanged (or treat as
changed) so the watcher continues to function; ensure the computed isChanged and
shouldTrigger use the safe result and preserve existing deep/forceTrigger
behavior.


if (shouldTrigger) {
if (cleanup) {
cleanup()
}
Expand Down
67 changes: 67 additions & 0 deletions packages/runtime-core/__tests__/apiWatch.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2061,4 +2061,71 @@ describe('api: watch', () => {
createApp(App).mount(root)
expect(onCleanup).toBeCalledTimes(0)
})
it('watch: should trigger cleanup even if manually skipped in callback (current behavior)', async () => {
const state = ref({ id: 1 })
let cleanupCount = 0
let effectCount = 0

watch(
state,
(newVal, oldVal, onCleanup) => {
// Even if we skip the logic here, the scheduler has already fired the cleanup
if (JSON.stringify(newVal) === JSON.stringify(oldVal)) {
return
}

effectCount++
onCleanup(() => {
cleanupCount++
})
},
{ deep: true },
)

state.value = { id: 2 }
await nextTick()
expect(effectCount).toBe(1)
expect(cleanupCount).toBe(0)

// Trigger with semantically identical value
state.value = { id: 2 }
await nextTick()

// Assert current behavior: cleanup fires even if we return early in the callback
expect(cleanupCount).toBe(1)
expect(effectCount).toBe(1)
})

it('watch: should respect custom equality check via "equals" option', async () => {
const state = ref({ id: 1 })
let cleanupCount = 0
let callbackCount = 0

watch(
state,
(newVal, oldVal, onCleanup) => {
callbackCount++
onCleanup(() => {
cleanupCount++
})
},
{
deep: true,
equals: (a, b) => JSON.stringify(a) === JSON.stringify(b),
},
)

state.value = { id: 2 }
await nextTick()
expect(callbackCount).toBe(1)
expect(cleanupCount).toBe(0)

// Trigger with semantically identical value
state.value = { id: 2 }
await nextTick()

// With the fix, these should remain unchanged
expect(callbackCount).toBe(1)
expect(cleanupCount).toBe(0)
})
})
6 changes: 6 additions & 0 deletions packages/runtime-core/src/apiWatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ export interface WatchOptions<Immediate = boolean> extends WatchEffectOptions {
immediate?: Immediate
deep?: boolean | number
once?: boolean
/**
* Custom equality function to determine if the value has changed.
* If provided, it takes precedence over the default equality check,
* `deep` watches, and `forceTrigger`.
*/
equals?: (a: any, b: any) => boolean
}

// Simple effect.
Expand Down