Add cacheNodeSeedData to RSC payload #58566
Merged
Conversation
This file contains 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
Tests Passed |
Stats from current PRDefault BuildGeneral Overall increase
|
| vercel/next.js canary | acdlite/next.js add-cachenodeseeddata-to-rsc-payload | Change | |
|---|---|---|---|
| buildDuration | 10.3s | 10.2s | N/A |
| buildDurationCached | 6s | 6.5s | |
| nodeModulesSize | 199 MB | 199 MB | |
| nextStartRea..uration (ms) | 416ms | 414ms | N/A |
Client Bundles (main, webpack)
| vercel/next.js canary | acdlite/next.js add-cachenodeseeddata-to-rsc-payload | Change | |
|---|---|---|---|
| 199-HASH.js gzip | 28.8 kB | 28.8 kB | N/A |
| 3f784ff6-HASH.js gzip | 53.3 kB | 53.3 kB | ✓ |
| 494.HASH.js gzip | 180 B | 181 B | N/A |
| framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
| main-app-HASH.js gzip | 241 B | 239 B | N/A |
| main-HASH.js gzip | 31.7 kB | 31.8 kB | N/A |
| webpack-HASH.js gzip | 1.7 kB | 1.7 kB | ✓ |
| Overall change | 100 kB | 100 kB | ✓ |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | acdlite/next.js add-cachenodeseeddata-to-rsc-payload | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | acdlite/next.js add-cachenodeseeddata-to-rsc-payload | Change | |
|---|---|---|---|
| _app-HASH.js gzip | 194 B | 195 B | N/A |
| _error-HASH.js gzip | 182 B | 181 B | N/A |
| amp-HASH.js gzip | 504 B | 506 B | N/A |
| css-HASH.js gzip | 322 B | 323 B | N/A |
| dynamic-HASH.js gzip | 2.5 kB | 2.5 kB | ✓ |
| edge-ssr-HASH.js gzip | 253 B | 255 B | N/A |
| head-HASH.js gzip | 348 B | 347 B | N/A |
| hooks-HASH.js gzip | 369 B | 368 B | N/A |
| image-HASH.js gzip | 4.3 kB | 4.3 kB | N/A |
| index-HASH.js gzip | 256 B | 256 B | ✓ |
| link-HASH.js gzip | 2.63 kB | 2.63 kB | N/A |
| routerDirect..HASH.js gzip | 311 B | 311 B | ✓ |
| script-HASH.js gzip | 384 B | 383 B | N/A |
| withRouter-HASH.js gzip | 307 B | 308 B | N/A |
| 1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
| Overall change | 3.17 kB | 3.17 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | acdlite/next.js add-cachenodeseeddata-to-rsc-payload | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 484 B | 483 B | N/A |
| Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | acdlite/next.js add-cachenodeseeddata-to-rsc-payload | Change | |
|---|---|---|---|
| index.html gzip | 528 B | 527 B | N/A |
| link.html gzip | 539 B | 540 B | N/A |
| withRouter.html gzip | 525 B | 522 B | N/A |
| Overall change | 0 B | 0 B | ✓ |
Edge SSR bundle Size
| vercel/next.js canary | acdlite/next.js add-cachenodeseeddata-to-rsc-payload | Change | |
|---|---|---|---|
| edge-ssr.js gzip | 92.6 kB | 92.6 kB | N/A |
| page.js gzip | 145 kB | 145 kB | N/A |
| Overall change | 0 B | 0 B | ✓ |
Middleware size
| vercel/next.js canary | acdlite/next.js add-cachenodeseeddata-to-rsc-payload | Change | |
|---|---|---|---|
| middleware-b..fest.js gzip | 627 B | 628 B | N/A |
| middleware-r..fest.js gzip | 150 B | 151 B | N/A |
| middleware.js gzip | 24.8 kB | 24.8 kB | ✓ |
| edge-runtime..pack.js gzip | 1.92 kB | 1.92 kB | ✓ |
| Overall change | 26.7 kB | 26.7 kB | ✓ |
Next Runtimes
| vercel/next.js canary | acdlite/next.js add-cachenodeseeddata-to-rsc-payload | Change | |
|---|---|---|---|
| app-page-exp...dev.js gzip | 167 kB | 167 kB | N/A |
| app-page-exp..prod.js gzip | 93.4 kB | 93.4 kB | N/A |
| app-page-tur..prod.js gzip | 94.1 kB | 94.1 kB | N/A |
| app-page-tur..prod.js gzip | 88.7 kB | 88.7 kB | N/A |
| app-page.run...dev.js gzip | 137 kB | 137 kB | N/A |
| app-page.run..prod.js gzip | 88 kB | 88 kB | ✓ |
| app-route-ex...dev.js gzip | 23.8 kB | 23.8 kB | ✓ |
| app-route-ex..prod.js gzip | 16.4 kB | 16.4 kB | ✓ |
| app-route-tu..prod.js gzip | 16.5 kB | 16.5 kB | ✓ |
| app-route-tu..prod.js gzip | 16 kB | 16 kB | ✓ |
| app-route.ru...dev.js gzip | 23.2 kB | 23.2 kB | ✓ |
| app-route.ru..prod.js gzip | 16 kB | 16 kB | ✓ |
| pages-api-tu..prod.js gzip | 9.37 kB | 9.37 kB | ✓ |
| pages-api.ru...dev.js gzip | 9.64 kB | 9.64 kB | ✓ |
| pages-api.ru..prod.js gzip | 9.37 kB | 9.37 kB | ✓ |
| pages-turbo...prod.js gzip | 21.8 kB | 21.8 kB | ✓ |
| pages.runtim...dev.js gzip | 22.5 kB | 22.5 kB | ✓ |
| pages.runtim..prod.js gzip | 21.8 kB | 21.8 kB | ✓ |
| server.runti..prod.js gzip | 49.1 kB | 49.1 kB | ✓ |
| Overall change | 344 kB | 344 kB | ✓ |
Diff details
Diff for page.js
Diff too large to display
Diff for 199-HASH.js
Diff too large to display
Diff for app-page-exp..ntime.dev.js
Diff too large to display
Diff for app-page-exp..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page.runtime.dev.js
Diff too large to display
Diff for app-page.runtime.prod.js
Diff too large to display
|
EDIT: Never mind, I think I fixed everything. |
acdlite
force-pushed
the
add-cachenodeseeddata-to-rsc-payload
branch
from
014f565
to
90f897f
Compare
acdlite
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
This adds an additional item to the FlightData type. The type of the field is currently `null`, but eventually this slot will represent a tree of data that is used to seed the cache nodes. This is a fragile change because the FlightData type is not covered by TypeScript. This was an intentional decision to optimize the size of the Flight payload. It's made more tricky because there are many places in the codebase that access the fields of FlightData using direct indexing, e.g. `flightData[0]`. To minimze the number of places I needed to update, I added the new field to the end of the array. However, many places access the fields using negative indexing (via the `slice` method), so I needed to update all of those. I also had to change any place that checked the length of the array. In the future, when we introduce clever types like this that are intentionally unsound, we should contain the unsoundness to a single module by only accessing the type with getter functions. Something like: ```js const treePath = getTreePatch(flightData); const seedData = getCacheNodeSeedData(flightData); ``` and so on. That way when we add a new field like this, we don't have to carefully update every single place that accesses the type. (TypeScript lacks the ability to mark a type as opaque, unfortunately, but I believe you can simulate it in other ways. This is one feature I miss from Flow.) I considered adding these getter methods as part of this PR, but since we're in the middle of a larger refactor of the Flight response type, I'd prefer to change as little as possible for now until we can land an MVP of PPR for client navigations. (That being said, if we struggle to land this, I'll reconsider.) My strategy for finding the places that needed to update was to change the type of FlightDataPath to a nonsense type (e.g. number) and track down all the TypeScript errors. I also searched for all references to variables named `flightDataPath` or similar. I also referred to a prior PR, vercel#42791, which added the `head` field to the FlightDataPath type. Because of the nature of the change, there's a moderately elevated risk that this will break something. However, if something does slip through the cracks, it's likely to fail fast, given how common most of these paths are; I expect any egregious oversights would be caught by our e2e test suite.
acdlite
force-pushed
the
add-cachenodeseeddata-to-rsc-payload
branch
from
90f897f
to
d52fa34
Compare
timneutkens
approved these changes
Nov 17, 2023
acdlite
added a commit
to acdlite/next.js
that referenced
this pull request
Nov 20, 2023
This reverts commit d5aea2c, which added an additional field to the RSC payload intended to represent a tree of subtree data for all the nested layouts in a response. I didn't think about the fact that the subtreeData slot, which represents the node at the root of the subtree, would be included twice. Instead, we should replace the subtreeData slot to be of the CacheNodeSeedData tree type, instead of just a single node. I'll do that in the following commits.
acdlite
added a commit
to acdlite/next.js
that referenced
this pull request
Nov 20, 2023
This reverts commit d5aea2c, which added an additional field to the RSC payload intended to represent a tree of subtree data for all the nested layouts in a response. I didn't think about the fact that the subtreeData slot, which represents the node at the root of the subtree, would be included twice. Instead, we should replace the subtreeData slot to be of the CacheNodeSeedData tree type, instead of just a single node. I'll do that in the following commits.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This adds an additional item to the FlightData type. The type of the field is currently
null, but eventually this slot will represent a tree of data that is used to seed the cache nodes.This is a fragile change because the FlightData type is not covered by TypeScript. This was an intentional decision to optimize the size of the Flight payload. It's made more tricky because there are many places in the codebase that access the fields of FlightData using direct indexing, e.g.
flightData[0].To minimze the number of places I needed to update, I added the new field to the end of the array. However, many places access the fields using negative indexing (via the
slicemethod), so I needed to update all of those. I also had to change any place that checked the length of the array.In the future, when we introduce clever types like this that are intentionally unsound, we should contain the unsoundness to a single module by only accessing the type with getter functions. Something like:
and so on.
That way when we add a new field like this, we don't have to carefully update every single place that accesses the type. (TypeScript lacks the ability to mark a type as opaque, unfortunately, but I believe you can simulate it in other ways. This is one feature I miss from Flow.)
I considered adding these getter methods as part of this PR, but since we're in the middle of a larger refactor of the Flight response type, I'd prefer to change as little as possible for now until we can land an MVP of PPR for client navigations. (That being said, if we struggle to land this, I'll reconsider.)
My strategy for finding the places that needed to update was to change the type of FlightDataPath to a nonsense type (e.g. number) and track down all the TypeScript errors. I also searched for all references to variables named
flightDataPathor similar. I also referred to a prior PR, #42791, which added theheadfield to the FlightDataPath type.Because of the nature of the change, there's a moderately elevated risk that this will break something. However, if something does slip through the cracks, it's likely to fail fast, given how common most of these paths are; I expect any egregious oversights would be caught by our e2e test suite.