From bdf885a0848c0aa97be2d7d16fa7bac60ff64f68 Mon Sep 17 00:00:00 2001 From: Daniel Choudhury Date: Thu, 9 Nov 2023 16:16:47 +0700 Subject: [PATCH] fix(stream-ssr): Move wait for all ready to fix bot rendering (#9389) --- packages/vite/src/streaming/streamHelpers.ts | 39 ++++++++++++--- .../transforms/serverInjectionTransform.ts | 20 ++++++-- .../tests/botRendering.spec.ts | 49 +++++++++++-------- 3 files changed, 76 insertions(+), 32 deletions(-) diff --git a/packages/vite/src/streaming/streamHelpers.ts b/packages/vite/src/streaming/streamHelpers.ts index e92e9038c624..eedd31a34d1f 100644 --- a/packages/vite/src/streaming/streamHelpers.ts +++ b/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,16 +130,24 @@ 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 = 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' }, }) @@ -146,6 +155,8 @@ export async function reactRenderToStreamResponse( 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 = reactStream + + for (const transform of transformsToApply) { + // If its false, skip + if (!transform) { + continue + } + outputStream = outputStream.pipeThrough(transform) + } + + return outputStream +} diff --git a/packages/vite/src/streaming/transforms/serverInjectionTransform.ts b/packages/vite/src/streaming/transforms/serverInjectionTransform.ts index 15475cf731b4..5908dfa0b373 100644 --- a/packages/vite/src/streaming/transforms/serverInjectionTransform.ts +++ b/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 + onlyOnFlush?: boolean +} + export function createServerInjectionTransform({ injectionState, -}: { - injectionState: Set -}) { + 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 diff --git a/tasks/smoke-tests/streaming-ssr-prod/tests/botRendering.spec.ts b/tasks/smoke-tests/streaming-ssr-prod/tests/botRendering.spec.ts index 439db2806448..59619645f3bc 100644 --- a/tasks/smoke-tests/streaming-ssr-prod/tests/botRendering.spec.ts +++ b/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, }) })