-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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
Refactor server/render for SSR streaming #31231
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
packages/next/build/webpack/loaders/next-middleware-ssr-loader/index.ts
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
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 think server/render
is getting out of hand, but that doesn't need to block this PR. Will spec out a change.
} | ||
const Component = (props: any) => { | ||
return ( | ||
<React.Suspense fallback={null}> |
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.
FYI, Frameworks shouldn't insert suspense boundaries, because the app has no way to customize the fallback/loading state. What do we need this for?
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.
This is how it's implemented in Next.js today:
next.js/packages/next/build/webpack/loaders/next-middleware-ssr-loader/index.ts
Lines 92 to 98 in 28a5d7a
const Component = props => { | |
return createElement( | |
React.Suspense, | |
{ fallback: null }, | |
createElement(FlightWrapper, props) | |
) | |
}` |
and previously the top-level suspense boundary didn't exist. We can surely get rid of it, but I'm unsure if it will cause hydration mismatch on the client side since we'll need a boundary there.
@@ -274,6 +277,49 @@ function checkRedirectValues( | |||
} | |||
} | |||
|
|||
// Create the wrapper component for a Flight stream. | |||
function createServerComponentRenderer( |
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.
We should move this to compile time, but that doesn't block this PR.
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.
An overall refactoring to the middleware, server/next-server
and server/render
is needed.
} | ||
} | ||
|
||
function renderToWebStream( |
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.
Not super sure what's happening here. You don't need (or really want, since it could lead to early buffering) a startWriting
-- you should just need to start reading from the stream inside the returned piper function.
I'd just move the buffering up to the middleware layer.
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.
Something was introduced in #31186, to delay resolving until onCompleteShell
happens. If there is an error occurred before that, we need to throw, otherwise we can safely return the piper.
Agreed that server/render is getting out of hand, looking forward to your spec! |
Stats from current PRDefault Build (Decrease detected ✓)General Overall increase
|
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
buildDuration | 23.7s | 24s | |
buildDurationCached | 4.4s | 4.4s | |
nodeModulesSize | 335 MB | 335 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.748 | 4.006 | |
/ avg req/sec | 667.09 | 624.06 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 2.024 | 2.019 | 0 |
/error-in-render avg req/sec | 1234.98 | 1238.1 | +3.12 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.2 kB | 42.2 kB | ✓ |
main-HASH.js gzip | 28.3 kB | 28.3 kB | ✓ |
webpack-HASH.js gzip | 1.45 kB | 1.45 kB | ✓ |
Overall change | 72.2 kB | 72.2 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.37 kB | 1.37 kB | ✓ |
_error-HASH.js gzip | 194 B | 194 B | ✓ |
amp-HASH.js gzip | 312 B | 312 B | ✓ |
css-HASH.js gzip | 327 B | 327 B | ✓ |
dynamic-HASH.js gzip | 2.38 kB | 2.38 kB | ✓ |
head-HASH.js gzip | 350 B | 350 B | ✓ |
hooks-HASH.js gzip | 635 B | 635 B | ✓ |
image-HASH.js gzip | 4.44 kB | 4.44 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 1.87 kB | 1.87 kB | ✓ |
routerDirect..HASH.js gzip | 321 B | 321 B | ✓ |
script-HASH.js gzip | 383 B | 383 B | ✓ |
withRouter-HASH.js gzip | 318 B | 318 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 13.3 kB | 13.3 kB | ✓ |
Client Build Manifests
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
_buildManifest.js gzip | 460 B | 460 B | ✓ |
Overall change | 460 B | 460 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
index.html gzip | 521 B | 521 B | ✓ |
link.html gzip | 534 B | 534 B | ✓ |
withRouter.html gzip | 515 B | 515 B | ✓ |
Overall change | 1.57 kB | 1.57 kB | ✓ |
Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
buildDuration | 24.2s | 24.1s | -34ms |
buildDurationCached | 4.3s | 4.3s | |
nodeModulesSize | 335 MB | 335 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.749 | 3.878 | |
/ avg req/sec | 666.86 | 644.61 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.982 | 1.955 | -0.03 |
/error-in-render avg req/sec | 1261.32 | 1278.5 | +17.18 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.3 kB | 42.3 kB | ✓ |
main-HASH.js gzip | 28.5 kB | 28.5 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 72.5 kB | 72.5 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
_error-HASH.js gzip | 180 B | 180 B | ✓ |
amp-HASH.js gzip | 305 B | 305 B | ✓ |
css-HASH.js gzip | 321 B | 321 B | ✓ |
dynamic-HASH.js gzip | 2.38 kB | 2.38 kB | ✓ |
head-HASH.js gzip | 342 B | 342 B | ✓ |
hooks-HASH.js gzip | 622 B | 622 B | ✓ |
image-HASH.js gzip | 4.46 kB | 4.46 kB | ✓ |
index-HASH.js gzip | 256 B | 256 B | ✓ |
link-HASH.js gzip | 1.91 kB | 1.91 kB | ✓ |
routerDirect..HASH.js gzip | 314 B | 314 B | ✓ |
script-HASH.js gzip | 375 B | 375 B | ✓ |
withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 13.2 kB | 13.2 kB | ✓ |
Client Build Manifests
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
_buildManifest.js gzip | 460 B | 460 B | ✓ |
Overall change | 460 B | 460 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | shuding/next.js shu/56d0 | Change | |
---|---|---|---|
index.html gzip | 523 B | 523 B | ✓ |
link.html gzip | 535 B | 535 B | ✓ |
withRouter.html gzip | 516 B | 516 B | ✓ |
Overall change | 1.57 kB | 1.57 kB | ✓ |
Initial step to refactor the rendering logic by decoupling the handler and renderer:
In 1), this PR also makes sure that gSSP/gSP are correctly executed before the Flight stream and
pageProps
androuter
are correctly delivered to the component.Related to #30994.
Bug
fixes #number
contributing.md
Feature
fixes #number
contributing.md
Documentation / Examples
yarn lint