fix(scheduler): job ordering when the post queue is flushing#12090
Merged
yyx990803 merged 2 commits intovuejs:mainfrom Oct 3, 2024
Merged
fix(scheduler): job ordering when the post queue is flushing#12090yyx990803 merged 2 commits intovuejs:mainfrom
yyx990803 merged 2 commits intovuejs:mainfrom
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: |
Member
|
/ecosystem-ci run |
Contributor
|
📝 Ran ecosystem CI: Open
|
Member
|
/ecosystem-ci run radix-vue |
Contributor
|
📝 Ran ecosystem CI: Open
|
Contributor
Author
|
Seems that |
Member
|
/ecosystem-ci run |
Contributor
Author
|
The |
Contributor
|
📝 Ran ecosystem CI: Open
|
yangmingshan
added a commit
to vue-mini/vue-mini
that referenced
this pull request
Oct 12, 2024
abdullah-wn
added a commit
to Lazy-work/vue
that referenced
this pull request
Jan 4, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are a couple of places where the scheduler has logic like this:
But there's a problem with how
flushJobsresets these variables:core/packages/runtime-core/src/scheduler.ts
Lines 258 to 264 in 29de6f8
The problem is that while
flushPostFlushCbs(seen)is running,isFlushingwill still betrue, butflushIndexwill be0. That leads to the value ofstartbeing1, when it should be0.Here's an example that shows this in action:
When clicking the button, notice that rendering runs before the
prewatcher. This is becausefindInsertionIndexstarts 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 whenflushPreFlushCbsis called, but it can happen via a nested call tomount()inside a post job.The main idea behind the fix is to use
-1rather than0forflushIndexwhen 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
isFlushingflag, 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 bothisFlushingandisFlushPending, just usingcurrentFlushPromisein 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 = nullto 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.