-
Notifications
You must be signed in to change notification settings - Fork 47k
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
[scheduler] Deadline object -> shouldYield #14025
Conversation
Instead of using a requestIdleCallback-style deadline object, expose a method Scheduler.shouldYield that returns true if there's a higher priority event in the queue.
React: size: -1.4%, gzip: -0.6% ReactDOM: size: -0.1%, gzip: -0.0% Details of bundled changes.Comparing: 21a79a1...55da990 react
react-dom
react-test-renderer
react-reconciler
react-art
react-native-renderer
scheduler
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change 😄
function shouldYield() { | ||
if ( | ||
scheduledCallbackTimeout === -1 || | ||
elapsedTimeInMs > scheduledCallbackTimeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's early and I'm probably being stupid, but shouldn't this be elapsedTimeInMs < scheduledCallbackTimeout
?
Edit Never mind. This is just saying that we should never yield if work has already timed out.
|
||
function performAsyncWork() { | ||
try { | ||
if (!shouldYieldToRenderer()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not initially obvious to me that dl.didTimeout
is semantically equivalent to shouldYieldToRenderer()
but I trust you here.
) { | ||
performWorkOnRoot( | ||
nextFlushedRoot, | ||
nextFlushedExpirationTime, | ||
currentRendererTime >= nextFlushedExpirationTime, | ||
!(currentRendererTime >= nextFlushedExpirationTime), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, why ! >=
rather than <
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Planned to change it once I proved it worked, then forgot :D
expectedNumberOfYields !== -1 && | ||
yieldedValues.length >= expectedNumberOfYields | ||
) { | ||
// We at least as many values as expected. Stop rendering. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"We yielded at least as many"?
* [scheduler] Deadline object -> shouldYield Instead of using a requestIdleCallback-style deadline object, expose a method Scheduler.shouldYield that returns true if there's a higher priority event in the queue. * Nits
* [scheduler] Deadline object -> shouldYield Instead of using a requestIdleCallback-style deadline object, expose a method Scheduler.shouldYield that returns true if there's a higher priority event in the queue. * Nits
Instead of using a requestIdleCallback-style deadline object, expose a method Scheduler.shouldYield that returns true if there's a higher priority event in the queue.