Hydration Suspend & Resume#2754
Conversation
|
Size Change: +367 B (0%) Total Size: 40.5 kB
ℹ️ View Unchanged
|
andrewiggins
left a comment
There was a problem hiding this comment.
I know I'm a little late to the party 😅 but left a couple thoughts from an initial read for a later PR. I suspect when we add some tests we'll have some tweaks to make so things can be addressed then.
| error: Promise<void>, | ||
| suspendingComponent: Component<any, any> | ||
| suspendingComponent: Component<any, any>, | ||
| oldVNode?: VNode |
There was a problem hiding this comment.
I think we can remove this - I didn't see anywhere where an implementation of _childDidSuspend took a third param
There was a problem hiding this comment.
I wasn't sure if we needed to add it in order to fix the issue you mentioned about picking the wrong vnode on which to check _hydrating.
| * to remain on screen and hydrate it when the suspense actually gets resolved. | ||
| * While in non-hydration cases the usual fallback -> component flow would occour. | ||
| */ | ||
| const vnode = c._vnode; |
There was a problem hiding this comment.
I still need to run this code to verify my understanding, but my first reading through makes me think this may need to be suspendingComponent._vnode since we set vnode._hydrating on the vnode that threw the error (diff/index.js:275), not on Suspense._vnode (though we could in Suspense's _catchError implementation at the top of this file).
There was a problem hiding this comment.
You may be right. I need to clarify which vnode(s) we want to set the _hydrating bit on. My preference would be to set _hydrating on the vnode for the component that caught/handled the suspend, since technically we bail out of the tree at that point and must resume from that point.
There was a problem hiding this comment.
Ah - yeah currently we're setting it on both the bailed-out VNode and the VNode for the component that handled the bailout:
https://github.com/preactjs/preact/pull/2754/files#diff-b5e3843a998fbd0144a6ac5b1b7536a4R33
There was a problem hiding this comment.
Yea, that makes sense to me (setting in on the place in the tree where we bail out and resume). Though I think when using compat's Suspense, the _catchError code isn't run since Suspense ships it's own _catchError implementation that doesn't have these changes to set hydrating, but that'd be trivial to add.
Looking back at it, I'm don't remember why we didn't just use componentDidCatch to implement Suspense... Maybe we needed something like your handled logic but no one tried that at the time?
There was a problem hiding this comment.
I think the reason for not using componentDidCatch was that some users were confused why the received a Promise in their error handlers instead of an Error object. We originally implemented it that way iirc, but backpedaled.
| _nextDom: PreactElement | Text | null; | ||
| _nextDom: PreactElement | null; | ||
| _component: Component | null; | ||
| _hydrating: boolean | null; |
There was a problem hiding this comment.
Perhaps this wouldn't compress as well since we deal with nulls way more than booleans, but just leaving a note for myself (cuz I'm curious) to explore pros/cons of just making this a normal boolean that is initialized to false and flipped between true/false as appropriate.
There was a problem hiding this comment.
@andrewiggins this needs to be a tri-state though:
true: the vnode has DOM from bailout during hydrationfalse: the vnode has DOM from bailout during initial render (new tree creation)null: the vnode has no DOM from bailout
We could use an int though, that's probably much better for perf.
There was a problem hiding this comment.
Oh, I missed that false case. Makes sense
This PR implements suspend & resume for both hydration and new tree construction. If rendering throws during hydration or new tree construction (first render of a component or tree), catching the error up the tree in something like an "Error Boundary" will attach the hydration state and available DOM tree to the boundary's VNode. Triggering a render on this node (via any mechanism) attempts to resume from this state. The throw/catch process can be repeated and will continue to preserve DOM state. Once rendering completes, affected VNodes are un-marked and the behavior no longer occurs.