Enable html post optimization for react 18 (#36837)

Follow up for #35888 to re-enable more test, and re-enable post processors after #36792 has better support for document.gIP with react 18. Apply post-pocessing when the the shell chunk is fully buffered. re-enabled integration tests for react 18: - amphtml - amphtml-custom-optimizer - app-document - font-optimization Fixes #35835 ## Bug - [x] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md`
vercel · May 12, 2022 · adb56ef · adb56ef
1 parent b717b6e
commit adb56ef
Show file tree

Hide file tree

Showing 15 changed files with 187 additions and 236 deletions.
diff --git a/packages/next/server/node-web-streams-helper.ts b/packages/next/server/node-web-streams-helper.ts
@@ -86,18 +86,18 @@ export function decodeText(input?: Uint8Array, textDecoder?: TextDecoder) {
     : new TextDecoder().decode(input)
 }
 
-export function createBufferedTransformStream(): TransformStream<
-  Uint8Array,
-  Uint8Array
-> {
+export function createBufferedTransformStream(
+  transform: (v: string) => string | Promise<string> = (v) => v
+): TransformStream<Uint8Array, Uint8Array> {
   let bufferedString = ''
   let pendingFlush: Promise<void> | null = null
 
   const flushBuffer = (controller: TransformStreamDefaultController) => {
     if (!pendingFlush) {
       pendingFlush = new Promise((resolve) => {
-        setTimeout(() => {
-          controller.enqueue(encodeText(bufferedString))
+        setTimeout(async () => {
+          const buffered = await transform(bufferedString)
+          controller.enqueue(encodeText(buffered))
           bufferedString = ''
           pendingFlush = null
           resolve()

diff --git a/packages/next/shared/lib/post-process.ts → packages/next/server/post-process.ts b/packages/next/shared/lib/post-process.ts → packages/next/server/post-process.ts
@@ -1,7 +1,18 @@
+import type { RenderOpts } from './render'
 import { parse, HTMLElement } from 'next/dist/compiled/node-html-parser'
-import { OPTIMIZED_FONT_PROVIDERS } from './constants'
-
-// const MIDDLEWARE_TIME_BUDGET = parseInt(process.env.__POST_PROCESS_MIDDLEWARE_TIME_BUDGET || '', 10) || 10
+import { OPTIMIZED_FONT_PROVIDERS } from '../shared/lib/constants'
+import { nonNullable } from '../lib/non-nullable'
+
+let optimizeAmp: typeof import('./optimize-amp').default | undefined
+let getFontDefinitionFromManifest:
+  | typeof import('./font-utils').getFontDefinitionFromManifest
+  | undefined
+
+if (process.env.NEXT_RUNTIME !== 'edge') {
+  optimizeAmp = require('./optimize-amp').default
+  getFontDefinitionFromManifest =
+    require('./font-utils').getFontDefinitionFromManifest
+}
 
 type postProcessOptions = {
   optimizeFonts: boolean
@@ -165,6 +176,73 @@ class FontOptimizerMiddleware implements PostProcessMiddleware {
   }
 }
 
+async function postProcessHTML(
+  pathname: string,
+  content: string,
+  renderOpts: RenderOpts,
+  { inAmpMode, hybridAmp }: { inAmpMode: boolean; hybridAmp: boolean }
+) {
+  const postProcessors: Array<(html: string) => Promise<string>> = [
+    process.env.NEXT_RUNTIME !== 'edge' && inAmpMode
+      ? async (html: string) => {
+          html = await optimizeAmp!(html, renderOpts.ampOptimizerConfig)
+          if (!renderOpts.ampSkipValidation && renderOpts.ampValidator) {
+            await renderOpts.ampValidator(html, pathname)
+          }
+          return html
+        }
+      : null,
+    process.env.NEXT_RUNTIME !== 'edge' && process.env.__NEXT_OPTIMIZE_FONTS
+      ? async (html: string) => {
+          const getFontDefinition = (url: string): string => {
+            if (renderOpts.fontManifest) {
+              return getFontDefinitionFromManifest!(
+                url,
+                renderOpts.fontManifest
+              )
+            }
+            return ''
+          }
+          return await processHTML(
+            html,
+            { getFontDefinition },
+            {
+              optimizeFonts: renderOpts.optimizeFonts,
+            }
+          )
+        }
+      : null,
+    process.env.NEXT_RUNTIME !== 'edge' && renderOpts.optimizeCss
+      ? async (html: string) => {
+          // eslint-disable-next-line import/no-extraneous-dependencies
+          const Critters = require('critters')
+          const cssOptimizer = new Critters({
+            ssrMode: true,
+            reduceInlineStyles: false,
+            path: renderOpts.distDir,
+            publicPath: `${renderOpts.assetPrefix}/_next/`,
+            preload: 'media',
+            fonts: false,
+            ...renderOpts.optimizeCss,
+          })
+          return await cssOptimizer.process(html)
+        }
+      : null,
+    inAmpMode || hybridAmp
+      ? async (html: string) => {
+          return html.replace(/&amp;amp=1/g, '&amp=1')
+        }
+      : null,
+  ].filter(nonNullable)
+
+  for (const postProcessor of postProcessors) {
+    if (postProcessor) {
+      content = await postProcessor(content)
+    }
+  }
+  return content
+}
+
 // Initialization
 registerPostProcessor(
   'Inline-Fonts',
@@ -174,4 +252,4 @@ registerPostProcessor(
   (options) => options.optimizeFonts || process.env.__NEXT_OPTIMIZE_FONTS
 )
 
-export default processHTML
+export { postProcessHTML }
diff --git a/packages/next/server/render.tsx b/packages/next/server/render.tsx
@@ -83,12 +83,10 @@ import { FlushEffectsContext } from '../shared/lib/flush-effects'
 import { interopDefault } from '../lib/interop-default'
 import stripAnsi from 'next/dist/compiled/strip-ansi'
 import { urlQueryToSearchParams } from '../shared/lib/router/utils/querystring'
+import { postProcessHTML } from './post-process'
 
-let optimizeAmp: typeof import('./optimize-amp').default
-let getFontDefinitionFromManifest: typeof import('./font-utils').getFontDefinitionFromManifest
 let tryGetPreviewData: typeof import('./api-utils/node').tryGetPreviewData
 let warn: typeof import('../build/output/log').warn
-let postProcess: typeof import('../shared/lib/post-process').default
 
 const DOCTYPE = '<!DOCTYPE html>'
 const ReactDOMServer = process.env.__NEXT_REACT_ROOT
@@ -97,12 +95,8 @@ const ReactDOMServer = process.env.__NEXT_REACT_ROOT
 
 if (process.env.NEXT_RUNTIME !== 'edge') {
   require('./node-polyfill-web-streams')
-  optimizeAmp = require('./optimize-amp').default
-  getFontDefinitionFromManifest =
-    require('./font-utils').getFontDefinitionFromManifest
   tryGetPreviewData = require('./api-utils/node').tryGetPreviewData
   warn = require('../build/output/log').warn
-  postProcess = require('../shared/lib/post-process').default
 } else {
   warn = console.warn.bind(console)
 }
@@ -471,7 +465,6 @@ export async function renderToHTML(
     ampPath = '',
     pageConfig = {},
     buildManifest,
-    fontManifest,
     reactLoadableManifest,
     ErrorDebug,
     getStaticProps,
@@ -535,13 +528,6 @@ export async function renderToHTML(
     })
   }
 
-  const getFontDefinition = (url: string): string => {
-    if (fontManifest) {
-      return getFontDefinitionFromManifest(url, fontManifest)
-    }
-    return ''
-  }
-
   let renderServerComponentData = isServerComponent
     ? query.__flight__ !== undefined
     : false
@@ -1487,65 +1473,67 @@ export async function renderToHTML(
         })
       }
 
-      const createBodyResult =
-        (initialStream: ReactReadableStream) => (suffix: string) => {
-          // this must be called inside bodyResult so appWrappers is
-          // up to date when `wrapApp` is called
-          const flushEffectHandler = (): string => {
-            const allFlushEffects = [
-              styledJsxFlushEffect,
-              ...(flushEffects || []),
-            ]
-            const flushed = ReactDOMServer.renderToString(
-              <>
-                {allFlushEffects.map((flushEffect, i) => (
-                  <React.Fragment key={i}>{flushEffect()}</React.Fragment>
-                ))}
-              </>
-            )
-            return flushed
-          }
+      const createBodyResult = (
+        initialStream: ReactReadableStream,
+        suffix?: string
+      ) => {
+        // this must be called inside bodyResult so appWrappers is
+        // up to date when `wrapApp` is called
+        const flushEffectHandler = (): string => {
+          const allFlushEffects = [
+            styledJsxFlushEffect,
+            ...(flushEffects || []),
+          ]
+          const flushed = ReactDOMServer.renderToString(
+            <>
+              {allFlushEffects.map((flushEffect, i) => (
+                <React.Fragment key={i}>{flushEffect()}</React.Fragment>
+              ))}
+            </>
+          )
+          return flushed
+        }
 
-          // Handle static data for server components.
-          async function generateStaticFlightDataIfNeeded() {
-            if (serverComponentsPageDataTransformStream) {
-              // If it's a server component with the Node.js runtime, we also
-              // statically generate the page data.
-              let data = ''
-
-              const readable = serverComponentsPageDataTransformStream.readable
-              const reader = readable.getReader()
-              const textDecoder = new TextDecoder()
-
-              while (true) {
-                const { done, value } = await reader.read()
-                if (done) {
-                  break
-                }
-                data += decodeText(value, textDecoder)
+        // Handle static data for server components.
+        async function generateStaticFlightDataIfNeeded() {
+          if (serverComponentsPageDataTransformStream) {
+            // If it's a server component with the Node.js runtime, we also
+            // statically generate the page data.
+            let data = ''
+
+            const readable = serverComponentsPageDataTransformStream.readable
+            const reader = readable.getReader()
+            const textDecoder = new TextDecoder()
+
+            while (true) {
+              const { done, value } = await reader.read()
+              if (done) {
+                break
               }
+              data += decodeText(value, textDecoder)
+            }
 
-              ;(renderOpts as any).pageData = {
-                ...(renderOpts as any).pageData,
-                __flight__: data,
-              }
-              return data
+            ;(renderOpts as any).pageData = {
+              ...(renderOpts as any).pageData,
+              __flight__: data,
             }
+            return data
           }
-
-          // @TODO: A potential improvement would be to reuse the inlined
-          // data stream, or pass a callback inside as this doesn't need to
-          // be streamed.
-          // Do not use `await` here.
-          generateStaticFlightDataIfNeeded()
-          return continueFromInitialStream(initialStream, {
-            suffix,
-            dataStream: serverComponentsInlinedTransformStream?.readable,
-            generateStaticHTML,
-            flushEffectHandler,
-          })
         }
 
+        // @TODO: A potential improvement would be to reuse the inlined
+        // data stream, or pass a callback inside as this doesn't need to
+        // be streamed.
+        // Do not use `await` here.
+        generateStaticFlightDataIfNeeded()
+        return continueFromInitialStream(initialStream, {
+          suffix,
+          dataStream: serverComponentsInlinedTransformStream?.readable,
+          generateStaticHTML,
+          flushEffectHandler,
+        })
+      }
+
       const hasDocumentGetInitialProps = !(
         isServerComponent ||
         process.env.NEXT_RUNTIME === 'edge' ||
@@ -1563,10 +1551,12 @@ export async function renderToHTML(
         documentInitialPropsRes = await loadDocumentInitialProps(renderShell)
         if (documentInitialPropsRes === null) return null
         const { docProps } = documentInitialPropsRes as any
-        bodyResult = createBodyResult(streamFromArray([docProps.html]))
+        // includes suffix in initial html stream
+        bodyResult = (suffix: string) =>
+          createBodyResult(streamFromArray([docProps.html, suffix]))
       } else {
         const stream = await renderShell(App, Component)
-        bodyResult = createBodyResult(stream)
+        bodyResult = (suffix: string) => createBodyResult(stream, suffix)
         documentInitialPropsRes = {}
       }
 
@@ -1738,7 +1728,7 @@ export async function renderToHTML(
     prefix.push('<!-- __NEXT_DATA__ -->')
   }
 
-  let streams = [
+  const streams = [
     streamFromArray(prefix),
     await documentResult.bodyResult(renderTargetSuffix),
   ]
@@ -1751,67 +1741,19 @@ export async function renderToHTML(
     return RenderResult.fromStatic((renderOpts as any).pageData)
   }
 
-  const postProcessors: Array<((html: string) => Promise<string>) | null> = (
-    generateStaticHTML
-      ? [
-          inAmpMode
-            ? async (html: string) => {
-                html = await optimizeAmp(html, renderOpts.ampOptimizerConfig)
-                if (!renderOpts.ampSkipValidation && renderOpts.ampValidator) {
-                  await renderOpts.ampValidator(html, pathname)
-                }
-                return html
-              }
-            : null,
-          process.env.NEXT_RUNTIME !== 'edge' &&
-          process.env.__NEXT_OPTIMIZE_FONTS
-            ? async (html: string) => {
-                return await postProcess(
-                  html,
-                  { getFontDefinition },
-                  {
-                    optimizeFonts: renderOpts.optimizeFonts,
-                  }
-                )
-              }
-            : null,
-          process.env.NEXT_RUNTIME !== 'edge' && renderOpts.optimizeCss
-            ? async (html: string) => {
-                // eslint-disable-next-line import/no-extraneous-dependencies
-                const Critters = require('critters')
-                const cssOptimizer = new Critters({
-                  ssrMode: true,
-                  reduceInlineStyles: false,
-                  path: renderOpts.distDir,
-                  publicPath: `${renderOpts.assetPrefix}/_next/`,
-                  preload: 'media',
-                  fonts: false,
-                  ...renderOpts.optimizeCss,
-                })
-                return await cssOptimizer.process(html)
-              }
-            : null,
-          inAmpMode || hybridAmp
-            ? async (html: string) => {
-                return html.replace(/&amp;amp=1/g, '&amp=1')
-              }
-            : null,
-        ]
-      : []
-  ).filter(Boolean)
-
-  if (generateStaticHTML || postProcessors.length > 0) {
-    let html = await streamToString(chainStreams(streams))
-    for (const postProcessor of postProcessors) {
-      if (postProcessor) {
-        html = await postProcessor(html)
-      }
-    }
-    return new RenderResult(html)
+  const postOptimize = (html: string) =>
+    postProcessHTML(pathname, html, renderOpts, { inAmpMode, hybridAmp })
+
+  if (generateStaticHTML) {
+    const html = await streamToString(chainStreams(streams))
+    const optimizedHtml = await postOptimize(html)
+    return new RenderResult(optimizedHtml)
   }
 
   return new RenderResult(
-    chainStreams(streams).pipeThrough(createBufferedTransformStream())
+    chainStreams(streams).pipeThrough(
+      createBufferedTransformStream(postOptimize)
+    )
   )
 }