Skip to content
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

Use renderToStream with React 18 #34106

Merged
merged 2 commits into from
Feb 8, 2022

Conversation

devknoll
Copy link
Contributor

@devknoll devknoll commented Feb 8, 2022

As per React 18 recommendation, we should use e.g. renderToReadableStream whenever we use createRoot. This is particularly important for currently supported suspense features like React.lazy to work properly during SSR.

However, unless you have opted in to streaming support (via the runtime flag), we will wait until onCompleteAll before sending it (via the generateStaticHTML flag).


Fixes #33879

@ijjk ijjk added created-by: Chrome Aurora PRs by the Google Chrome team: https://web.dev/aurora type: next labels Feb 8, 2022
@ijjk

This comment has been minimized.

@devknoll devknoll marked this pull request as ready for review February 8, 2022 19:11
@ijjk
Copy link
Member

ijjk commented Feb 8, 2022

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
buildDuration 20.4s 20.5s ⚠️ +54ms
buildDurationCached 7.7s 7.2s -549ms
nodeModulesSize 359 MB 359 MB ⚠️ +654 B
Page Load Tests Overall increase ✓
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
/ failed reqs 0 0
/ total time (seconds) 4.413 4.536 ⚠️ +0.12
/ avg req/sec 566.47 551.12 ⚠️ -15.35
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 2.169 2.097 -0.07
/error-in-render avg req/sec 1152.86 1192.13 +39.27
Client Bundles (main, webpack, commons)
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 27.5 kB 27.5 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.2 kB 71.2 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 194 B 194 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 326 B 326 B
dynamic-HASH.js gzip 2.57 kB 2.57 kB
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 919 B 919 B
image-HASH.js gzip 5.01 kB 5.01 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.26 kB 2.26 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 14.7 kB 14.7 kB
Client Build Manifests
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
_buildManifest.js gzip 458 B 458 B
Overall change 458 B 458 B
Rendered Page Sizes
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
index.html gzip 531 B 531 B
link.html gzip 544 B 544 B
withRouter.html gzip 525 B 525 B
Overall change 1.6 kB 1.6 kB

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
buildDuration 22.6s 23.6s ⚠️ +982ms
buildDurationCached 7.3s 7.6s ⚠️ +220ms
nodeModulesSize 359 MB 359 MB ⚠️ +654 B
Page Load Tests Overall increase ✓
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
/ failed reqs 0 0
/ total time (seconds) 4.151 4.119 -0.03
/ avg req/sec 602.27 607 +4.73
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 2.161 2.064 -0.1
/error-in-render avg req/sec 1156.65 1211.19 +54.54
Client Bundles (main, webpack, commons)
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.1 kB 42.1 kB
main-HASH.js gzip 27.5 kB 27.5 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.3 kB 71.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root 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.56 kB 2.56 kB
head-HASH.js gzip 342 B 342 B
hooks-HASH.js gzip 911 B 911 B
image-HASH.js gzip 5.05 kB 5.05 kB
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.28 kB 2.28 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 14.7 kB 14.7 kB
Client Build Manifests
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary azukaru/next.js x-use-stream-with-react-root Change
index.html gzip 533 B 533 B
link.html gzip 547 B 547 B
withRouter.html gzip 527 B 527 B
Overall change 1.61 kB 1.61 kB
Commit: f589fa2

@kodiakhq kodiakhq bot merged commit df29561 into vercel:canary Feb 8, 2022
@gaearon
Copy link
Contributor

gaearon commented Feb 11, 2022

I'm getting

TypeError: ReactDOMServer.renderToReadableStream is not a function
--
04:08:38.682 | at /vercel/path0/beta/node_modules/next/dist/server/render.js:1098:39
04:08:38.682 | at new Promise (<anonymous>)
04:08:38.682 | at renderToStream (/vercel/path0/beta/node_modules/next/dist/server/render.js:1079:12)
04:08:38.682 | at Object.bodyResult (/vercel/path0/beta/node_modules/next/dist/server/render.js:797:34)
04:08:38.682 | at renderToHTML (/vercel/path0/beta/node_modules/next/dist/server/render.js:935:30)
04:08:38.682 | at async /vercel/path0/beta/node_modules/next/dist/export/worker.js:275:36
04:08:38.682 | at async Span.traceAsyncFn (/vercel/path0/beta/node_modules/next/dist/trace/trace.js:75:20)

I'm a bit surprised by the code:

).renderToReadableStream(element, {

Isn't this supposed to be called renderToPipeableStream? That's what ReactDOMServer API is called:

https://github.com/facebook/react/blob/64223fed82414ee41839e95ebc97df330b2b71ca/packages/react-dom/server.node.js#L39-L44

@devknoll
Copy link
Contributor Author

devknoll commented Feb 11, 2022

@gaearon which version of Next.js are you running? Can you make sure it's v12.0.11-canary.10 or higher? There was a bug in canary 9 that was breaking this.

We've decided to use just renderToReadableStream (the browser/Web Streams API) for both Node.js and the Edge runtime since ReadableStream is available (or polyfillable) on recent Node versions and we haven't seen progress/interest on facebook/react#22350 or reactwg/react-18#118 and this allows us to move faster

@gaearon
Copy link
Contributor

gaearon commented Feb 11, 2022

We've decided to use just renderToReadableStream (the browser/Web Streams API) for both Node.js and the Edge runtime since ReadableStream is available (or polyfillable) on recent Node versions

Do you have concerns about this:

In Node.js WritableStreams (and in many other platforms before it), there's a flush() method that React calls to force a signal to any GZIP middleware that it now has to flush its buffer even if it's not yet full. This doesn't exist on https://github.com/reactwg/react-18/discussions/66s and that's why we don't support ReadableStream. Web Streams primarily use ReadableStreams as the return value of things like responseWith. You can use WritableStreams (but not in Firefox). However, there's whatwg/streams#960 even in a WritableStream. I'm sure there will be solution eventually. However, if you're trying to build a production server on top of WebStreams with compression support, you'll need to invent an API convention for this that we can add support for.

?

@devknoll
Copy link
Contributor Author

devknoll commented Feb 11, 2022

Yes, that's why we would love to have a more generic stream interface :)

For now, we just buffer everything React writes during a macro task and manually flush() it at the start of the next one. Not ideal, but good enough for now.

@gaearon
Copy link
Contributor

gaearon commented Feb 11, 2022

Hmm, canary.10 fixes the crash but the static build times out now:

https://vercel.com/fbopensource/beta-reactjs-org/E1KYDNrqyisLTfXRRtTGDJTfTVva

What can we check?

@gaearon
Copy link
Contributor

gaearon commented Feb 11, 2022

Note the latest React now returns a Promise and onCompleteShell is gone.

https://github.com/facebook/react/pull/23247/files

But I'm running an older version so that shouldn't be the reason..

@devknoll
Copy link
Contributor Author

Taking a look now

natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022
As per React 18 recommendation, we should use e.g. `renderToReadableStream` whenever we use `createRoot`. This is particularly important for currently supported suspense features like `React.lazy` to work properly during SSR.

However, unless you have opted in to streaming support (via [the `runtime` flag](vercel#34068)), we will wait until `onCompleteAll` before sending it (via the `generateStaticHTML` flag).

---

Fixes vercel#33879
@gaearon
Copy link
Contributor

gaearon commented Feb 18, 2022

12.0.10 worked for us but 12.1.0 hangs again. reactjs/react.dev#4354

@devknoll
Copy link
Contributor Author

devknoll commented Mar 3, 2022

@gaearon Sorry for the delay here. I believe this should be fixed after this change in React: facebook/react#23387

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Chrome Aurora PRs by the Google Chrome team: https://web.dev/aurora type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React.lazy() content HTML does not get included in SSG output
4 participants