-
Notifications
You must be signed in to change notification settings - Fork 49k
[Fizz] Don't outline Boundaries that may contribute to the preamble #34058
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
Conversation
Suspense boundaries that may have contributed to the preamble should not be outlined due to size because these boundaries are only meant to be in fallback state if the boundary actually errors. This change excludes any boundary which has the potential to contribute to the preamble. We could alternatively track which boundaries actually contributed to the preamble but in practice there will be very few and I think this is sufficient. One problem with this approach is it makes Suspense above body opt out of the mode where we omit rel="expect" for large shells. In essence Suspense above body has the semantics of a Shell (it blocks flushing until resolved) but it doesn't get tracked as request bytes and thus we will not opt users into the skipped blocking shell for very large boundaries. This will be fixed in a followup
Comparing: 9784cb3...bad8142 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) |
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.
Pull Request Overview
This PR modifies the boundary outlining logic in React Server Components to prevent Suspense boundaries that may contribute to the preamble from being outlined based on size. The change ensures that these boundaries are only shown in fallback state when they actually error, rather than being outlined for performance reasons.
- Adds a check to prevent outlining boundaries with
contentPreamble === null
- Includes a comprehensive test to verify boundaries contributing to preamble are always flushed regardless of size
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/react-server/src/ReactFizzServer.js | Updates isEligibleForOutlining function to exclude boundaries that may contribute to preamble |
packages/react-dom/src/tests/ReactDOMFizzServer-test.js | Adds test case verifying large boundaries contributing to preamble are not outlined |
|
||
it('should always flush the boundaries contributing the preamble regardless of their size', async () => { | ||
const longDescription = | ||
`I need to make this segment somewhat large because it needs to be large enought to be outlined during the initial flush. Setting the progressive chunk size to near zero isn't enough because there is a fixed minimum size that we use to avoid doing the size tracking altogether and this needs to be larger than that at least. |
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.
There is a typo in the comment: 'enought' should be 'enough'.
`I need to make this segment somewhat large because it needs to be large enought to be outlined during the initial flush. Setting the progressive chunk size to near zero isn't enough because there is a fixed minimum size that we use to avoid doing the size tracking altogether and this needs to be larger than that at least. | |
`I need to make this segment somewhat large because it needs to be large enough to be outlined during the initial flush. Setting the progressive chunk size to near zero isn't enough because there is a fixed minimum size that we use to avoid doing the size tracking altogether and this needs to be larger than that at least. |
Copilot uses AI. Check for mistakes.
…34058) Suspense boundaries that may have contributed to the preamble should not be outlined due to size because these boundaries are only meant to be in fallback state if the boundary actually errors. This change excludes any boundary which has the potential to contribute to the preamble. We could alternatively track which boundaries actually contributed to the preamble but in practice there will be very few and I think this is sufficient. One problem with this approach is it makes Suspense above body opt out of the mode where we omit rel="expect" for large shells. In essence Suspense above body has the semantics of a Shell (it blocks flushing until resolved) but it doesn't get tracked as request bytes and thus we will not opt users into the skipped blocking shell for very large boundaries. This will be fixed in a followup DiffTrain build for [9877346](9877346)
…e request byteSize (#34059) Stacked on #34058 When tracking how large the shell is we currently only track the bytes of everything above Suspense boundaries. However since Boundaries that contribute to the preamble will always be inlined when the shell flushes they should also be considered as part of the request byteSize since they always flush alongside the shell. This change adds this tracking
…e request byteSize (#34059) Stacked on #34058 When tracking how large the shell is we currently only track the bytes of everything above Suspense boundaries. However since Boundaries that contribute to the preamble will always be inlined when the shell flushes they should also be considered as part of the request byteSize since they always flush alongside the shell. This change adds this tracking DiffTrain build for [8de7aed](8de7aed)
…e request byteSize (facebook#34059) Stacked on facebook#34058 When tracking how large the shell is we currently only track the bytes of everything above Suspense boundaries. However since Boundaries that contribute to the preamble will always be inlined when the shell flushes they should also be considered as part of the request byteSize since they always flush alongside the shell. This change adds this tracking DiffTrain build for [8de7aed](facebook@8de7aed)
…e request byteSize (facebook#34059) Stacked on facebook#34058 When tracking how large the shell is we currently only track the bytes of everything above Suspense boundaries. However since Boundaries that contribute to the preamble will always be inlined when the shell flushes they should also be considered as part of the request byteSize since they always flush alongside the shell. This change adds this tracking DiffTrain build for [8de7aed](facebook@8de7aed)
…e request byteSize (facebook#34059) Stacked on facebook#34058 When tracking how large the shell is we currently only track the bytes of everything above Suspense boundaries. However since Boundaries that contribute to the preamble will always be inlined when the shell flushes they should also be considered as part of the request byteSize since they always flush alongside the shell. This change adds this tracking DiffTrain build for [8de7aed](facebook@8de7aed)
…e request byteSize (facebook#34059) Stacked on facebook#34058 When tracking how large the shell is we currently only track the bytes of everything above Suspense boundaries. However since Boundaries that contribute to the preamble will always be inlined when the shell flushes they should also be considered as part of the request byteSize since they always flush alongside the shell. This change adds this tracking DiffTrain build for [8de7aed](facebook@8de7aed)
Suspense boundaries that may have contributed to the preamble should not be outlined due to size because these boundaries are only meant to be in fallback state if the boundary actually errors. This change excludes any boundary which has the potential to contribute to the preamble. We could alternatively track which boundaries actually contributed to the preamble but in practice there will be very few and I think this is sufficient.
One problem with this approach is it makes Suspense above body opt out of the mode where we omit rel="expect" for large shells. In essence Suspense above body has the semantics of a Shell (it blocks flushing until resolved) but it doesn't get tracked as request bytes and thus we will not opt users into the skipped blocking shell for very large boundaries.
This will be fixed in a followup