fix(watch): post flush watchers should not fire when component is unmounted

fix #1603

packages/runtime-core/__tests__/apiWatch.spec.ts

@@ -260,12 +260,13 @@ describe('api: watch', () => {
   it('flush timing: post (default)', async () => {
     const count = ref(0)
     let callCount = 0
+    let result
     const assertion = jest.fn(count => {
       callCount++
       // on mount, the watcher callback should be called before DOM render
       // on update, should be called after the count is updated
       const expectedDOM = callCount === 1 ? `` : `${count}`
-      expect(serializeInner(root)).toBe(expectedDOM)
+      result = serializeInner(root) === expectedDOM
     })
 
     const Comp = {
@@ -279,10 +280,12 @@ describe('api: watch', () => {
     const root = nodeOps.createElement('div')
     render(h(Comp), root)
     expect(assertion).toHaveBeenCalledTimes(1)
+    expect(result).toBe(true)
 
     count.value++
     await nextTick()
     expect(assertion).toHaveBeenCalledTimes(2)
+    expect(result).toBe(true)
   })
 
   it('flush timing: pre', async () => {
@@ -290,16 +293,18 @@ describe('api: watch', () => {
     const count2 = ref(0)
 
     let callCount = 0
+    let result1
+    let result2
     const assertion = jest.fn((count, count2Value) => {
       callCount++
       // on mount, the watcher callback should be called before DOM render
       // on update, should be called before the count is updated
       const expectedDOM = callCount === 1 ? `` : `${count - 1}`
-      expect(serializeInner(root)).toBe(expectedDOM)
+      result1 = serializeInner(root) === expectedDOM
 
       // in a pre-flush callback, all state should have been updated
-      const expectedState = callCount === 1 ? 0 : 1
-      expect(count2Value).toBe(expectedState)
+      const expectedState = callCount - 1
+      result2 = count === expectedState && count2Value === expectedState
     })
 
     const Comp = {
@@ -318,12 +323,16 @@ describe('api: watch', () => {
     const root = nodeOps.createElement('div')
     render(h(Comp), root)
     expect(assertion).toHaveBeenCalledTimes(1)
+    expect(result1).toBe(true)
+    expect(result2).toBe(true)
 
     count.value++
     count2.value++
     await nextTick()
     // two mutations should result in 1 callback execution
     expect(assertion).toHaveBeenCalledTimes(2)
+    expect(result1).toBe(true)
+    expect(result2).toBe(true)
   })
 
   it('flush timing: sync', async () => {
@@ -331,17 +340,19 @@ describe('api: watch', () => {
     const count2 = ref(0)
 
     let callCount = 0
+    let result1
+    let result2
     const assertion = jest.fn(count => {
       callCount++
       // on mount, the watcher callback should be called before DOM render
       // on update, should be called before the count is updated
       const expectedDOM = callCount === 1 ? `` : `${count - 1}`
-      expect(serializeInner(root)).toBe(expectedDOM)
+      result1 = serializeInner(root) === expectedDOM
 
       // in a sync callback, state mutation on the next line should not have
       // executed yet on the 2nd call, but will be on the 3rd call.
       const expectedState = callCount < 3 ? 0 : 1
-      expect(count2.value).toBe(expectedState)
+      result2 = count2.value === expectedState
     })
 
     const Comp = {
@@ -360,11 +371,57 @@ describe('api: watch', () => {
     const root = nodeOps.createElement('div')
     render(h(Comp), root)
     expect(assertion).toHaveBeenCalledTimes(1)
+    expect(result1).toBe(true)
+    expect(result2).toBe(true)
 
     count.value++
     count2.value++
     await nextTick()
     expect(assertion).toHaveBeenCalledTimes(3)
+    expect(result1).toBe(true)
+    expect(result2).toBe(true)
+  })
+
+  it('should not fire on component unmount w/ flush: post', async () => {
+    const toggle = ref(true)
+    const cb = jest.fn()
+    const Comp = {
+      setup() {
+        watch(toggle, cb)
+      },
+      render() {}
+    }
+    const App = {
+      render() {
+        return toggle.value ? h(Comp) : null
+      }
+    }
+    render(h(App), nodeOps.createElement('div'))
+    expect(cb).not.toHaveBeenCalled()
+    toggle.value = false
+    await nextTick()
+    expect(cb).not.toHaveBeenCalled()
+  })
+
+  it('should fire on component unmount w/ flush: pre', async () => {
+    const toggle = ref(true)
+    const cb = jest.fn()
+    const Comp = {
+      setup() {
+        watch(toggle, cb, { flush: 'pre' })
+      },
+      render() {}
+    }
+    const App = {
+      render() {
+        return toggle.value ? h(Comp) : null
+      }
+    }
+    render(h(App), nodeOps.createElement('div'))
+    expect(cb).not.toHaveBeenCalled()
+    toggle.value = false
+    await nextTick()
+    expect(cb).toHaveBeenCalledTimes(1)
   })
 
   it('deep', async () => {

packages/runtime-core/__tests__/components/Suspense.spec.ts

@@ -170,7 +170,7 @@ describe('Suspense', () => {
         })
 
         const count = ref(0)
-        watch(count, v => {
+        watch(count, () => {
           calls.push('watch callback')
         })
         count.value++ // trigger the watcher now
@@ -367,7 +367,7 @@ describe('Suspense', () => {
     await nextTick()
     expect(serializeInner(root)).toBe(`<!---->`)
     // should discard effects (except for immediate ones)
-    expect(calls).toEqual(['immediate effect', 'watch callback', 'unmounted'])
+    expect(calls).toEqual(['immediate effect', 'unmounted'])
   })
 
   test('unmount suspense after resolve', async () => {

packages/runtime-core/src/apiWatch.ts

@@ -234,33 +234,39 @@ function doWatch(
   }
 
   let oldValue = isArray(source) ? [] : INITIAL_WATCHER_VALUE
-  const applyCb = cb
-    ? () => {
-        if (instance && instance.isUnmounted) {
-          return
-        }
-        const newValue = runner()
-        if (deep || hasChanged(newValue, oldValue)) {
-          // cleanup before running cb again
-          if (cleanup) {
-            cleanup()
-          }
-          callWithAsyncErrorHandling(cb, instance, ErrorCodes.WATCH_CALLBACK, [
-            newValue,
-            // pass undefined as the old value when it's changed for the first time
-            oldValue === INITIAL_WATCHER_VALUE ? undefined : oldValue,
-            onInvalidate
-          ])
-          oldValue = newValue
+  const job = () => {
+    if (!runner.active) {
+      return
+    }
+    if (cb) {
+      // watch(source, cb)
+      const newValue = runner()
+      if (deep || hasChanged(newValue, oldValue)) {
+        // cleanup before running cb again
+        if (cleanup) {
+          cleanup()
         }
+        callWithAsyncErrorHandling(cb, instance, ErrorCodes.WATCH_CALLBACK, [
+          newValue,
+          // pass undefined as the old value when it's changed for the first time
+          oldValue === INITIAL_WATCHER_VALUE ? undefined : oldValue,
+          onInvalidate
+        ])
+        oldValue = newValue
       }
-    : void 0
+    } else {
+      // watchEffect
+      runner()
+    }
+  }
 
   let scheduler: (job: () => any) => void
   if (flush === 'sync') {
     scheduler = invoke
   } else if (flush === 'pre') {
-    scheduler = job => {
+    // ensure it's queued before component updates (which have positive ids)
+    job.id = -1
+    scheduler = () => {
       if (!instance || instance.isMounted) {
         queueJob(job)
       } else {
@@ -270,22 +276,22 @@ function doWatch(
       }
     }
   } else {
-    scheduler = job => queuePostRenderEffect(job, instance && instance.suspense)
+    scheduler = () => queuePostRenderEffect(job, instance && instance.suspense)
   }
 
   const runner = effect(getter, {
     lazy: true,
     onTrack,
     onTrigger,
-    scheduler: applyCb ? () => scheduler(applyCb) : scheduler
+    scheduler
   })
 
   recordInstanceBoundEffect(runner)
 
   // initial run
-  if (applyCb) {
+  if (cb) {
     if (immediate) {
-      applyCb()
+      job()
     } else {
       oldValue = runner()
     }

packages/runtime-core/src/renderer.ts

@@ -2025,19 +2025,17 @@ function baseCreateRenderer(
     if (bum) {
       invokeArrayFns(bum)
     }
+    if (effects) {
+      for (let i = 0; i < effects.length; i++) {
+        stop(effects[i])
+      }
+    }
     // update may be null if a component is unmounted before its async
     // setup has resolved.
     if (update) {
       stop(update)
       unmount(subTree, instance, parentSuspense, doRemove)
     }
-    if (effects) {
-      queuePostRenderEffect(() => {
-        for (let i = 0; i < effects.length; i++) {
-          stop(effects[i])
-        }
-      }, parentSuspense)
-    }
     // unmounted hook
     if (um) {
       queuePostRenderEffect(um, parentSuspense)