-
Notifications
You must be signed in to change notification settings - Fork 324
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
Consume RSC response in the server #463
Conversation
bda6272
to
f7adeb8
Compare
c63f678
to
2ea3f3a
Compare
const writingSSR = bufferReadableStream( | ||
readable.getReader(), | ||
(chunk) => { | ||
isDocumentMalformed = !chunk.endsWith('>'); |
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.
@jplhomer Do you have a better idea to avoid writing RSC when SSR is in a malformed state?
This lets SSR write everything directly so the browser gets it asap. RSC writing is slightly delayed (it only writes when SSR is not malformed).
I saw Next.js uses a setTimeout
around SSR writing and buffers it a bit but I'm not sure if that fixes this issue.
Edit: thinking that there might be random >
symbols that don't signify a closing tag... 🤔
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.
What does "malformed state" mean in this context? Like when only a partial HTML tag has been written, and we don't want to start writing a script tag to flush RSC?
If so, is there a solution similar to using a TransformStream that Seb proposes here?
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.
Yes, the document has a partial tag written at the end so if we something else, the tag won't be valid. I've seen things like <html<script>...</script>>
or <div class="something<script>...</script>">
If so, is there a solution similar to using a TransformStream that Seb proposes here?
The thing is, we are already using a TransformStream with a similar approach.
This note from that post:
Note: React can write fractional HTML chunks so it's not safe to always inject HTML anywhere in a write call. The above technique relies on the fact that React won't render anything between writing. We assume that no more link tags will be collected between fractional writes. It is not safe to write things after React since there can be another write call coming after it.
What I understand is that the "link tags" in the example are collected from the SSR itself (think of Helmet). Therefore, there won't be new link tags generated until SSR makes more progress.
However, in our case, our script tags come from a different stream, the RSC, so it's completely random.
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.
Thinking about that note again, I think I understood why Next.js uses a setTimeout
. I think React probably writes fractional chunks synchronously. So if you delay the writing one tick, it might be already in good shape 🤔
Edit: as I thought, fractional chunks are indeed only written synchronously, so a timeout 0 works 👍
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.
🤯
<Suspense fallback={<BoxFallback />}> | ||
<StorefrontInfo /> | ||
</Suspense> | ||
<Suspense fallback={<BoxFallback />}> | ||
<TemplateLinks /> | ||
</Suspense> |
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.
Note: This was the culprit causing issues in Vite dev server for Hydration, on the homepage only. I refactored this to use multiple Suspense boundaries, etc.
Now it breaks in Workers 😭 seeing quite a bit of this with react-helmet-async:
TypeError: Cannot read property 'add' of undefined
at e2.r2.init (/Users/joshlarson/src/github.com/Shopify/hydrogen/node_modules/react-helmet-async/lib/index.module.js:1:10719)
at e2.r2.render (/Users/joshlarson/src/github.com/Shopify/hydrogen/node_modules/react-helmet-async/lib/index.module.js:1:10742)
at Fc$1 (/Users/joshlarson/src/github.com/Shopify/hydrogen/node_modules/react-dom/cjs/react-dom-server.browser.production.min.js:63:111)
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.
Thanks for the fix!
Now it breaks in Workers 😭 seeing quite a bit of this with react-helmet-async:
The only way I can reproduce this is:
- Start dev server in port 3000
- Open tab
- Close dev server and start Miniflare
- Let the previous tab reload
If I start from a new tab or just refresh, it seems to work well. I've also deployed it to CFW and works 🤔
Edit: ahh, it is happening randomly in the development server as well...
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 could look into this
@jplhomer I think if we figure out the random issue with the helmet we could start reviewing/merging this PR 🤔 -- I didn't manage to reproduce it consistently. I'll try to explore early hydration (with the new bootstrapScript thingy) in a different PR. |
<Suspense fallback={<BoxFallback />}> | ||
<StorefrontInfo /> | ||
</Suspense> | ||
<Suspense fallback={<BoxFallback />}> | ||
<TemplateLinks /> | ||
</Suspense> |
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 could look into this
isPendingSsrWrite = false; | ||
// React can write fractional chunks synchronously. | ||
// This timeout ensures we only write full HTML tags | ||
// in order to allow RSC writing concurrently. |
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'm confused, how does a timeout do that?
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 it's the equivalent of nextTick
Shopify/hydrogen#463 (comment)
(I don't fully understand how React writes synchronously but glad this works!)
Promise.all([writingSSR, writingRSC]).then(() => { | ||
// Last SSR write might be pending, delay closing the writable one tick | ||
setTimeout(() => writable.close(), 0); | ||
logServerResponse('str', log, request, responseOptions.status); |
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 a bit of a change in the way we measure timing. The console.log for the request with the time will now be the whole time it took to RSC and stream. At some point it would be nice to also log timing at various other spots. A few ideas:
- Time for first SSR byte
- Time for first RSC script tag
* feat: implement RSC using react-server and react-client (#317) * wip: Create custom flight renderer with local version of react-server * wip: mock client manifest and add module reference to client components * wip: hack RSC resolution * wip: wrap component in Proxy to access original properties * refactor: rename RSC server files * wip: add official RSC hydrator * fix: Rename response.readRoot and remove explicit hydration * wip: remove RR and Helmet providers from the server * wip: Add test app * wip: remove hydration providers * wip: add renderToReadableStream for RSC * refactor: cleanup custom RSC code * refactor: move and rename files * wip: fix test app * refactor: cleanup custom RSC code * refactor: do not pass named boolean in RSC * refactor: simplify ClientMarker * test: fix ClientMarker tests * fix: delay throwing error when client component is missing * refactor: simplify import globs code * feat: provide request object to rendering tree * wip: example of useServerRequest * feat: inline RSC response in the SSR HTML response * fix: add default value to the SSR provider * feat: stream rsc in script tags * feat: update the starter template to work * fix: lint errors * refactor: move code around and cleanup * chore: upgrade Vite to 2.7.0 * chore: add react-server-dom-vite as vendor * feat: replace local react-server and react-client with react-server-dom-vite vendor * fix: react-server-dom-vite allow exporting hooks from client components temporarily * fix: move the server logging locations * feat: Implement ReadableStream branch in SSR and RSC * test: fix playground apps * fix: avoid importing undefined exports depending on the running environment * feat: Buffer RSC response if it cannot be streamed * fix: tests * fix: e2e test * fix: linting * fix: rename test helper * fix: update hydrogen template files * feat: remove react-ssr-prepass. RSC already makes ReadableStream required * refactor: simplify hydration calls * feat: remove old dependencies * feat: enable tree shaking in worker build * fix: Release stream lock before piping * chore: fix formatting * refactor: move code to entry-server to be consistent * refactor: remove HydrationWriter in favor of Node native PassThrough * fix: move constant to a separate file to avoid importing app logic in GraphiQL * refactor: remove old code * fix: maybe fix tests for windows * fix: stream import in workers * fix: flush RSC right after writing head * feat: replace RenderCacheProvider with new ServerRequestProvider cache * refactor: cleanup * fix: suspense breaking hydration * fix: normalize RSC chunks * refactor: simplify customBody check * feat: minor tree-shaking improvements * fix: enable browser hydration * fix: replace TransformStream with ReadableStream to support Firefox * refactor: cleanup and rename variables * fix: normalizePath * fix: better regex * fix: request.context property is reserved in CFW * perf: Do not clone request to avoid extra memory and warnings in CFW * chore: add TODO to remove weird Suspense boundary * fix: Add back ShopifyProvider; call setShopConfig as part of renderHydrogen * chore: remove RSCTest demo code * fix: useMemo is allowed in RSC * fix: Dot not create Response with streams to support CFW Co-authored-by: Bret Little <bret.little@shopify.com> Co-authored-by: M. Bagher Abiat <zorofight94@gmail.com> Co-authored-by: Josh Larson <josh.larson@shopify.com> * feat: Consume RSC response in the server (#463) * wip: Create custom flight renderer with local version of react-server * wip: mock client manifest and add module reference to client components * wip: hack RSC resolution * wip: wrap component in Proxy to access original properties * refactor: rename RSC server files * wip: add official RSC hydrator * fix: Rename response.readRoot and remove explicit hydration * wip: remove RR and Helmet providers from the server * wip: Add test app * wip: remove hydration providers * wip: add renderToReadableStream for RSC * refactor: cleanup custom RSC code * refactor: move and rename files * wip: fix test app * refactor: cleanup custom RSC code * refactor: do not pass named boolean in RSC * refactor: simplify ClientMarker * test: fix ClientMarker tests * fix: delay throwing error when client component is missing * refactor: simplify import globs code * feat: provide request object to rendering tree * wip: example of useServerRequest * feat: inline RSC response in the SSR HTML response * fix: add default value to the SSR provider * feat: stream rsc in script tags * feat: update the starter template to work * fix: lint errors * refactor: move code around and cleanup * chore: upgrade Vite to 2.7.0 * chore: add react-server-dom-vite as vendor * feat: replace local react-server and react-client with react-server-dom-vite vendor * fix: react-server-dom-vite allow exporting hooks from client components temporarily * fix: move the server logging locations * feat: Implement ReadableStream branch in SSR and RSC * test: fix playground apps * fix: avoid importing undefined exports depending on the running environment * feat: Buffer RSC response if it cannot be streamed * fix: tests * fix: e2e test * fix: linting * fix: rename test helper * fix: update hydrogen template files * feat: remove react-ssr-prepass. RSC already makes ReadableStream required * refactor: simplify hydration calls * feat: remove old dependencies * feat: enable tree shaking in worker build * fix: Release stream lock before piping * chore: fix formatting * refactor: move code to entry-server to be consistent * refactor: remove HydrationWriter in favor of Node native PassThrough * fix: move constant to a separate file to avoid importing app logic in GraphiQL * refactor: remove old code * fix: maybe fix tests for windows * fix: stream import in workers * fix: flush RSC right after writing head * feat: replace RenderCacheProvider with new ServerRequestProvider cache * refactor: cleanup * fix: suspense breaking hydration * fix: normalize RSC chunks * refactor: simplify customBody check * feat: minor tree-shaking improvements * fix: enable browser hydration * fix: replace TransformStream with ReadableStream to support Firefox * refactor: cleanup and rename variables * feat: Make ReadableStream globally available * wip: consume RSC response in server * wip: do not close stream connection * wip: fix reentrant error with ReadableStreams in RSC * fix: import Node RSC logic dynamically * fix: rename Cache.client.js to avoid bundling it in workers build * fix: add temporary monkey patch for React Fizz in workers build * feat: Combine SSR and RSC responses in Node * fix: write RSC before ending response when not streaming * feat: Combine SSR and RSC responses in Worker * feat: Use ReadableStream in CFW and stream if supported * fix: add another temporary monkey patch for React Fizz/Flight in workers build * fix: close writable on redirects * fix: Fix hydration during dev by using more Suspense boundaries * fix: add back response.socket listener * feat: support script nonce and shorten access to __flight * feat: avoid writing fractional chunks in SSR * fix: initialize flight container in buffered rendering * fix: ensure flight container is added in Node * refactor: `readable` to `ssrReadable` for clarity Co-authored-by: Bret Little <bret.little@shopify.com> Co-authored-by: Josh Larson <josh.larson@shopify.com> * fix: make ServerHandlerConfig required * chore: update changelog * chore: update docs Co-authored-by: Fran Dios <frankdiox@gmail.com> Co-authored-by: Bret Little <bret.little@shopify.com> Co-authored-by: M. Bagher Abiat <zorofight94@gmail.com>
* Fix licenses * Fix oclif manifest
Description
According to React team's feedback, the recommended way to combine SSR + RSC is to consume the RSC response in the server.
How this works for an initial page request:
App
with RSCrenderToReadableStream
(fromreact-server-dom-vite
)tee
that stream so we can consume it in two wayscreateFromReadableStream
which gives us a React representation of the RSC output as React components a laresponse.readRoot()
. This is very similar to what you see in the browser when afetch('/react')
response is consumed.<Html />
, and pass that torenderToReadableStream
(fromreact-dom/server
)rscToScriptTagReadable
. This stream reads the RSC syntax from the initial readable and transforms it into<script>
tags. These script tags are responsible for incrementally hydrating the initial SSR page load. This is ideal, because it means we don't have to call/react
on the initial page load. Flush RSC response inline with SSR response #250response.doNotStream()
,response.redirect()
and other caching headers.TransformStream
and have both the SSR and the Script tag readable streams write to the transform writable. We do this in a way that prevents script tags from being written in the middle of an open SSR HTML tag, for example.new Response(transform.readable)
which streams the response to the browser.Note: We have encountered a couple random errors here and there, either related to bugs in React or perhaps 3p libraries like react-helmet. We'll keep investigating and fixing these throughout the dev preview period.
Other notes:
react-server-dom-vite
implementation to add back a reentrancy hack. This is necessary to support ReadableStream implementation of Flight for the time being until we land a fix upstream.Before submitting the PR, please make sure you do the following:
Unreleased
heading in the package'sCHANGELOG.md
fixes #123
)