Skip finalizeContainerChildren for suspended transition #30521
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
Work-in-progress idea targeting the same situation as facebook#30513. In React Native, we've found a bug with suspense boundaries reverting to the fallback in the following case. The expected behavior (and behavior on web) is as follows: 1. Render a Suspense boundary on initial mount, resolve the promise, show the content. 2. In a transition, update the content of the existing Suspense boundary, suspending again. 3. The UI should continue to show the previous content 4. Resolve the promise, update to the new content. However on RN, step 3 shows the fallback again. This is unexpected since we're in a transition and the fallback has already revealed. What's happening is that in step 3 we call completeWork() which in turn calls finalizeContainerChildren(), which updates to use the fallback. However, this isn't committed to the screen since we never call commitRoot() (and therefore don't call replaceContainerChildren()). Instead, we detec that the render exited with status RootSuspendedOnDelay and that the lane was transition-only, so we don't actually commit the fallback. However, RN currently commits the content it gets in finalizeContainerChildren(), rather than waiting for replaceContainerChildren() from commitRoot(). The original intent was for finalize...() to do layout, and for replace...() to actually commit the updated tree, which would preserve the web behavior. facebook#30513 is a brute force way to address this by simply moving all the native work from finalize -> replace. What i'm experimenting with in this PR is to skip calling finalizeContainerChildren() if the above conditions hold: if this is a suspended render in a transition, don't bother finalizing children.
josephsavona
force-pushed
the
skip-finalize-for-suspended-transition
branch
from
f3d151f to
df27420
Compare
|
Comparing: bea5a2b...5aa09ec Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
This wouldn't be sufficient in that you still need #30513 or split Fabric into completeWork (perform layout) and commitWork (send to main thread). That's because this is not the only case that waits to commit and might be interrupted before it commits, but also might commit after a timeout. For example a throttled Suspense reveal is another case. In principle I don't think this is necessarily wrong to skip in the case where we know we won't commit. However if we know we won't commit we should really trigger the unwind path rather than the complete path to skip any work including any warm up work. This is related to the sibling warm up problem though since that is triggering the unwind path so we really need to know where that lands. E.g. if it's a warm up path, then do all of it, otherwise don't. I'll also note that it's not necessarily wrong to do the layout of a tree that won't commit. That's because Yoga layout nodes can keep caches and a tree that has subtrees that remain the same can be skipped later. So it's a form of warm up. However, it would probably be better to do the final tree with holes in it rather than preparing a tree with Suspense boundaries with fallbacks. Like the closer you are to the final content the more likely a re-layout:ed node will end up with the same size as the optimistic render. Which takes me to the other observation. We really shouldn't be rendering the fallbacks in this case. Again, the easiest would probably be to unwind instead of complete when doing this. |
Yes! Agree that this would be ideal. So finish the render, then if this condition holds unwind instead of running completeWork(). Or even better, if we’re in a transition we can unwind as soon as we hit a suspended boundary, avoiding rendering the fallback. I’ll explore both options. |
4 participants
Work-in-progress idea targeting the same situation as #30513.
In React Native, we've found a bug with suspense boundaries reverting to the fallback in the following case. The expected behavior (and behavior on web) is as follows:
However on RN, step 3 shows the fallback again. This is unexpected since we're in a transition and the fallback has already revealed.
What's happening is that in step 3 we call completeWork() which in turn calls finalizeContainerChildren(), which updates to use the fallback. However, this isn't committed to the screen since we never call commitRoot() (and therefore don't call replaceContainerChildren()). Instead, we detec that the render exited with status RootSuspendedOnDelay and that the lane was transition-only, so we don't actually commit the fallback.
However, RN currently commits the content it gets in finalizeContainerChildren(), rather than waiting for replaceContainerChildren() from commitRoot(). The original intent was for finalize...() to do layout, and for replace...() to actually commit the updated tree, which would preserve the web behavior.
#30513 is a brute force way to address this by simply moving all the native work from finalize -> replace. What i'm experimenting with in this PR is to skip calling finalizeContainerChildren() if the above conditions hold: if this is a suspended render in a transition, don't bother finalizing children.
NOTE: i'm sure that this is failing to account for some cases and likely won't work exactly as-is. I'm curious for feedback about whether this general approach could work — is there some set of conditions for which we can skip finalizeContainerChildren — or whether that is fundamentally flawed and it cannot be skipped.