-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(scheduler): job ordering when the post queue is flushing #12090
fix(scheduler): job ordering when the post queue is flushing #12090
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-ssr
@vue/reactivity
@vue/compiler-sfc
@vue/runtime-dom
@vue/runtime-core
@vue/shared
@vue/server-renderer
@vue/compat
vue
commit: |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
/ecosystem-ci run radix-vue |
📝 Ran ecosystem CI: Open
|
Seems that |
/ecosystem-ci run |
The |
📝 Ran ecosystem CI: Open
|
There are a couple of places where the scheduler has logic like this:
But there's a problem with how
flushJobs
resets these variables:core/packages/runtime-core/src/scheduler.ts
Lines 258 to 264 in 29de6f8
The problem is that while
flushPostFlushCbs(seen)
is running,isFlushing
will still betrue
, butflushIndex
will be0
. That leads to the value ofstart
being1
, when it should be0
.Here's an example that shows this in action:
When clicking the button, notice that rendering runs before the
pre
watcher. This is becausefindInsertionIndex
starts the binary search at1
, instead of0
.Here's the same example, but using this PR:
The other place that uses similar logic is
flushPreFlushCbs
. It's much more difficult to hit that one, as the post queue isn't usually running whenflushPreFlushCbs
is called, but it can happen via a nested call tomount()
inside a post job.The main idea behind the fix is to use
-1
rather than0
forflushIndex
when the queue isn't currently flushing. The result is that the 'next job' is alwaysflushIndex + 1
, irrespective of whether the queue is currently running. So code like this:... simplifies down to:
With this change made, there's no longer any need to have a separate
isFlushing
flag, so I merged it intoisFlushPending
. But once those two flags are merged, the new flag is essentially just a boolean version ofcurrentFlushPromise
. So I removed bothisFlushing
andisFlushPending
, just usingcurrentFlushPromise
in their place.That all shaved a few extra bytes off the build and reduced the complexity of the scheduler a little.
When I first opened this PR it also included a change that moved the line
currentFlushPromise = null
to the end offlushJobs
. I still think that's more correct, but it breaks the tests forradix-vue
, so I've reverted that part of the change.