-
Notifications
You must be signed in to change notification settings - Fork 46.3k
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
Add withSuspenseConfig API #15593
Add withSuspenseConfig API #15593
Conversation
React: size: 🔺+0.4%, gzip: 🔺+0.3% ReactDOM: size: 🔺+0.9%, gzip: 🔺+0.8% Details of bundled changes.Comparing: 1160b37...4347de6 react
react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
Why does this example show a "user blocking update suspended" if I click more than once? |
@gaearon You need scheduler.next too. |
OK this does work better. https://gist.github.com/gaearon/15f4b0f8265a1988428f7f60f347d80d |
af05c4a
to
41c90de
Compare
@@ -220,6 +220,7 @@ ReactBatch.prototype.render = function(children: ReactNodeList) { | |||
internalRoot, | |||
null, | |||
expirationTime, | |||
null, |
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.
This means that batches never suspend, but I'm not sure it makes sense for them to suspend anyway since they control the commit anyway. I'm not sure when the promise should resolve in this new multi-commit world.
// Within the scope of the callback, mark all updates as being allowed to suspend. | ||
export function suspendIfNeeded(scope: () => void, config?: SuspenseConfig) { | ||
const previousConfig = ReactCurrentBatchConfig.suspense; | ||
ReactCurrentBatchConfig.suspense = config || DefaultSuspenseConfig; |
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.
Nit: Explicit comparison?
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.
What do we do elsewhere? I don't want any bad types like undefined or false to leak into the implementation since it might not immediately throw only deopt, and I don't want to add a bunch of prod runtime code since this should be a fairly very light operation.
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.
Elsewhere we check for null and undefined explicitly, like
ReactCurrentBatchConfig.suspense = config !== undefined && config !== null ? config : DefaultSuspenseConfig;
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 guess I'm wondering why you've chosen a different pattern here, and whether we should go change the other sites. Like root.render
:
react/packages/react-dom/src/client/ReactDOM.js
Lines 372 to 378 in 3d8b836
ReactRoot.prototype.render = function( | |
children: ReactNodeList, | |
callback: ?() => mixed, | |
): Work { | |
const root = this._internalRoot; | |
const work = new ReactWork(); | |
callback = callback === undefined ? null : callback; |
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.
Now I wonder if null
should be resetting it to not suspend? I guess you can also pass in timeout: 0 but then that opts you out of JND.
const previousConfig = ReactCurrentBatchConfig.suspense; | ||
ReactCurrentBatchConfig.suspense = config || DefaultSuspenseConfig; | ||
try { | ||
scope(); |
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.
Should this return? For consistency with flushSync
, next
, runWithPriority
, et al.
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 guess. What is that ever used for? I could imagine things like async functions but that seems like very confusing bad practice to use an async function here.
export function markRenderEventTime(expirationTime: ExpirationTime): void { | ||
if (expirationTime < workInProgressRootMostRecentEventTime) { | ||
workInProgressRootMostRecentEventTime = expirationTime; | ||
export function markRenderEventTimeAndConfig( |
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.
Naming suggestion: markBatchConfig
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.
markExpirationAndSuspenseConfig
?
Several callers must supply the expiration but don't supply a config, and that config likely wouldn't be the "whole batch's config" but specifically it is where you pass an object or null that is the interesting flag here.
// We don't know exactly when the update was scheduled, but we can infer an | ||
// approximate start time from the expiration time. | ||
const earliestExpirationTimeMs = expirationTimeToMs(expirationTime); | ||
return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION; | ||
return ( | ||
earliestExpirationTimeMs - |
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.
Let's store the start time on the batch config so we don't have to infer it? Can do in a follow up.
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.
However the event time computation is pretty broken right now and it might be nice to fix it soon so we can rule it out as a source of possible wonkiness when we get bug reports.
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 don't want to have to create a new object for the batch too.
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.
Can it not be the same object as suspenseConfig
?
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.
suspenseConfig
is a user space object so mutating it gets fragile. I also expect it to mostly be a hoisted shared object. E.g. the default one right now but also the defaults in hooks would just use a hoisted config.
We could have another field for that but that requires me to track another thing for the render path. It's probably the right strategy eventually but I think we need to think about it some more.
41c90de
to
ffab198
Compare
Is this ready for review? Don't see any tests |
ffab198
to
ce030c0
Compare
How do you decide what to put on React (this) vs Scheduler (next)? I understand React gives a stronger sintleton guarantee but as a user the distinction seems blurred to me. Especially because a bunch of similar modifiers live on the renderer (flushSync). |
Adds a "current" suspense config that gets applied to all updates scheduled during the current scope. I suspect we might want to add other types of configurations to the "batch" so I called it the "batch config". This works across renderers/roots but they won't actually necessarily go into the same batch.
We'll use this info to suspend a commit for longer when necessary.
This lets us track which renders we want to suspend for a short time vs a longer time if possible.
This can be used to implement spinners that don't flicker if the data and rendering is really fast.
This is a required argument in the type signature but people may not supply it and this is a user facing object.
This allow opting out of suspending in some nested scope. A lot of time when you use this function you'll use it with high level helpers. Those helpers often want to accept some additional configuration for suspense and if it should suspend at all. The easiest way is to just have the api accept null or a suspense config and pass it through. However, then you have to remember that calling suspendIfNeeded has a default. It gets simpler by just saying tat you can pass the config. You can have your own default in user space.
f66d761
to
800d8c3
Compare
This ensures that if we've scheduled lower pri work that doesn't have a suspenseConfig, we don't consider its expiration as the timeout.
This adds
unstable_withSuspenseConfig(callback, config)
to thereact
package. This is a primitive that can be used to implement "busy spinners". I recommend reviewing the code commit by commit.This sets the config as a global state available to all React renderers during the scope of the synchronously executed callback (first arg). If you don't pass a config, it disables suspense (for longer than the default JND).
If any updates are scheduled (render, setState, dispatch) during this scope, then they get this "suspense config" associated with the update.
If during a render pass, we process one of those updates, we mark this render pass as having a "suspense config".
If during a render pass, we are forced to turn already visible content back into a fallback, we mark this render as suspended with a delay if possible. (This doesn't not happen during initial render of new content.)
If we have both marked this render as having a suspense config and it was suspended with delay, then we extend the suspended time beyond JND all the way to the timeout defined in the suspense config.
The suspense config has these options:
The last two are used when we have actually completed a render fairly quickly. If we have exceeded
loadingDelayMs
but have not yet exceededloadingDelayMs + minLoadingDurationMs
then we suspend the commit until that time even though we could commit early. This can be used to implement busy spinners that delay when they first show but want to stay visible for a minimum time if they do show, to avoid flickering.Tasks: