fix: retry on Suspense render and preserve hook state#5054
fix: retry on Suspense render and preserve hook state#5054mary-ext wants to merge 1 commit intopreactjs:mainfrom
Conversation
📊 Tachometer Benchmark ResultsSummary⏳ Benchmarks are currently running. Results below are out of date. duration
usedJSHeapSize
Results⏳ Benchmarks are currently running. Results below are out of date. create10kduration
usedJSHeapSize
filter-listduration
usedJSHeapSize
hydrate1kduration
usedJSHeapSize
many-updatesduration
usedJSHeapSize
replace1kduration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-updateduration
usedJSHeapSize
tododuration
usedJSHeapSize
update10th1kduration
usedJSHeapSize
|
This is kind of incorrect, for a mounted component react preserves the hooks-state however for a component that has yet to mount the state is discarded --> WIP Fiber discarded
I should check this but in react 17-18 this was that way in React as well - during my research at least. Which is also the reason why I'm going to try and find time in the following weeks to verify this |
|
I tested this in https://stackblitz.com/edit/vitejs-vite-emrmkojz?file=src%2FApp.tsx&terminal=dev
|
I inferred this would be the case when I glanced on software-mansion/react-freeze@2e6ced2 but somehow ignored it and didn't think of checking in-depth aside from checking whether hook state is preserved at all, definitely worth replicating |
noted, might not make too much sense here I think |
mary-ext
force-pushed
the
fix-suspense
branch
2 times, most recently
from
b4efcd7 to
c54d8bd
Compare
| resolve().then(assert).catch(assert); | ||
| }); | ||
|
|
||
| it('should reset hooks of components', () => { |
There was a problem hiding this comment.
Can we re-introduce these tests
There was a problem hiding this comment.
reintroduced but renamed the test case since the behavior changed
compat/src/suspense.js
Outdated
| this._suspendedVNode = null; | ||
| this._pendingSuspensionCount = 0; | ||
| this._suspenders = null; | ||
| state._suspended = null; |
There was a problem hiding this comment.
Saving some bytes, I am however weary about mutating state here, what is the reason for this? Also we reset this._suspendedVNode does this mean that two subsequent renders will not work?
| this._suspendedVNode = null; | |
| this._pendingSuspensionCount = 0; | |
| this._suspenders = null; | |
| state._suspended = null; | |
| this._pendingSuspensionCount = 0; | |
| state._suspended = this._suspendedVNode = this._suspenders = null; |
There was a problem hiding this comment.
we need state._suspended to be null immediately as the render code immediately below it checks state._suspended to decide whether to render props.children or the fallback, using setState mean we'd wait another tick.
I'm not sure which case you mean by two subsequent renders here but I have added tests for them
compat/src/suspense.js
Outdated
| }); | ||
|
|
||
| vnode._component.__hooks = null; | ||
| if (vnode._dom != null) { |
There was a problem hiding this comment.
My concern with this is that in Suspenseful scenarios we will set _dom maybe we should use _excess for everything then to counter-act this becoming a problem https://github.com/preactjs/preact/blob/main/src/diff/index.js#L361 - this does mean that we can't backport this to the 10.x release line safely. It would be part of v11
There was a problem hiding this comment.
seems fine as a v11-only change, should I change it to _excess then? though it's worth noting that _detachOnNextRender, which calls detachedClone, is only set when we're not in the middle of hydration
Lines 168 to 180 in 21dd6d0
There was a problem hiding this comment.
I mainly mean that during the old hydration route the hooks clearing logic will miss behave because it will see suspended vnodes as mounted due to the presence of _dom. I don't think it's a bad thing to, in a separate refactor, move that core logic to fully use _excess
There was a problem hiding this comment.
ah sorry, should've checked how it actually goes for v10, I've added an _excess check as an extra
- If a parent rerenders while a child suspends, retry rerendering suspended children. This supports patterns like react-freeze where a never-resolving thenable freezes a subtree and a parent re-render clears it. - Preserve hook state (useState, etc.) across suspend/unsuspend for already-mounted components. Discard hook state for components that suspend during initial mount (matching React's WIP fiber discard). - Run effect cleanups (both useEffect and useLayoutEffect) uniformly during suspension. Effects rerun when unsuspended.
3 participants
I was figuring out why react-freeze didn't work and it boiled down these issues:
when a child suspends, Preact does not differentiate useEffect and useLayoutEffect, only the latter should be cleaned up during suspension