[Fizz] Fix root segment IDs #27371

sebmarkbage · 2023-09-14T02:38:00Z

Typically we assign IDs lazily when flushing to minimize the ids we have to assign and we try to maximize inlining.

When we prerender we could always flush into a buffer before returning but that's not actually what we do right now. We complete rendering before returning but we don't actually run the flush path until someone reads the resulting stream. We assign IDs eagerly when something postpone so that we can refer to those ids in the holes without flushing first.

This leads to some interesting conditions that needs to consider this.

This PR also deals with rootSegmentId which is the ID of the segment that contains the body of a Suspense boundary. The boundary needs to know this in addition to the Suspense Boundary's ID to know how to inject the root segment into the boundary.

Why is the suspense boundary ID and the rootSegmentID just not the same ID? Why don't segments and suspense boundary not share ID namespace? That's a good question and I don't really remember. It might not be a good reason and maybe they should just be the same.

react-sizebot · 2023-09-14T02:41:45Z

Comparing: 95c9554...f95256a

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	166.62 kB	166.62 kB	=	52.13 kB	52.13 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	176.05 kB	176.05 kB	=	54.97 kB	54.97 kB
facebook-www/ReactDOM-prod.classic.js	=	571.73 kB	571.73 kB	=	100.64 kB	100.64 kB
facebook-www/ReactDOM-prod.modern.js	=	555.46 kB	555.46 kB	=	97.75 kB	97.75 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/react-server/cjs/react-server.production.min.js	+0.38%	29.88 kB	30.00 kB	+0.32%	9.90 kB	9.93 kB
oss-experimental/react-server/cjs/react-server.development.js	+0.32%	169.38 kB	169.92 kB	+0.34%	41.60 kB	41.74 kB
oss-stable-semver/react-server/cjs/react-server.development.js	+0.29%	155.71 kB	156.16 kB	+0.35%	38.22 kB	38.35 kB
oss-stable/react-server/cjs/react-server.development.js	+0.29%	155.71 kB	156.16 kB	+0.35%	38.22 kB	38.35 kB
oss-stable-semver/react-server/cjs/react-server.production.min.js	+0.23%	27.41 kB	27.48 kB	+0.21%	9.17 kB	9.19 kB
oss-stable/react-server/cjs/react-server.production.min.js	+0.23%	27.41 kB	27.48 kB	+0.21%	9.17 kB	9.19 kB

Generated by 🚫 dangerJS against f95256a

Typically we assign IDs lazily when flushing to minimize the ids we have to assign and we try to maximize inlining. When we prerender we could always flush into a buffer before returning but that's not actually what we do right now. We complete rendering before returning but we don't actually run the flush path until someone reads the resulting stream. We assign IDs eagerly when something postpone so that we can refer to those ids in the holes without flushing first. This leads to some interesting conditions that needs to consider this. This PR also deals with rootSegmentId which is the ID of the segment that contains the body of a Suspense boundary. The boundary needs to know this in addition to the Suspense Boundary's ID to know how to inject the root segment into the boundary. Why is the suspense boundary ID and the rootSegmentID just not the same ID? Why don't segments and suspense boundary not share ID namespace? That's a good question and I don't really remember. It might not be a good reason and maybe they should just be the same. DiffTrain build for [a6e4791](a6e4791)

Typically we assign IDs lazily when flushing to minimize the ids we have to assign and we try to maximize inlining. When we prerender we could always flush into a buffer before returning but that's not actually what we do right now. We complete rendering before returning but we don't actually run the flush path until someone reads the resulting stream. We assign IDs eagerly when something postpone so that we can refer to those ids in the holes without flushing first. This leads to some interesting conditions that needs to consider this. This PR also deals with rootSegmentId which is the ID of the segment that contains the body of a Suspense boundary. The boundary needs to know this in addition to the Suspense Boundary's ID to know how to inject the root segment into the boundary. Why is the suspense boundary ID and the rootSegmentID just not the same ID? Why don't segments and suspense boundary not share ID namespace? That's a good question and I don't really remember. It might not be a good reason and maybe they should just be the same.

Typically we assign IDs lazily when flushing to minimize the ids we have to assign and we try to maximize inlining. When we prerender we could always flush into a buffer before returning but that's not actually what we do right now. We complete rendering before returning but we don't actually run the flush path until someone reads the resulting stream. We assign IDs eagerly when something postpone so that we can refer to those ids in the holes without flushing first. This leads to some interesting conditions that needs to consider this. This PR also deals with rootSegmentId which is the ID of the segment that contains the body of a Suspense boundary. The boundary needs to know this in addition to the Suspense Boundary's ID to know how to inject the root segment into the boundary. Why is the suspense boundary ID and the rootSegmentID just not the same ID? Why don't segments and suspense boundary not share ID namespace? That's a good question and I don't really remember. It might not be a good reason and maybe they should just be the same. DiffTrain build for commit a6e4791.

Fix segment IDs

f95256a

sebmarkbage requested a review from acdlite September 14, 2023 02:38

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 14, 2023

acdlite approved these changes Sep 14, 2023

View reviewed changes

sebmarkbage merged commit a6e4791 into facebook:main Sep 14, 2023
36 checks passed

acdlite mentioned this pull request Sep 14, 2023

Update React from dd480ef92 to d6dcad6a8. vercel/next.js#55399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fizz] Fix root segment IDs #27371

[Fizz] Fix root segment IDs #27371

sebmarkbage commented Sep 14, 2023

react-sizebot commented Sep 14, 2023

[Fizz] Fix root segment IDs #27371

[Fizz] Fix root segment IDs #27371

Conversation

sebmarkbage commented Sep 14, 2023

react-sizebot commented Sep 14, 2023

Critical size changes

Significant size changes