API for prerendering a top-level update and deferring the commit #11346
Conversation
| // Perform work on root as if the given expiration time is the current time. | ||
| // This has the effect of synchronously flushing all work up to and | ||
| // including the given time. | ||
| performWorkOnRoot(root, expirationTime, expirationTime); |
There was a problem hiding this comment.
@sebmarkbage This is the part we were discussing earlier. Need some way to tell the reconciler to treat all work up to expirationTime as synchronous. Here we do this by passing the same time as both the current time and the expiration time. But since the reconciler only uses the current time to check whether it's sync or async, a boolean would also work.
| expect(container.textContent).toEqual('Hi'); | ||
| }); | ||
|
|
||
| it("does not restart a blocked root that wasn't updated", () => { |
There was a problem hiding this comment.
This feels like a superset of the previous test (same things being tested, but with a few more expects). Should we remove the previous?
There was a problem hiding this comment.
No I like having the focused cases so that if I’m debugging this later and those fail I can focus on fixing those first, which may have the effect of fixing the complex ones too.
| // TODO: This belongs in the renderer, which suggests we should use something | ||
| // other than the UpdateQueue type, since it would be duplicated across | ||
| // packages regardless. | ||
| deferredCommits: UpdateQueue<null> | null, |
There was a problem hiding this comment.
Can you explain conceptually what this is and why we need it? Why isn't this just part of the normal update queue? What is it about this that requires a separate data structure?
I was expecting these to go on the update queue of the root and just not committed.
There was a problem hiding this comment.
Hmm, that's how I did it in #11232... The root state contains a deferred field that's true if the tree shouldn't be committed yet. Don't know why I didn't do that here again. I think maybe I thought it would conflict with the new deterministic updates behavior, but that doesn't sound right. I'll update.
There was a problem hiding this comment.
Ah, I think it was this: the current renderRoot API returns finishedWork if the tree completes, but there's no way to say "this tree has no more work left but also it isn't ready to commit yet."
I wish we could return multiple values from renderRoot. Like a tuple of (finishedWork, deferCommit, didError, errorObject)
There was a problem hiding this comment.
I don't see how this follows. There's something off in the model here. Both the deferred flag and this separate list models something much more powerful (arbitrary updates being deferred and placed in separate order) than what can actually happen. Makes it hard to think about and track bugs
There was a problem hiding this comment.
The tricky part is that a root can be deferred at multiple levels. When the first one is flushed, the second level is still blocked from committing.
After talking offline, I'll update to use an array of expiration times instead of using the UpdateQueue type.
|
@sebmarkbage Ended up storing both the "deferred" state and completion callbacks on the work objects, and turning the work objects into a linked list. Could also use an array, but since we were already allocating the nodes, a linked list seemed better. |
acdlite
force-pushed
the
prerendering
branch
2 times, most recently
from
e28e46f to
2318255
Compare
| return; | ||
| } | ||
| for (let i = 0; i < callbacks.length; i++) { | ||
| // TODO: Error handling |
There was a problem hiding this comment.
For whenever we figure out how errors inside a then callback should be handled :D Would like to do that before landing this PR, if we intend this to ship in 16.1.
There was a problem hiding this comment.
My current thinking is that then callbacks should always resolve when the tree completes, regardless of errors. I don't think we can/should support rejection handlers in the case of unhandled render errors, because it won't be clear which update caused the error. Could be from a previous or subsequent update.
What I'm not sure about is the rethrow behavior. If there's an unhandled render error, and the then callback throws, which error should we rethrow at the end?
Decouples the UpdateQueue type from the Fiber type.
Adds the ability to start rendering work without flushing the changes to the screen, by blocking the commit phase. This can be used to coordinate React's commit phase with other async work. - root.prerender schedules an update and returns a work object. - work.commit unblocks the commit phase and flushes the remaining work synchronously. (Not yet implemented is work.then, which schedules a callback that fires once the work has completed.)
We need to keep track of top-level completion callbacks (those passed to `work.then`) and expiration times at which commits are deferred. Rather than storing these in an UpdateQueue, we can use the work object, which already contain an expiration time and completion callback. These now form a list of work objects, sorted by expiration time.
Adds the ability to start rendering work without flushing the changes
to the screen, by blocking the commit phase. This can be used to
coordinate React's commit phase with other async work.
root.prerenderschedules an update and returns a work object.work.thenschedules a completion callback that fires once the work completes.work.commitunblocks the commit phase and flushes the remainingwork synchronously.
(Lazy roots that start rendering before the DOM container is available are not yet implemented; I'll do that in a subsequent PR.)
root.renderandroot.unmountwere updated to also return work objects. In those cases, since the commit phase is not deferred, by the time thethencallback is fired, the work has already committed.