fix(reactivity): ensure computed always expose value

fix #3099

Also changes the original fix for #910 by moving the fix from
reactivity to the scheduler

packages/reactivity/__tests__/computed.spec.ts

@@ -193,4 +193,10 @@ describe('reactivity/computed', () => {
     expect(isReadonly(z)).toBe(false)
     expect(isReadonly(z.value.a)).toBe(false)
   })
+
+  it('should expose value when stopped', () => {
+    const x = computed(() => 1)
+    stop(x.effect)
+    expect(x.value).toBe(1)
+  })
 })

packages/reactivity/__tests__/effect.spec.ts

@@ -709,28 +709,6 @@ describe('reactivity/effect', () => {
     expect(dummy).toBe(3)
   })
 
-  it('stop with scheduler', () => {
-    let dummy
-    const obj = reactive({ prop: 1 })
-    const queue: (() => void)[] = []
-    const runner = effect(
-      () => {
-        dummy = obj.prop
-      },
-      {
-        scheduler: e => queue.push(e)
-      }
-    )
-    obj.prop = 2
-    expect(dummy).toBe(1)
-    expect(queue.length).toBe(1)
-    stop(runner)
-
-    // a scheduled effect should not execute anymore after stopped
-    queue.forEach(e => e())
-    expect(dummy).toBe(1)
-  })
-
   it('events: onStop', () => {
     const onStop = jest.fn()
     const runner = effect(() => {}, {

packages/reactivity/src/effect.ts

@@ -26,6 +26,21 @@ export interface ReactiveEffectOptions {
   onTrack?: (event: DebuggerEvent) => void
   onTrigger?: (event: DebuggerEvent) => void
   onStop?: () => void
+  /**
+   * Indicates whether the job is allowed to recursively trigger itself when
+   * managed by the scheduler.
+   *
+   * By default, a job cannot trigger itself because some built-in method calls,
+   * e.g. Array.prototype.push actually performs reads as well (#1740) which
+   * can lead to confusing infinite loops.
+   * The allowed cases are component update functions and watch callbacks.
+   * Component update functions may update child component props, which in turn
+   * trigger flush: "pre" watch callbacks that mutates state that the parent
+   * relies on (#1801). Watch callbacks doesn't track its dependencies so if it
+   * triggers itself again, it's likely intentional and it is the user's
+   * responsibility to perform recursive state mutation that eventually
+   * stabilizes (#1727).
+   */
   allowRecurse?: boolean
 }
 
@@ -84,7 +99,7 @@ function createReactiveEffect<T = any>(
 ): ReactiveEffect<T> {
   const effect = function reactiveEffect(): unknown {
     if (!effect.active) {
-      return options.scheduler ? undefined : fn()
+      return fn()
     }
     if (!effectStack.includes(effect)) {
       cleanup(effect)

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

@@ -1,3 +1,4 @@
+import { effect, stop } from '@vue/reactivity'
 import {
   queueJob,
   nextTick,
@@ -568,4 +569,27 @@ describe('scheduler', () => {
     await nextTick()
     expect(count).toBe(1)
   })
+
+  // #910
+  test('should not run stopped reactive effects', async () => {
+    const spy = jest.fn()
+
+    // simulate parent component that toggles child
+    const job1 = () => {
+      stop(job2)
+    }
+    job1.id = 0 // need the id to ensure job1 is sorted before job2
+
+    // simulate child that's triggered by the same reactive change that
+    // triggers its toggle
+    const job2 = effect(() => spy())
+    expect(spy).toHaveBeenCalledTimes(1)
+
+    queueJob(job1)
+    queueJob(job2)
+    await nextTick()
+
+    // should not be called again
+    expect(spy).toHaveBeenCalledTimes(1)
+  })
 })

packages/runtime-core/src/scheduler.ts

@@ -3,27 +3,14 @@ import { isArray } from '@vue/shared'
 import { ComponentPublicInstance } from './componentPublicInstance'
 import { ComponentInternalInstance, getComponentName } from './component'
 import { warn } from './warning'
+import { ReactiveEffect } from '@vue/reactivity'
 
-export interface SchedulerJob {
-  (): void
+export interface SchedulerJob extends Function, Partial<ReactiveEffect> {
   /**
-   * unique job id, only present on raw effects, e.g. component render effect
+   * Attached by renderer.ts when setting up a component's render effect
+   * Used to obtain component information when reporting max recursive updates.
+   * dev only.
    */
-  id?: number
-  /**
-   * Indicates whether the job is allowed to recursively trigger itself.
-   * By default, a job cannot trigger itself because some built-in method calls,
-   * e.g. Array.prototype.push actually performs reads as well (#1740) which
-   * can lead to confusing infinite loops.
-   * The allowed cases are component update functions and watch callbacks.
-   * Component update functions may update child component props, which in turn
-   * trigger flush: "pre" watch callbacks that mutates state that the parent
-   * relies on (#1801). Watch callbacks doesn't track its dependencies so if it
-   * triggers itself again, it's likely intentional and it is the user's
-   * responsibility to perform recursive state mutation that eventually
-   * stabilizes (#1727).
-   */
-  allowRecurse?: boolean
   ownerInstance?: ComponentInternalInstance
 }
 
@@ -243,7 +230,7 @@ function flushJobs(seen?: CountMap) {
   try {
     for (flushIndex = 0; flushIndex < queue.length; flushIndex++) {
       const job = queue[flushIndex]
-      if (job) {
+      if (job && job.active !== false) {
         if (__DEV__ && checkRecursiveUpdates(seen!, job)) {
           continue
         }