[Wait for Hooks before merging] Scheduler.unstable_next #14756
Merged
Conversation
This file contains 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
acdlite
force-pushed
the
scheduler-next
branch
from
89b06f8 to
b738b33
Compare
|
React: size: 🔺+1.1%, gzip: 🔺+0.5% ReactDOM: size: 🔺+0.4%, gzip: -0.4% Details of bundled changes.Comparing: d1326f4...5bc0696 react
react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
scheduler
Generated by 🚫 dangerJS |
sebmarkbage
approved these changes
Feb 5, 2019
Jessidhia
reviewed
Feb 5, 2019
| // If we're in the middle of rendering a tree, do not update at the same | ||
| // expiration time that is already rendering. | ||
| if (nextRoot !== null && expirationTime === nextRenderExpirationTime) { | ||
| expirationTime -= 1; |
There was a problem hiding this comment.
One thing that confused me when I was reading this: is this before or after the nextRenderExpirationTime?
| // Outside of concurrent mode, updates are always synchronous. | ||
| expirationTime = Sync; | ||
| } else if (isWorking && !isCommitting) { | ||
| // During render phase, updates expire during as the current render. |
There was a problem hiding this comment.
"during the current render"? "as the current render"? "with the current render"?
| @@ -264,6 +264,37 @@ function unstable_runWithPriority(priorityLevel, eventHandler) { | |||
| } | |||
| } | |||
|
|
|||
| function unstable_next(eventHandler) { | |||
There was a problem hiding this comment.
This is IIUC an inlined version of:
return runWithPriority(
Math.min(NormalPriority, getCurrentPriorityLevel()),
eventHandler
)?
Would be nice to be able to treat it more as a continuation; there is already support for it in the scheduler after all... but continuations would inherit the current deadline, defeating the point. Would it work if the continuation itself was wrapped with wrapCallback?
At any rate adding non-imperative continuations to React event handlers is likely a breaking change.
Jessidhia
reviewed
Feb 5, 2019
| passiveEffectCallbackHandle = schedulePassiveEffects(callback); | ||
| passiveEffectCallbackHandle = runWithPriority(NormalPriority, () => { | ||
| return schedulePassiveEffects(callback); | ||
| }); |
There was a problem hiding this comment.
When I was reading this code last week I was wondering how you'd be able to mess with the schedulePassiveEffects priority. Doesn't seem normally possible... unless you cause a Sync update I guess...
acdlite
changed the title
Changes the implementation of syncUpdates, deferredUpdates, and interactiveUpdates to use runWithPriority, so This is the minimum integration between Scheduler and React needed to unblock use of the Scheduler.next API.
Changes the implementation of syncUpdates, deferredUpdates, and interactiveUpdates to use runWithPriority, so This is the minimum integration between Scheduler and React needed to unblock use of the Scheduler.next API.
acdlite
force-pushed
the
scheduler-next
branch
from
b738b33 to
5bc0696
Compare
bvaughn
pushed a commit
to bvaughn/react
that referenced
this pull request
Feb 6, 2019
This reverts commit bc9818f.
n8schloss
pushed a commit
to n8schloss/react
that referenced
this pull request
Feb 8, 2019
* Add Scheduler.unstable_next * Use Scheduler to prioritize updates Changes the implementation of syncUpdates, deferredUpdates, and interactiveUpdates to use runWithPriority, so This is the minimum integration between Scheduler and React needed to unblock use of the Scheduler.next API. * Add Scheduler.unstable_next * Use Scheduler to prioritize updates Changes the implementation of syncUpdates, deferredUpdates, and interactiveUpdates to use runWithPriority, so This is the minimum integration between Scheduler and React needed to unblock use of the Scheduler.next API.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Mar 1, 2019
This PR introduced some bugs in concurrent mode during internal testing. Until we figure out a proper solution, I'm going to try reverting it. Not totally certain this is sufficient to unbreak the bugs we found, but I'm using this branch to determine that.
acdlite
added a commit
that referenced
this pull request
Mar 1, 2019
* Revert #14756 changes to ReactFiberScheduler This PR introduced some bugs in concurrent mode during internal testing. Until we figure out a proper solution, I'm going to try reverting it. Not totally certain this is sufficient to unbreak the bugs we found, but I'm using this branch to determine that. * Add back commented out Scheduler import With a note not to use named imports next time we import Scheduler in this module.
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.
When a user initiates an action, they expect immediate feedback. They don't necessarily expect the action to finish immediately, but they do expect some sort of response from the UI within a timeframe that feels immediate, so they know their request was received.
In React, the recommended way to achieve this is to split expensive updates into two separate tasks. The first task contains work that needs to finish immediately. We refer to this as a "user blocking" update. An example is, when switching to a new tab in a tabbed interface, the new tab should change color immediately after it's clicked.
Everything that doesn't need to happen immediately (i.e. doesn't block the user) should happen in a second task. This task typically takes longer to finish, either because it's CPU intensive or because it relies on IO-bound resources. In the tab switching example, this includes loading and rendering the content that corresponds to the newly selected tab. We refer to this as a "normal" update, because this category tends to encompass most of the work in a React application.
To implement this pattern in React, you need a separate
setStatecall for each of the two tasks. The firstsetStateshould happen directly inside the event handler. React will automatically infer that this update is user-blocking and schedule it at the corresponding priority.The second update — the more expensive one that does the bulk of the work — should be wrapped in a call to
Scheduler.next:Updates initiated inside the function passed to
Scheduler.nextare scheduled with "normal" priority, instead of user-blocking priority. (The function itself is immediately invoked, a labatchedUpdatesandflushSync.)The name "next" is intentionally vague about its exact semantics. It's meant to imply that the task scheduled inside passed function happen after the task scheduled directly inside the event handler.