fix(stream-ssr): Move wait for all ready to fix bot rendering

packages/vite/src/streaming/streamHelpers.ts

@@ -67,6 +67,7 @@ export async function reactRenderToStreamResponse(
   // This is a transformer stream, that will inject all things called with useServerInsertedHtml
   const serverInjectionTransform = createServerInjectionTransform({
     injectionState,
+    onlyOnFlush: waitForAllReady,
   })
 
   // Timeout after 10 seconds
@@ -129,23 +130,33 @@ export async function reactRenderToStreamResponse(
         renderToStreamOptions
       )
 
-    const output = reactStream
-      .pipeThrough(bufferTransform)
-      .pipeThrough(serverInjectionTransform)
-      .pipeThrough(timeoutTransform)
-
+    // @NOTE: very important that we await this before we apply any transforms
     if (waitForAllReady) {
       await reactStream.allReady
+      clearTimeout(timeoutHandle)
     }
 
-    return new Response(output, {
+    const transformsToApply = [
+      !waitForAllReady && bufferTransform,
+      serverInjectionTransform,
+      !waitForAllReady && timeoutTransform,
+    ]
+
+    const outputStream: ReadableStream<Uint8Array> = applyStreamTransforms(
+      reactStream,
+      transformsToApply
+    )
+
+    return new Response(outputStream, {
       status: didErrorOutsideShell ? 500 : 200, // I think better right? Prevents caching a bad page
       headers: { 'content-type': 'text/html' },
     })
   } catch (e) {
     console.error('🔻 Failed to render shell')
     streamOptions.onError?.(e as Error)
 
+    clearTimeout(timeoutHandle)
+
     // @TODO Asking for clarification from React team. Their documentation on this is incomplete I think.
     // Having the Document (and bootstrap scripts) here allows client to recover from errors in the shell
     // To test this, throw an error in the App on the server only
@@ -164,3 +175,19 @@ export async function reactRenderToStreamResponse(
     })
   }
 }
+function applyStreamTransforms(
+  reactStream: ReactDOMServerReadableStream,
+  transformsToApply: (TransformStream | false)[]
+) {
+  let outputStream: ReadableStream<Uint8Array> = reactStream
+
+  for (const transform of transformsToApply) {
+    // If its false, skip
+    if (!transform) {
+      continue
+    }
+    outputStream = outputStream.pipeThrough(transform)
+  }
+
+  return outputStream
+}

packages/vite/src/streaming/transforms/serverInjectionTransform.ts

@@ -8,15 +8,25 @@ import { ServerInjectedHtml } from '@redwoodjs/web/dist/components/ServerInject'
 
 import { encodeText } from './encode-decode'
 
+type CreateServerInjectionArgs = {
+  injectionState: Set<RenderCallback>
+  onlyOnFlush?: boolean
+}
+
 export function createServerInjectionTransform({
   injectionState,
-}: {
-  injectionState: Set<RenderCallback>
-}) {
+  onlyOnFlush = false,
+}: CreateServerInjectionArgs) {
   const transformStream = new TransformStream({
     transform(chunk, controller) {
-      const mergedBytes = insertHtml(chunk)
-      controller.enqueue(mergedBytes)
+      if (onlyOnFlush) {
+        // when waiting for flush (or all ready), we do NOT buffer the stream
+        // and its not safe to inject except at the end
+        controller.enqueue(chunk)
+      } else {
+        const mergedBytes = insertHtml(chunk)
+        controller.enqueue(mergedBytes)
+      }
     },
     flush(controller) {
       // Before you finish, flush injected HTML again

tasks/smoke-tests/streaming-ssr-prod/tests/botRendering.spec.ts

@@ -1,42 +1,49 @@
-import type { Page } from '@playwright/test'
 import { test } from '@playwright/test'
 
 import { checkDelayedPageRendering } from '../../shared/delayedPage'
 import { checkHomePageCellRender } from '../../shared/homePage'
 
-let botPageNoJs: Page
+// UA taken from https://developers.google.com/search/docs/crawling-indexing/overview-google-crawlers
+const BOT_USERAGENT =
+  'Mozilla/5.0 AppleWebKit/537.36 (KHTML, like Gecko; compatible; Googlebot/2.1; +http://www.google.com/bot.html) Chrome/W.X.Y.Z Safari/537.36'
 
-test.beforeAll(async ({ browser }) => {
-  // UA taken from https://developers.google.com/search/docs/crawling-indexing/overview-google-crawlers
+test('Check that homepage has content fully rendered from the server, without JS', async ({
+  browser,
+}) => {
   const botContext = await browser.newContext({
-    userAgent:
-      'Mozilla/5.0 AppleWebKit/537.36 (KHTML, like Gecko; compatible; Googlebot/2.1; +http://www.google.com/bot.html) Chrome/W.X.Y.Z Safari/537.36',
-    // @@MARK TODO awaiting react team feedback. I dont understand why React is still injecting JS instead of giving us
-    // a fully formed HTML page
-    // javaScriptEnabled: false,
+    userAgent: BOT_USERAGENT,
+    // Even without JS, this should be a fully rendered page
+    javaScriptEnabled: false,
   })
 
-  const botPage = await botContext.newPage()
-  await botPage.route('**/*.*.{js,tsx,ts,jsx}', (route) => route.abort())
+  const botPageNoJs = await botContext.newPage()
 
-  botPageNoJs = botPage
-})
-
-test.afterAll(() => {
-  botPageNoJs.close()
-})
-
-test('Check that homepage has content rendered from the server', async () => {
   await botPageNoJs.goto('/')
 
   // Appears when Cell is successfully rendered
   await botPageNoJs.waitForSelector('article')
 
   await checkHomePageCellRender(botPageNoJs)
+
+  await botPageNoJs.close()
 })
 
-test('Check delayed page is NOT progressively rendered', async () => {
-  await checkDelayedPageRendering(botPageNoJs, {
+test('Check delayed page is NOT progressively rendered', async ({
+  browser,
+}) => {
+  // For this test we need to enable JS, but block all client side scripts
+  // So that we can check that expected delay is 0
+  const botContext = await browser.newContext({
+    userAgent: BOT_USERAGENT,
+  })
+
+  const botPageNoBundle = await botContext.newPage()
+
+  await botPageNoBundle.route('**/*.*.{js,tsx,ts,jsx}', (route) =>
+    route.abort()
+  )
+
+  await checkDelayedPageRendering(botPageNoBundle, {
     expectedDelay: 0,
   })
 })