-
Notifications
You must be signed in to change notification settings - Fork 47.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't prerender siblings of suspended component #26380
Conversation
Comparing: 77ba161...11133b8 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
|
6175521
to
5f4bfbc
Compare
15070a9
to
9fd1afc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some questions mostly for my edification
// TODO: Once we stop prerendering siblings, instead of resetting the parent | ||
// of the node being unwound, we should be able to reset node itself as we | ||
// unwind the stack. Saves an additional null check. | ||
const returnFiber = incompleteWork.return; | ||
if (returnFiber !== null) { | ||
// Mark the parent fiber as incomplete and clear its subtree flags. | ||
// TODO: Once we stop prerendering siblings, we may be able to get rid of | ||
// the Incomplete flag because unwinding to the nearest boundary will | ||
// happen synchronously. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean once we remove this even from the legacy suspense boundary path? My understanding was that this PR implements this already
@@ -739,7 +737,7 @@ function completeDehydratedSuspenseBoundary( | |||
) { | |||
warnIfUnhydratedTailNodes(workInProgress); | |||
resetHydrationState(); | |||
workInProgress.flags |= ForceClientRender | Incomplete | ShouldCapture; | |||
workInProgress.flags |= ForceClientRender | DidCapture; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this was required because of the dev warning about incomplete fibers on the completwork path but I'm trying to wrap my head around the semantics of ShouldCapture -> DidCapture. Like I get that the ForceClientRender flag is what now triggers the client fallback render (which sorta seems like what it always should have been) but does marking DidCapture change any of the work loop behaviors? I haven't connected it to the rest of the changes in my mental model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should have been DidCapture
all along, it just happened to work the other way, too, though it was doing some redundant work.
I can't actually remember the reason they were ever separate but now that we always immediately unwind to the boundary after something throws, we might not need either of them...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted this part to its own PR with a test demonstrating the change: #26445
fd61a85
to
73304e9
Compare
fe0fc67
to
0135c6d
Compare
) (This was reviewed and approved as part of #26380; I'm extracting it into its own PR so that it can bisected later if it causes an issue.) I noticed while working on a PR that when an error happens during hydration, and we revert to client rendering, React actually does _two_ additional render passes instead of just one. We didn't notice it earlier because none of our tests happened to assert on how many renders it took to recover, only on the final output. It's possible this extra render pass had other consequences that I'm not aware of, like messing with some assumption in the recoverable errors logic. This adds a test to demonstrate the issue. (One problem is that we don't have much test coverage of this scenario in the first place, which likely would have caught this earlier.)
A mini-refactor to split completeUnitOfWork into two functions: completeUnitOfWork and unwindUnitOfWork. The existing function is already almost complete forked. I think splitting them up makes sense because it makes it easier to specialize the behavior. My practical motivation is that I'm going to change the "unwind" phase to synchronously unwind to the nearest Suspense/error boundary. This means we'll no longer prerender the siblings of a suspended tree. I'll address this in a subsequent step.
Today if something suspends, React will continue rendering the siblings of that component. Our original rationale for prerendering the siblings of a suspended component was to initiate any lazy fetches that they might contain. This was when we were more bullish about lazy fetching being a good idea some of the time (when combined with prefetching), as opposed to our latest thinking, which is that it's almost always a bad idea. Another rationale for the original behavior was that the render was I/O bound, anyway, so we might as do some extra work in the meantime. But this was before we had the concept of instant loading states: when navigating to a new screen, it's better to show a loading state as soon as you can (often a skeleton UI), rather than delay the transition. (There are still cases where we block the render, when a suitable loading state is not available; it's just not _all_ cases where something suspends.) So the biggest issue with our existing implementation is that the prerendering of the siblings happens within the same render pass as the one that suspended — _before_ the loading state appears. What we should do instead is immediately unwind the stack as soon as something suspends, to unblock the loading state. If we want to preserve the ability to prerender the siblings, what we could do is schedule special render pass immediately after the fallback is displayed. This is likely what we'll do in the future. However, in the new implementation of `use`, there's another reason we don't prerender siblings: so we can preserve the state of the stack when something suspends, and resume where we left of when the promise resolves without replaying the parents. The only way to do this currently is to suspend the entire work loop. Fiber does not currently support rendering multiple siblings in "parallel". Once you move onto the next sibling, the stack of the previous sibling is discarded and cannot be restored. We do plan to implement this feature, but it will require a not-insignificant refactor. Given that lazy data fetching is already bad for performance, the best trade off for now seems to be to disable prerendering of siblings. This gives us the best performance characteristics when you're following best practices (i.e. hoist data fetches to Server Components or route loaders), at the expense of an already bad pattern a bit worse. Later, when we implement resumable context stacks, we can reenable sibling prerendering. Though even then the use case will mostly be to prerender the CPU-bound work, not lazy fetches.
0135c6d
to
11133b8
Compare
) (This was reviewed and approved as part of #26380; I'm extracting it into its own PR so that it can bisected later if it causes an issue.) I noticed while working on a PR that when an error happens during hydration, and we revert to client rendering, React actually does _two_ additional render passes instead of just one. We didn't notice it earlier because none of our tests happened to assert on how many renders it took to recover, only on the final output. It's possible this extra render pass had other consequences that I'm not aware of, like messing with some assumption in the recoverable errors logic. This adds a test to demonstrate the issue. (One problem is that we don't have much test coverage of this scenario in the first place, which likely would have caught this earlier.) DiffTrain build for [77ba161](77ba161)
Today if something suspends, React will continue rendering the siblings of that component. Our original rationale for prerendering the siblings of a suspended component was to initiate any lazy fetches that they might contain. This was when we were more bullish about lazy fetching being a good idea some of the time (when combined with prefetching), as opposed to our latest thinking, which is that it's almost always a bad idea. Another rationale for the original behavior was that the render was I/O bound, anyway, so we might as do some extra work in the meantime. But this was before we had the concept of instant loading states: when navigating to a new screen, it's better to show a loading state as soon as you can (often a skeleton UI), rather than delay the transition. (There are still cases where we block the render, when a suitable loading state is not available; it's just not _all_ cases where something suspends.) So the biggest issue with our existing implementation is that the prerendering of the siblings happens within the same render pass as the one that suspended — _before_ the loading state appears. What we should do instead is immediately unwind the stack as soon as something suspends, to unblock the loading state. If we want to preserve the ability to prerender the siblings, what we could do is schedule special render pass immediately after the fallback is displayed. This is likely what we'll do in the future. However, in the new implementation of `use`, there's another reason we don't prerender siblings: so we can preserve the state of the stack when something suspends, and resume where we left of when the promise resolves without replaying the parents. The only way to do this currently is to suspend the entire work loop. Fiber does not currently support rendering multiple siblings in "parallel". Once you move onto the next sibling, the stack of the previous sibling is discarded and cannot be restored. We do plan to implement this feature, but it will require a not-insignificant refactor. Given that lazy data fetching is already bad for performance, the best trade off for now seems to be to disable prerendering of siblings. This gives us the best performance characteristics when you're following best practices (i.e. hoist data fetches to Server Components or route loaders), at the expense of making an already bad pattern a bit worse. Later, when we implement resumable context stacks, we can reenable sibling prerendering. Though even then the use case will mostly be to prerender the CPU-bound work, not lazy fetches. DiffTrain build for [12a1d14](12a1d14)
Summary: This sync includes the following changes: - **[ca01f359b](facebook/react@ca01f359b )**: Remove skipUnmountedBoundaries ([#26489](facebook/react#26489)) //<Ricky>// - **[43a70a610](facebook/react@43a70a610 )**: Limit the meaning of "custom element" to not include `is` ([#26524](facebook/react#26524)) //<Sebastian Markbåge>// - **[1308e49a6](facebook/react@1308e49a6 )**: [Flight Plugin] Scan for "use client" ([#26474](facebook/react#26474)) //<dan>// - **[1a1d61fed](facebook/react@1a1d61fed )**: Warn for ARIA typos on custom elements ([#26523](facebook/react#26523)) //<Sebastian Markbåge>// - **[73deff0d5](facebook/react@73deff0d5 )**: Refactor DOMProperty and CSSProperty ([#26513](facebook/react#26513)) //<Sebastian Markbåge>// - **[2d51251e6](facebook/react@2d51251e6 )**: Clean up deferRenderPhaseUpdateToNextBatch ([#26511](facebook/react#26511)) //<Andrew Clark>// - **[0ffc7f632](facebook/react@0ffc7f632 )**: Update useMemoCache test to confirm that cache persists across errors ([#26510](facebook/react#26510)) //<Joseph Savona>// - **[29a3be78b](facebook/react@29a3be78b )**: Move ReactDOMFloat to react-dom/src/ ([#26514](facebook/react#26514)) //<Sebastian Markbåge>// - **[4c2fc0190](facebook/react@4c2fc0190 )**: Generate safe javascript url instead of throwing with disableJavaScriptURLs is on ([#26507](facebook/react#26507)) //<Sebastian Markbåge>// - **[f0aafa1a7](facebook/react@f0aafa1a7 )**: Convert a few more tests to waitFor test helpers ([#26509](facebook/react#26509)) //<Andrew Clark>// - **[90995ef8b](facebook/react@90995ef8b )**: Delete "triangle" resuming fuzz tester ([#26508](facebook/react#26508)) //<Andrew Clark>// - **[f118b7ceb](facebook/react@f118b7ceb )**: [Flight] Gated test for dropped transport of undefined object values ([#26478](facebook/react#26478)) //<Sebastian Silbermann>// - **[fd0511c72](facebook/react@fd0511c72 )**: [Flight] Add support BigInt support ([#26479](facebook/react#26479)) //<Sebastian Silbermann>// - **[85de6fde5](facebook/react@85de6fde5 )**: Refactor DOM special cases per tags including controlled fields ([#26501](facebook/react#26501)) //<Sebastian Markbåge>// - **[1f5cdf8c7](facebook/react@1f5cdf8c7 )**: Update Suspense fuzz tests to use `act` ([#26498](facebook/react#26498)) //<Andrew Clark>// - **[f62cb39ee](facebook/react@f62cb39ee )**: Make disableSchedulerTimeoutInWorkLoop a static ff ([#26497](facebook/react#26497)) //<Ricky>// - **[41b4714f1](facebook/react@41b4714f1 )**: Remove disableNativeComponentFrames ([#26490](facebook/react#26490)) //<Ricky>// - **[fc90eb636](facebook/react@fc90eb636 )**: Codemod more tests to waitFor pattern ([#26494](facebook/react#26494)) //<Andrew Clark>// - **[e0bbc2662](facebook/react@e0bbc2662 )**: Improve tests that deal with microtasks ([#26493](facebook/react#26493)) //<Andrew Clark>// - **[8faf75193](facebook/react@8faf75193 )**: Codemod some expiration tests to waitForExpired ([#26491](facebook/react#26491)) //<Andrew Clark>// - **[8342a0992](facebook/react@8342a0992 )**: Remove unused feature flag disableSchedulerTimeoutBasedOnReactExpirationTime ([#26488](facebook/react#26488)) //<Jan Kassens>// - **[afea1d0c5](facebook/react@afea1d0c5 )**: [flow] make Flow suppressions explicit on the error ([#26487](facebook/react#26487)) //<Jan Kassens>// - **[768f965de](facebook/react@768f965de )**: Suspensily committing a prerendered tree ([#26434](facebook/react#26434)) //<Andrew Clark>// - **[d12bdcda6](facebook/react@d12bdcda6 )**: Fix Flow types of useEffectEvent ([#26468](facebook/react#26468)) //<Sebastian Silbermann>// - **[73b6435ca](facebook/react@73b6435ca )**: [Float][Fiber] Implement waitForCommitToBeReady for stylesheet resources ([#26450](facebook/react#26450)) //<Josh Story>// - **[175962c10](facebook/react@175962c10 )**: Fix remaining CommonJS imports after Rollup upgrade ([#26473](facebook/react#26473)) //<dan>// - **[909c6dacf](facebook/react@909c6dacf )**: Update Rollup to 3.x ([#26442](facebook/react#26442)) //<Mark Erikson>// - **[9c54b29b4](facebook/react@9c54b29b4 )**: Remove ReactFabricPublicInstance and used definition from ReactNativePrivateInterface ([#26437](facebook/react#26437)) //<Rubén Norte>// - **[f77099b6f](facebook/react@f77099b6f )**: Remove layout effect warning on the server ([#26395](facebook/react#26395)) //<Ricky>// - **[51a7c45f8](facebook/react@51a7c45f8 )**: Bugfix: SuspenseList incorrectly forces a fallback ([#26453](facebook/react#26453)) //<Andrew Clark>// - **[afb3d51dc](facebook/react@afb3d51dc )**: Fix enableClientRenderFallbackOnTextMismatch flag ([#26457](facebook/react#26457)) //<Sebastian Markbåge>// - **[8e17bfd14](facebook/react@8e17bfd14 )**: Make InternalInstanceHandle type opaque in ReactNativeTypes ([#26461](facebook/react#26461)) //<Rubén Norte>// - **[b93b4f074](facebook/react@b93b4f074 )**: Should not throw for children of iframe or object ([#26458](facebook/react#26458)) //<Sebastian Markbåge>// - **[c0b34bc5f](facebook/react@c0b34bc5f )**: chore: update links of docs and api ([#26455](facebook/react#26455)) //<Leedom>// - **[ffb6733ee](facebook/react@ffb6733ee )**: fix docs link for useSyncExternalStore ([#26452](facebook/react#26452)) //<Valor(华洛)>// - **[12a1d140e](facebook/react@12a1d140e )**: Don't prerender siblings of suspended component ([#26380](facebook/react#26380)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 77ba161...ca01f35 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44669450 fbshipit-source-id: f160aad4719a00df3ceeca78d5f3fcd0aa0f8437
Summary: This sync includes the following changes: - **[ca01f359b](facebook/react@ca01f359b )**: Remove skipUnmountedBoundaries ([facebook#26489](facebook/react#26489)) //<Ricky>// - **[43a70a610](facebook/react@43a70a610 )**: Limit the meaning of "custom element" to not include `is` ([facebook#26524](facebook/react#26524)) //<Sebastian Markbåge>// - **[1308e49a6](facebook/react@1308e49a6 )**: [Flight Plugin] Scan for "use client" ([facebook#26474](facebook/react#26474)) //<dan>// - **[1a1d61fed](facebook/react@1a1d61fed )**: Warn for ARIA typos on custom elements ([facebook#26523](facebook/react#26523)) //<Sebastian Markbåge>// - **[73deff0d5](facebook/react@73deff0d5 )**: Refactor DOMProperty and CSSProperty ([facebook#26513](facebook/react#26513)) //<Sebastian Markbåge>// - **[2d51251e6](facebook/react@2d51251e6 )**: Clean up deferRenderPhaseUpdateToNextBatch ([facebook#26511](facebook/react#26511)) //<Andrew Clark>// - **[0ffc7f632](facebook/react@0ffc7f632 )**: Update useMemoCache test to confirm that cache persists across errors ([facebook#26510](facebook/react#26510)) //<Joseph Savona>// - **[29a3be78b](facebook/react@29a3be78b )**: Move ReactDOMFloat to react-dom/src/ ([facebook#26514](facebook/react#26514)) //<Sebastian Markbåge>// - **[4c2fc0190](facebook/react@4c2fc0190 )**: Generate safe javascript url instead of throwing with disableJavaScriptURLs is on ([facebook#26507](facebook/react#26507)) //<Sebastian Markbåge>// - **[f0aafa1a7](facebook/react@f0aafa1a7 )**: Convert a few more tests to waitFor test helpers ([facebook#26509](facebook/react#26509)) //<Andrew Clark>// - **[90995ef8b](facebook/react@90995ef8b )**: Delete "triangle" resuming fuzz tester ([facebook#26508](facebook/react#26508)) //<Andrew Clark>// - **[f118b7ceb](facebook/react@f118b7ceb )**: [Flight] Gated test for dropped transport of undefined object values ([facebook#26478](facebook/react#26478)) //<Sebastian Silbermann>// - **[fd0511c72](facebook/react@fd0511c72 )**: [Flight] Add support BigInt support ([facebook#26479](facebook/react#26479)) //<Sebastian Silbermann>// - **[85de6fde5](facebook/react@85de6fde5 )**: Refactor DOM special cases per tags including controlled fields ([facebook#26501](facebook/react#26501)) //<Sebastian Markbåge>// - **[1f5cdf8c7](facebook/react@1f5cdf8c7 )**: Update Suspense fuzz tests to use `act` ([facebook#26498](facebook/react#26498)) //<Andrew Clark>// - **[f62cb39ee](facebook/react@f62cb39ee )**: Make disableSchedulerTimeoutInWorkLoop a static ff ([facebook#26497](facebook/react#26497)) //<Ricky>// - **[41b4714f1](facebook/react@41b4714f1 )**: Remove disableNativeComponentFrames ([facebook#26490](facebook/react#26490)) //<Ricky>// - **[fc90eb636](facebook/react@fc90eb636 )**: Codemod more tests to waitFor pattern ([facebook#26494](facebook/react#26494)) //<Andrew Clark>// - **[e0bbc2662](facebook/react@e0bbc2662 )**: Improve tests that deal with microtasks ([facebook#26493](facebook/react#26493)) //<Andrew Clark>// - **[8faf75193](facebook/react@8faf75193 )**: Codemod some expiration tests to waitForExpired ([facebook#26491](facebook/react#26491)) //<Andrew Clark>// - **[8342a0992](facebook/react@8342a0992 )**: Remove unused feature flag disableSchedulerTimeoutBasedOnReactExpirationTime ([facebook#26488](facebook/react#26488)) //<Jan Kassens>// - **[afea1d0c5](facebook/react@afea1d0c5 )**: [flow] make Flow suppressions explicit on the error ([facebook#26487](facebook/react#26487)) //<Jan Kassens>// - **[768f965de](facebook/react@768f965de )**: Suspensily committing a prerendered tree ([facebook#26434](facebook/react#26434)) //<Andrew Clark>// - **[d12bdcda6](facebook/react@d12bdcda6 )**: Fix Flow types of useEffectEvent ([facebook#26468](facebook/react#26468)) //<Sebastian Silbermann>// - **[73b6435ca](facebook/react@73b6435ca )**: [Float][Fiber] Implement waitForCommitToBeReady for stylesheet resources ([facebook#26450](facebook/react#26450)) //<Josh Story>// - **[175962c10](facebook/react@175962c10 )**: Fix remaining CommonJS imports after Rollup upgrade ([facebook#26473](facebook/react#26473)) //<dan>// - **[909c6dacf](facebook/react@909c6dacf )**: Update Rollup to 3.x ([facebook#26442](facebook/react#26442)) //<Mark Erikson>// - **[9c54b29b4](facebook/react@9c54b29b4 )**: Remove ReactFabricPublicInstance and used definition from ReactNativePrivateInterface ([facebook#26437](facebook/react#26437)) //<Rubén Norte>// - **[f77099b6f](facebook/react@f77099b6f )**: Remove layout effect warning on the server ([facebook#26395](facebook/react#26395)) //<Ricky>// - **[51a7c45f8](facebook/react@51a7c45f8 )**: Bugfix: SuspenseList incorrectly forces a fallback ([facebook#26453](facebook/react#26453)) //<Andrew Clark>// - **[afb3d51dc](facebook/react@afb3d51dc )**: Fix enableClientRenderFallbackOnTextMismatch flag ([facebook#26457](facebook/react#26457)) //<Sebastian Markbåge>// - **[8e17bfd14](facebook/react@8e17bfd14 )**: Make InternalInstanceHandle type opaque in ReactNativeTypes ([facebook#26461](facebook/react#26461)) //<Rubén Norte>// - **[b93b4f074](facebook/react@b93b4f074 )**: Should not throw for children of iframe or object ([facebook#26458](facebook/react#26458)) //<Sebastian Markbåge>// - **[c0b34bc5f](facebook/react@c0b34bc5f )**: chore: update links of docs and api ([facebook#26455](facebook/react#26455)) //<Leedom>// - **[ffb6733ee](facebook/react@ffb6733ee )**: fix docs link for useSyncExternalStore ([facebook#26452](facebook/react#26452)) //<Valor(华洛)>// - **[12a1d140e](facebook/react@12a1d140e )**: Don't prerender siblings of suspended component ([facebook#26380](facebook/react#26380)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 77ba161...ca01f35 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44669450 fbshipit-source-id: f160aad4719a00df3ceeca78d5f3fcd0aa0f8437
Summary: This sync includes the following changes: - **[ca01f359b](facebook/react@ca01f359b )**: Remove skipUnmountedBoundaries ([facebook#26489](facebook/react#26489)) //<Ricky>// - **[43a70a610](facebook/react@43a70a610 )**: Limit the meaning of "custom element" to not include `is` ([facebook#26524](facebook/react#26524)) //<Sebastian Markbåge>// - **[1308e49a6](facebook/react@1308e49a6 )**: [Flight Plugin] Scan for "use client" ([facebook#26474](facebook/react#26474)) //<dan>// - **[1a1d61fed](facebook/react@1a1d61fed )**: Warn for ARIA typos on custom elements ([facebook#26523](facebook/react#26523)) //<Sebastian Markbåge>// - **[73deff0d5](facebook/react@73deff0d5 )**: Refactor DOMProperty and CSSProperty ([facebook#26513](facebook/react#26513)) //<Sebastian Markbåge>// - **[2d51251e6](facebook/react@2d51251e6 )**: Clean up deferRenderPhaseUpdateToNextBatch ([facebook#26511](facebook/react#26511)) //<Andrew Clark>// - **[0ffc7f632](facebook/react@0ffc7f632 )**: Update useMemoCache test to confirm that cache persists across errors ([facebook#26510](facebook/react#26510)) //<Joseph Savona>// - **[29a3be78b](facebook/react@29a3be78b )**: Move ReactDOMFloat to react-dom/src/ ([facebook#26514](facebook/react#26514)) //<Sebastian Markbåge>// - **[4c2fc0190](facebook/react@4c2fc0190 )**: Generate safe javascript url instead of throwing with disableJavaScriptURLs is on ([facebook#26507](facebook/react#26507)) //<Sebastian Markbåge>// - **[f0aafa1a7](facebook/react@f0aafa1a7 )**: Convert a few more tests to waitFor test helpers ([facebook#26509](facebook/react#26509)) //<Andrew Clark>// - **[90995ef8b](facebook/react@90995ef8b )**: Delete "triangle" resuming fuzz tester ([facebook#26508](facebook/react#26508)) //<Andrew Clark>// - **[f118b7ceb](facebook/react@f118b7ceb )**: [Flight] Gated test for dropped transport of undefined object values ([facebook#26478](facebook/react#26478)) //<Sebastian Silbermann>// - **[fd0511c72](facebook/react@fd0511c72 )**: [Flight] Add support BigInt support ([facebook#26479](facebook/react#26479)) //<Sebastian Silbermann>// - **[85de6fde5](facebook/react@85de6fde5 )**: Refactor DOM special cases per tags including controlled fields ([facebook#26501](facebook/react#26501)) //<Sebastian Markbåge>// - **[1f5cdf8c7](facebook/react@1f5cdf8c7 )**: Update Suspense fuzz tests to use `act` ([facebook#26498](facebook/react#26498)) //<Andrew Clark>// - **[f62cb39ee](facebook/react@f62cb39ee )**: Make disableSchedulerTimeoutInWorkLoop a static ff ([facebook#26497](facebook/react#26497)) //<Ricky>// - **[41b4714f1](facebook/react@41b4714f1 )**: Remove disableNativeComponentFrames ([facebook#26490](facebook/react#26490)) //<Ricky>// - **[fc90eb636](facebook/react@fc90eb636 )**: Codemod more tests to waitFor pattern ([facebook#26494](facebook/react#26494)) //<Andrew Clark>// - **[e0bbc2662](facebook/react@e0bbc2662 )**: Improve tests that deal with microtasks ([facebook#26493](facebook/react#26493)) //<Andrew Clark>// - **[8faf75193](facebook/react@8faf75193 )**: Codemod some expiration tests to waitForExpired ([facebook#26491](facebook/react#26491)) //<Andrew Clark>// - **[8342a0992](facebook/react@8342a0992 )**: Remove unused feature flag disableSchedulerTimeoutBasedOnReactExpirationTime ([facebook#26488](facebook/react#26488)) //<Jan Kassens>// - **[afea1d0c5](facebook/react@afea1d0c5 )**: [flow] make Flow suppressions explicit on the error ([facebook#26487](facebook/react#26487)) //<Jan Kassens>// - **[768f965de](facebook/react@768f965de )**: Suspensily committing a prerendered tree ([facebook#26434](facebook/react#26434)) //<Andrew Clark>// - **[d12bdcda6](facebook/react@d12bdcda6 )**: Fix Flow types of useEffectEvent ([facebook#26468](facebook/react#26468)) //<Sebastian Silbermann>// - **[73b6435ca](facebook/react@73b6435ca )**: [Float][Fiber] Implement waitForCommitToBeReady for stylesheet resources ([facebook#26450](facebook/react#26450)) //<Josh Story>// - **[175962c10](facebook/react@175962c10 )**: Fix remaining CommonJS imports after Rollup upgrade ([facebook#26473](facebook/react#26473)) //<dan>// - **[909c6dacf](facebook/react@909c6dacf )**: Update Rollup to 3.x ([facebook#26442](facebook/react#26442)) //<Mark Erikson>// - **[9c54b29b4](facebook/react@9c54b29b4 )**: Remove ReactFabricPublicInstance and used definition from ReactNativePrivateInterface ([facebook#26437](facebook/react#26437)) //<Rubén Norte>// - **[f77099b6f](facebook/react@f77099b6f )**: Remove layout effect warning on the server ([facebook#26395](facebook/react#26395)) //<Ricky>// - **[51a7c45f8](facebook/react@51a7c45f8 )**: Bugfix: SuspenseList incorrectly forces a fallback ([facebook#26453](facebook/react#26453)) //<Andrew Clark>// - **[afb3d51dc](facebook/react@afb3d51dc )**: Fix enableClientRenderFallbackOnTextMismatch flag ([facebook#26457](facebook/react#26457)) //<Sebastian Markbåge>// - **[8e17bfd14](facebook/react@8e17bfd14 )**: Make InternalInstanceHandle type opaque in ReactNativeTypes ([facebook#26461](facebook/react#26461)) //<Rubén Norte>// - **[b93b4f074](facebook/react@b93b4f074 )**: Should not throw for children of iframe or object ([facebook#26458](facebook/react#26458)) //<Sebastian Markbåge>// - **[c0b34bc5f](facebook/react@c0b34bc5f )**: chore: update links of docs and api ([facebook#26455](facebook/react#26455)) //<Leedom>// - **[ffb6733ee](facebook/react@ffb6733ee )**: fix docs link for useSyncExternalStore ([facebook#26452](facebook/react#26452)) //<Valor(华洛)>// - **[12a1d140e](facebook/react@12a1d140e )**: Don't prerender siblings of suspended component ([facebook#26380](facebook/react#26380)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 77ba161...ca01f35 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44669450 fbshipit-source-id: f160aad4719a00df3ceeca78d5f3fcd0aa0f8437
…28299) While investigating #28285 I found a possible bug in handling Suspense and mismatches. As the tests show, if the first sibling in a boundary suspends, and the second has a mismatch, we will NOT show a fallback. If the first sibling is a mismatch, and the second sibling suspends, we WILL show a fallback. [Here's a stackbliz showing the behavior on Canary](https://stackblitz.com/edit/stackblitz-starters-bh3snf?file=src%2Fstyle.css,public%2Findex.html,src%2Findex.tsx). This breakage was introduced by: #26380. Before this PR, we would not show a fallback in either case. That PR makes it so that we don't pre-render siblings of suspended trees, so presumably, whatever detection we had to avoid fallbacks on mismatches, requires knowing there's a mismatch in the tree when we suspend.
…acebook#28299) While investigating facebook#28285 I found a possible bug in handling Suspense and mismatches. As the tests show, if the first sibling in a boundary suspends, and the second has a mismatch, we will NOT show a fallback. If the first sibling is a mismatch, and the second sibling suspends, we WILL show a fallback. [Here's a stackbliz showing the behavior on Canary](https://stackblitz.com/edit/stackblitz-starters-bh3snf?file=src%2Fstyle.css,public%2Findex.html,src%2Findex.tsx). This breakage was introduced by: facebook#26380. Before this PR, we would not show a fallback in either case. That PR makes it so that we don't pre-render siblings of suspended trees, so presumably, whatever detection we had to avoid fallbacks on mismatches, requires knowing there's a mismatch in the tree when we suspend.
) (This was reviewed and approved as part of #26380; I'm extracting it into its own PR so that it can bisected later if it causes an issue.) I noticed while working on a PR that when an error happens during hydration, and we revert to client rendering, React actually does _two_ additional render passes instead of just one. We didn't notice it earlier because none of our tests happened to assert on how many renders it took to recover, only on the final output. It's possible this extra render pass had other consequences that I'm not aware of, like messing with some assumption in the recoverable errors logic. This adds a test to demonstrate the issue. (One problem is that we don't have much test coverage of this scenario in the first place, which likely would have caught this earlier.) DiffTrain build for commit 77ba161.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My favorite way to do data fetching is as close to the place where I am using the data.
React made it possible up until this change. Now I am forced to define my data needs in a top level component 20 parents up from where it's actually being used. This change broke component encapsulation. The holy grail of react.js.
Also React.lazy.
Please revert this change or at least make it opt-in if you think there are users who need to render their components as serially loaded waterfalls.
@acdlite @eps1lon @gnoff is it possible to still talk about this? Making this a waterfall will have an impact that would put everything that relied on suspense at an immediate disadvantage. Not just on the dom (react-query et al), but also platforms like three (via react-three-fiber) are inherently async, everything is a fetch or an async worker/wasm. Userland work arounds like pre-cache don't apply to non-user facing async tasks and 3rd party modules. It can't be put into global space because it would break tree shaking. And like @capaj mentioned it breaks encapsulation. It would also diminish encapsulated async composition & interop, a major, ground breaking feature in current React. <Center>
<AsyncThingA />
<AsyncThingB />
</Center>
The loss of this capability cannot be understated. Please consider undoing or making the behaviour opt in/out. SuspenseList's <Suspense revealOrder={"forwards" | "backwards" | "together"} fallback={...}>
<Foo />
<Bar />
</Suspense> |
I have to agree with the previous commenters. This blocks one of the best React patterns - fetching the data in the places where it is used. We should really discuss this, or I'd like to hear a motivation for this change that takes into account patterns used by data fetching libraries like react-query. Moving the data fetching logic to top level components it's just not a pattern I can imagine getting onboard with :( |
Has there been any "real world" performance testing done on this, using current popular React libraries / demos / large scale apps? It seems like something that will overwhelmingly benefit things like Next.js while disadvantaging nearly every other React library & non-next.js app. Which, if that turns out to be the case in the testing, surely can't be acceptable? |
And where's the route loader for nextjs? Putting data fetches in server components means your data cannot change, which is fundamental dealbreaking in building useful internet-rich applications. If data is always static you might as well have web 1.0 interfaces. |
Hi! We started testing how this could affect our sites. We updated React and Next to Canary on a branch of https://kidsuper.world/, a site that uses many models and textures. First, we recorded the current approach; it takes about 2.5 seconds to load the .glb models: Then, we installed the canary version of react; Same .glb models, takes about 3.5 seconds to load: Links:Current deployment: https://kidsuper.world/ |
I agree that this should be an |
Please make this configurable at the very least. People (who do not need or want RSC or route loaders) will upgrade to React 19 without understanding this breaking change. They will not like the "fix" and they shouldn't have to. |
This, in its current state, breaks several React patterns including the ones enabled by things such as parallel data loading and TanStack Query. It should be configurable at the very least, and opt-in ideally. "Given that lazy data fetching is already bad for performance, [...] following best practices (i.e. hoist data fetches to Server Components or route loaders), [...]" also seems like an overly broad take, it may be bad on purely performance metrics but when implemented well can be great for UX, especially when the content you're loading is asset-type (like Matias's example) or extremely large (like datasets). Making this behaviour opt-in would allow the developers to pick the right strategy for what they aim to achieve. |
This change is to optimize for users with Slow 3G internet connection (include myself). Great move. Thanks. |
What if I want to prioritize one sibling over the suspended one? Where is the freedom to do so? PS: The use of seems to justify such a change feels a bit off. |
The whole idea of the React Eco system pushing or encouraging the use of SSR or Server is in my humble opinion not useful for majority of the Apps where the server-side Tech Stack is something other than JS/TS. We have our server side in Java, Python. Now, from architectural standpoint, if one wanted to go server side, Java provides, JSP, Thymeleaf, which is much easier to use. The major advantage is 2-fold:
So, I am not sure how this trend of coming full circle back to the old days of PHP/JSP kind of architecture is good for those app where the First Time Load performance is not the highest priority. |
@revskill10 If you used suspense (via react-query et al) in your generic React app this change will double, tripple, quadruple loading times, depending on the amount of your requests. It will water fall your task runners (fetches etc): one runs after the other until the UI is presented. A real world application was already posted here #26380 (comment) It just added a tame second, it could be way worse than that. But that second on a fast connection already would have meant 5-10 seconds more for kidsuper.world on your 3G. |
@drcmda No. My point is "stop initiating the fetch inside a component tree", that doesn't work on slow network. And it's the reason i don't use react-query. It's based on false asumption with fast network. |
Please don’t kill our nice SPA kit for Vercel. SSR sucks, it’s too complicated of an idea for it to ever be good and it reminds me of something Microsoft would try to foist on us like ASP.NET WebForms where they try to hide the minor complexity of client/server by constantly shuttling all of the server state around. |
It's important not to turn this into a discussion about the pros / cons of SSR, SSR is a nice feature (if you happen to have a tech stack that can use it) but it shouldn't come at the detriment of how the vast majority of currently deployed React apps operate. What I'd like to see come from this are:
|
I'm not good at programming my apologies
…On Fri, Jun 14, 2024, 5:22 AM Wayne Bloss ***@***.***> wrote:
Please don’t kill our nice SPA kit for Vercel. SSR sucks, it’s too
complicated of an idea for it to ever be good and it reminds me of
something Microsoft would try to foist on us like ASP.NET WebForms where
they try to hide the minor complexity of client/server by constantly
shuttling all of the server state around.
—
Reply to this email directly, view it on GitHub
<#26380 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BADUZUMVGEFOROY2BZIBU43ZHLOANAVCNFSM6AAAAABJHPGJDWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRXHA4TCOJRGM>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Today if something suspends, React will continue rendering the siblings of that component.
Our original rationale for prerendering the siblings of a suspended component was to initiate any lazy fetches that they might contain. This was when we were more bullish about lazy fetching being a good idea some of the time (when combined with prefetching), as opposed to our latest thinking, which is that it's almost always a bad idea.
Another rationale for the original behavior was that the render was I/O bound, anyway, so we might as do some extra work in the meantime. But this was before we had the concept of instant loading states: when navigating to a new screen, it's better to show a loading state as soon as you can (often a skeleton UI), rather than delay the transition. (There are still cases where we block the render, when a suitable loading state is not available; it's just not all cases where something suspends.) So the biggest issue with our existing implementation is that the prerendering of the siblings happens within the same render pass as the one that suspended — before the loading state appears.
What we should do instead is immediately unwind the stack as soon as something suspends, to unblock the loading state.
If we want to preserve the ability to prerender the siblings, what we could do is schedule special render pass immediately after the fallback is displayed. This is likely what we'll do in the future. However, in the new implementation of
use
, there's another reason we don't prerender siblings: so we can preserve the state of the stack when something suspends, and resume where we left of when the promise resolves without replaying the parents. The only way to do this currently is to suspend the entire work loop. Fiber does not currently support rendering multiple siblings in "parallel". Once you move onto the next sibling, the stack of the previous sibling is discarded and cannot be restored. We do plan to implement this feature, but it will require a not-insignificant refactor.Given that lazy data fetching is already bad for performance, the best trade off for now seems to be to disable prerendering of siblings. This gives us the best performance characteristics when you're following best practices (i.e. hoist data fetches to Server Components or route loaders), at the expense of making an already bad pattern a bit worse.
Later, when we implement resumable context stacks, we can reenable sibling prerendering. Though even then the use case will mostly be to prerender the CPU-bound work, not lazy fetches.