[RN] experiment to move Fabric completeWork to the commit phase #30513
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Comparing: 87b1a94...c518910 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
facebook-github-bot
added
the
React Core Team
kassens
changed the title
kassens
changed the title
josephsavona
approved these changes
Jul 29, 2024
There was a problem hiding this comment.
This is intended for quick experimentation to see if this causes obvious issues, so let's ship and get data to decide next steps. If this works then we can explore the approach @sebmarkbage suggested of potentially splitting up the work of completeRoot() with layout happening in finalizeContainerChildren() and actual committing the tree in replaceContainerChildren().
But we should also consider whether we even need to render and layout the fallback if we're never going to show it, it seems like we may not need that anymore in this case.
github-actions bot
pushed a commit
that referenced
this pull request
Jul 29, 2024
There is currently a mismatch in how the persistent mode JS API and the Fabric native code interpret `completeRoot`. This is a short-lived experiment to see the effect of moving the Fabric `completeRoot` call from `finalizeContainerChildren` to `replaceContainerChildren` which in some cases does not get called. DiffTrain build for commit 6b82f3c.
josephsavona
added a commit
to josephsavona/react
that referenced
this pull request
Jul 30, 2024
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
added a commit
to josephsavona/react
that referenced
this pull request
Jul 30, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is currently a mismatch in how the persistent mode JS API and the Fabric native code interpret
completeRoot.This is a short-lived experiment to see the effect of moving the Fabric
completeRootcall fromfinalizeContainerChildrentoreplaceContainerChildrenwhich in some cases does not get called.