Seed CacheNodes immediately after receiving response #58669
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
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
Diff detailsDiff for page.jsDiff too large to display Diff for 199-HASH.jsDiff too large to display Diff for app-page-exp..ntime.dev.jsDiff too large to display Diff for app-page-exp..time.prod.jsDiff too large to display Diff for app-page-tur..time.prod.jsDiff too large to display Diff for app-page-tur..time.prod.jsDiff too large to display Diff for app-page.runtime.dev.jsDiff too large to display Diff for app-page.runtime.prod.jsDiff too large to display |
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.
I need to use the result of createComponentTree in two different parts of the Flight response, but it shouldn't be called during a prefetch. So I hoisted the shouldSkipComponentTree check to earlier in the control flow. This also lets us get rid of an extra React element that was being used to lazily call createComponentTree.
The Flight response type includes the component node for the root of the subtree that was rendered. Instead of passing just the root node, we want to pass the entire tree of nested layouts. As an imcremental step, this changes the type to pass a CacheNodeSeedData object, but it passes only a partial tree that includes only the root node. In the next step, we'll pass the entire tree in this object, and in exchange we'll remove the initialChildNode prop to LayoutRouter.
acdlite
force-pushed
the
seed-cachenodes-immediately
branch
3 times, most recently
from
846933e
to
71f2f3c
Compare
We render nested layouts in parallel on the server. This means we should be able to create a CacheNode entry for every layout in the tree as soon as the Flight response is received by the client, even before the nested layouts have streamed in. Currently, we wait until the nested layouts start rendering before we write them into the cache, which prevents us from performing certain optimizations. That's because the CacheNodes are sent as a prop to LayoutRouter; the only way to unwrap the CacheNode is to wait for LayoutRouter to render. In previous PRs, I updated the server to create a top-level data structure that contains all the CacheNodes for the entire tree. This PR updates the client side to receive the nodes and write them into the cache.
acdlite
force-pushed
the
seed-cachenodes-immediately
branch
from
71f2f3c
to
165cb54
Compare
acdlite
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
feedthejim
reviewed
Nov 21, 2023
| @@ -11,21 +11,22 @@ export function applyFlightData( | |||
| wasPrefetched: boolean = false | |||
| ): boolean { | |||
| // The one before last item is the router state tree patch | |||
| const [treePatch, subTreeData, head /* , cacheNodeSeedData */] = | |||
| flightDataPath.slice(-4) | |||
| const [treePatch, cacheNodeSeedData, head] = flightDataPath.slice(-3) | |||
There was a problem hiding this comment.
While you're touching this, should extract the slices call to some sort of helper or something? It's always been sort of error-prone and this is a good opportunity to clean it up
There was a problem hiding this comment.
(same for the random array accesses)
There was a problem hiding this comment.
I intentionally left these alone because we’re about to completely change this data structure anyway.
There was a problem hiding this comment.
But yes I agree we should use accessor functions for internally unsound data structures in the future
feedthejim
approved these changes
Nov 21, 2023
|
looks good to me on my end, nice reverse engineering :) |
ztanner
approved these changes
Nov 21, 2023
acdlite
added a commit
to acdlite/next.js
that referenced
this pull request
Nov 21, 2023
We previously used preloadComponent to render nested page/layouts in parallel, but now that all layouts are passed to React at the top level (vercel#58669), we no longer need this special module. A flaw of preloadComponent was that it only called the top-most component in the layout. In the new approach, React can render the entire layout tree in parallel.
This was referenced Nov 21, 2023
kodiakhq bot
pushed a commit
that referenced
this pull request
Nov 22, 2023
After the changes in #58669, it's now possible for these `DynamicUsageErrors` to be caught in the streaming renderer. We didn't have any logic to handle this happening inside the existing catch block, so this adds a check for that specific error and re-throws it so that it properly bubbles to `exportPage`. `exportPage` checks the error to see if it's a `DynamicUsageError` and if so, marks the page as dynamic. (This resolves an issue with particular scenarios hanging the build, caught by a few of our tests: [example](https://github.com/vercel/next.js/actions/runs/6949079695/job/18907597751?pr=58744), [example](https://github.com/vercel/next.js/actions/runs/6948412805/job/18904387924))
kodiakhq bot
pushed a commit
that referenced
this pull request
Nov 27, 2023
We previously used preloadComponent to render nested page/layouts in parallel, but now that all layouts are passed to React at the top level (#58669), we no longer need this special module. A flaw of preloadComponent was that it only called the top-most component per layout. In the new approach, React can preload non-layout components, too. Co-authored-by: Zack Tanner <zacktanner@gmail.com>
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.
Based on #58666
I submitted this stack as separate PRs so I could run CI but I want to land them as a single unit. So I've left all but this last one in draft mode. You can review them commit-by-commit.
We render nested layouts in parallel on the server. This means we should be able to create a CacheNode entry for every layout in the tree as soon as the Flight response is received by the client, even before the nested layouts have streamed in. Currently, we wait until the nested layouts start rendering before we write them into the cache, which prevents us from performing certain optimizations. That's because the CacheNodes are sent as a prop to LayoutRouter; the only way to unwrap the CacheNode is to wait for LayoutRouter to render.
In previous PRs, I updated the server to create a top-level data structure that contains all the CacheNodes for the entire tree. This PR updates the client side to receive the nodes and write them into the cache.