refactor(runtime-core/scheduler): dedicated preFlush queue

properly fix #1763, #1777, #1781

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

@@ -2,7 +2,9 @@ import {
   queueJob,
   nextTick,
   queuePostFlushCb,
-  invalidateJob
+  invalidateJob,
+  queuePreFlushCb,
+  flushPreFlushCbs
 } from '../src/scheduler'
 
 describe('scheduler', () => {
@@ -75,6 +77,128 @@ describe('scheduler', () => {
     })
   })
 
+  describe('queuePreFlushCb', () => {
+    it('basic usage', async () => {
+      const calls: string[] = []
+      const cb1 = () => {
+        calls.push('cb1')
+      }
+      const cb2 = () => {
+        calls.push('cb2')
+      }
+
+      queuePreFlushCb(cb1)
+      queuePreFlushCb(cb2)
+
+      expect(calls).toEqual([])
+      await nextTick()
+      expect(calls).toEqual(['cb1', 'cb2'])
+    })
+
+    it('should dedupe queued preFlushCb', async () => {
+      const calls: string[] = []
+      const cb1 = () => {
+        calls.push('cb1')
+      }
+      const cb2 = () => {
+        calls.push('cb2')
+      }
+      const cb3 = () => {
+        calls.push('cb3')
+      }
+
+      queuePreFlushCb(cb1)
+      queuePreFlushCb(cb2)
+      queuePreFlushCb(cb1)
+      queuePreFlushCb(cb2)
+      queuePreFlushCb(cb3)
+
+      expect(calls).toEqual([])
+      await nextTick()
+      expect(calls).toEqual(['cb1', 'cb2', 'cb3'])
+    })
+
+    it('chained queuePreFlushCb', async () => {
+      const calls: string[] = []
+      const cb1 = () => {
+        calls.push('cb1')
+        // cb2 will be executed after cb1 at the same tick
+        queuePreFlushCb(cb2)
+      }
+      const cb2 = () => {
+        calls.push('cb2')
+      }
+      queuePreFlushCb(cb1)
+
+      await nextTick()
+      expect(calls).toEqual(['cb1', 'cb2'])
+    })
+  })
+
+  describe('queueJob w/ queuePreFlushCb', () => {
+    it('queueJob inside preFlushCb', async () => {
+      const calls: string[] = []
+      const job1 = () => {
+        calls.push('job1')
+      }
+      const cb1 = () => {
+        // queueJob in postFlushCb
+        calls.push('cb1')
+        queueJob(job1)
+      }
+
+      queuePreFlushCb(cb1)
+      await nextTick()
+      expect(calls).toEqual(['cb1', 'job1'])
+    })
+
+    it('queueJob & preFlushCb inside preFlushCb', async () => {
+      const calls: string[] = []
+      const job1 = () => {
+        calls.push('job1')
+      }
+      const cb1 = () => {
+        calls.push('cb1')
+        queueJob(job1)
+        // cb2 should execute before the job
+        queuePreFlushCb(cb2)
+      }
+      const cb2 = () => {
+        calls.push('cb2')
+      }
+
+      queuePreFlushCb(cb1)
+      await nextTick()
+      expect(calls).toEqual(['cb1', 'cb2', 'job1'])
+    })
+
+    it('preFlushCb inside queueJob', async () => {
+      const calls: string[] = []
+      const job1 = () => {
+        // the only case where a pre-flush cb can be queued inside a job is
+        // when updating the props of a child component. This is handled
+        // directly inside `updateComponentPreRender` to avoid non atomic
+        // cb triggers (#1763)
+        queuePreFlushCb(cb1)
+        queuePreFlushCb(cb2)
+        flushPreFlushCbs(undefined, job1)
+        calls.push('job1')
+      }
+      const cb1 = () => {
+        calls.push('cb1')
+        // a cb triggers its parent job, which should be skipped
+        queueJob(job1)
+      }
+      const cb2 = () => {
+        calls.push('cb2')
+      }
+
+      queueJob(job1)
+      await nextTick()
+      expect(calls).toEqual(['cb1', 'cb2', 'job1'])
+    })
+  })
+
   describe('queuePostFlushCb', () => {
     it('basic usage', async () => {
       const calls: string[] = []

packages/runtime-core/src/apiWatch.ts

@@ -7,7 +7,7 @@ import {
   ReactiveEffectOptions,
   isReactive
 } from '@vue/reactivity'
-import { queueJob, SchedulerJob } from './scheduler'
+import { SchedulerJob, queuePreFlushCb } from './scheduler'
 import {
   EMPTY_OBJ,
   isObject,
@@ -271,7 +271,7 @@ function doWatch(
     job.id = -1
     scheduler = () => {
       if (!instance || instance.isMounted) {
-        queueJob(job)
+        queuePreFlushCb(job)
       } else {
         // with 'pre' option, the first call must happen before
         // the component is mounted so it is called synchronously.

packages/runtime-core/src/renderer.ts

@@ -41,7 +41,7 @@ import {
   queuePostFlushCb,
   flushPostFlushCbs,
   invalidateJob,
-  runPreflushJobs
+  flushPreFlushCbs
 } from './scheduler'
 import { effect, stop, ReactiveEffectOptions, isRef } from '@vue/reactivity'
 import { updateProps } from './componentProps'
@@ -1430,7 +1430,10 @@ function baseCreateRenderer(
     instance.next = null
     updateProps(instance, nextVNode.props, prevProps, optimized)
     updateSlots(instance, nextVNode.children)
-    runPreflushJobs(instance.update)
+
+    // props update may have triggered pre-flush watchers.
+    // flush them before the render update.
+    flushPreFlushCbs(undefined, instance.update)
   }
 
   const patchChildren: PatchChildrenFn = (

packages/runtime-core/src/scheduler.ts

@@ -16,17 +16,23 @@ export interface SchedulerJob {
   cb?: boolean
 }
 
+let isFlushing = false
+let isFlushPending = false
+
 const queue: (SchedulerJob | null)[] = []
-const postFlushCbs: Function[] = []
+let flushIndex = 0
+
+const pendingPreFlushCbs: Function[] = []
+let activePreFlushCbs: Function[] | null = null
+let preFlushIndex = 0
+
+const pendingPostFlushCbs: Function[] = []
+let activePostFlushCbs: Function[] | null = null
+let postFlushIndex = 0
+
 const resolvedPromise: Promise<any> = Promise.resolve()
 let currentFlushPromise: Promise<void> | null = null
 
-let isFlushing = false
-let isFlushPending = false
-let flushIndex = 0
-let pendingPostFlushCbs: Function[] | null = null
-let pendingPostFlushIndex = 0
-let hasPendingPreFlushJobs = false
 let currentPreFlushParentJob: SchedulerJob | null = null
 
 const RECURSION_LIMIT = 100
@@ -53,11 +59,17 @@ export function queueJob(job: SchedulerJob) {
     job !== currentPreFlushParentJob
   ) {
     queue.push(job)
-    if ((job.id as number) < 0) hasPendingPreFlushJobs = true
     queueFlush()
   }
 }
 
+function queueFlush() {
+  if (!isFlushing && !isFlushPending) {
+    isFlushPending = true
+    currentFlushPromise = resolvedPromise.then(flushJobs)
+  }
+}
+
 export function invalidateJob(job: SchedulerJob) {
   const i = queue.indexOf(job)
   if (i > -1) {
@@ -65,78 +77,84 @@ export function invalidateJob(job: SchedulerJob) {
   }
 }
 
-/**
- * Run flush: 'pre' watcher callbacks. This is only called in
- * `updateComponentPreRender` to cover the case where pre-flush watchers are
- * triggered by the change of a component's props. This means the scheduler is
- * already flushing and we are already inside the component's update effect,
- * right when the render function is about to be called. So if the watcher
- * triggers the same component to update, we don't want it to be queued (this
- * is checked via `currentPreFlushParentJob`).
- */
-export function runPreflushJobs(parentJob: SchedulerJob) {
-  if (hasPendingPreFlushJobs) {
-    currentPreFlushParentJob = parentJob
-    hasPendingPreFlushJobs = false
-    for (let job, i = flushIndex + 1; i < queue.length; i++) {
-      job = queue[i]
-      if (job && (job.id as number) < 0) {
-        job()
-        queue[i] = null
-      }
-    }
-    currentPreFlushParentJob = null
-  }
-}
-
-export function queuePostFlushCb(cb: Function | Function[]) {
+function queueCb(
+  cb: Function | Function[],
+  activeQueue: Function[] | null,
+  pendingQueue: Function[],
+  index: number
+) {
   if (!isArray(cb)) {
     if (
-      !pendingPostFlushCbs ||
-      !pendingPostFlushCbs.includes(
-        cb,
-        (cb as SchedulerJob).cb
-          ? pendingPostFlushIndex + 1
-          : pendingPostFlushIndex
-      )
+      !activeQueue ||
+      !activeQueue.includes(cb, (cb as SchedulerJob).cb ? index + 1 : index)
     ) {
-      postFlushCbs.push(cb)
+      pendingQueue.push(cb)
     }
   } else {
     // if cb is an array, it is a component lifecycle hook which can only be
     // triggered by a job, which is already deduped in the main queue, so
     // we can skip dupicate check here to improve perf
-    postFlushCbs.push(...cb)
+    pendingQueue.push(...cb)
   }
   queueFlush()
 }
 
-function queueFlush() {
-  if (!isFlushing && !isFlushPending) {
-    isFlushPending = true
-    currentFlushPromise = resolvedPromise.then(flushJobs)
-  }
+export function queuePreFlushCb(cb: Function) {
+  queueCb(cb, activePreFlushCbs, pendingPreFlushCbs, preFlushIndex)
 }
 
-export function flushPostFlushCbs(seen?: CountMap) {
-  if (postFlushCbs.length) {
-    pendingPostFlushCbs = [...new Set(postFlushCbs)]
-    postFlushCbs.length = 0
+export function queuePostFlushCb(cb: Function | Function[]) {
+  queueCb(cb, activePostFlushCbs, pendingPostFlushCbs, postFlushIndex)
+}
+
+export function flushPreFlushCbs(
+  seen?: CountMap,
+  parentJob: SchedulerJob | null = null
+) {
+  if (pendingPreFlushCbs.length) {
+    currentPreFlushParentJob = parentJob
+    activePreFlushCbs = [...new Set(pendingPreFlushCbs)]
+    pendingPreFlushCbs.length = 0
     if (__DEV__) {
       seen = seen || new Map()
     }
     for (
-      pendingPostFlushIndex = 0;
-      pendingPostFlushIndex < pendingPostFlushCbs.length;
-      pendingPostFlushIndex++
+      preFlushIndex = 0;
+      preFlushIndex < activePreFlushCbs.length;
+      preFlushIndex++
     ) {
       if (__DEV__) {
-        checkRecursiveUpdates(seen!, pendingPostFlushCbs[pendingPostFlushIndex])
+        checkRecursiveUpdates(seen!, activePreFlushCbs[preFlushIndex])
       }
-      pendingPostFlushCbs[pendingPostFlushIndex]()
+      activePreFlushCbs[preFlushIndex]()
     }
-    pendingPostFlushCbs = null
-    pendingPostFlushIndex = 0
+    activePreFlushCbs = null
+    preFlushIndex = 0
+    currentPreFlushParentJob = null
+    // recursively flush until it drains
+    flushPreFlushCbs(seen, parentJob)
+  }
+}
+
+export function flushPostFlushCbs(seen?: CountMap) {
+  if (pendingPostFlushCbs.length) {
+    activePostFlushCbs = [...new Set(pendingPostFlushCbs)]
+    pendingPostFlushCbs.length = 0
+    if (__DEV__) {
+      seen = seen || new Map()
+    }
+    for (
+      postFlushIndex = 0;
+      postFlushIndex < activePostFlushCbs.length;
+      postFlushIndex++
+    ) {
+      if (__DEV__) {
+        checkRecursiveUpdates(seen!, activePostFlushCbs[postFlushIndex])
+      }
+      activePostFlushCbs[postFlushIndex]()
+    }
+    activePostFlushCbs = null
+    postFlushIndex = 0
   }
 }
 
@@ -149,6 +167,8 @@ function flushJobs(seen?: CountMap) {
     seen = seen || new Map()
   }
 
+  flushPreFlushCbs(seen)
+
   // Sort queue before flush.
   // This ensures that:
   // 1. Components are updated from parent to child. (because parent is always
@@ -175,11 +195,12 @@ function flushJobs(seen?: CountMap) {
     queue.length = 0
 
     flushPostFlushCbs(seen)
+
     isFlushing = false
     currentFlushPromise = null
     // some postFlushCb queued jobs!
     // keep flushing until it drains.
-    if (queue.length || postFlushCbs.length) {
+    if (queue.length || pendingPostFlushCbs.length) {
       flushJobs(seen)
     }
   }