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.
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.
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.
Elsewhere we check for null and undefined explicitly, like
ReactCurrentBatchConfig.suspense = config !== undefined && config !== null ? config : DefaultSuspenseConfig;There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Should this return? For consistency with flushSync, next, runWithPriority, et al.
There was a problem hiding this comment.
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.
Naming suggestion: markBatchConfig
There was a problem hiding this comment.
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.
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.
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.
I don't want to have to create a new object for the batch too.
There was a problem hiding this comment.
Can it not be the same object as suspenseConfig?
There was a problem hiding this comment.
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 thereactpackage. 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
loadingDelayMsbut have not yet exceededloadingDelayMs + minLoadingDurationMsthen 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: