Pages Error Route Module Rendering (#51374)

Continuing on changes to move the rendering logic into the bundles. - Unifies edge render functions: `appRenderToHTML` and `pagesRenderToHTML` becomes `renderToHTML` - When an error occurs, the error module's match is used so the module's internal render can be used (easing transition away from using the server's render method) - Improved test resiliance
2023-06-22 09:13:13 -06:00 · 2023-06-22 09:13:13 -06:00 · 5871b0d6d7
commit 5871b0d6d7
parent 5127c33fc9
7 changed files with 243 additions and 153 deletions
--- a/packages/next/src/build/webpack/loaders/next-edge-ssr-loader/index.ts
+++ b/packages/next/src/build/webpack/loaders/next-edge-ssr-loader/index.ts
@ -116,17 +116,16 @@ const edgeSSRLoader: webpack.LoaderDefinitionFunction<EdgeSSRLoaderQuery> =
    ${
      isAppDir
        ? `
-      import { renderToHTMLOrFlight as appRenderToHTML } from 'next/dist/esm/server/app-render/app-render'
+      import { renderToHTMLOrFlight as renderToHTML } from 'next/dist/esm/server/app-render/app-render'
      import * as pageMod from ${JSON.stringify(pageModPath)}
      const Document = null
-      const pagesRenderToHTML = null
      const appMod = null
      const errorMod = null
      const error500Mod = null
    `
        : `
      import Document from ${stringifiedDocumentPath}
-      import { renderToHTML as pagesRenderToHTML } from 'next/dist/esm/server/render'
+      import { renderToHTML } from 'next/dist/esm/server/render'
      import * as pageMod from ${stringifiedPagePath}
      import * as appMod from ${stringifiedAppPath}
      import * as errorMod from ${stringifiedErrorPath}
@ -135,7 +134,6 @@ const edgeSSRLoader: webpack.LoaderDefinitionFunction<EdgeSSRLoaderQuery> =
          ? `import * as error500Mod from ${stringified500Path}`
          : `const error500Mod = null`
      }
-      const appRenderToHTML = null
    `
    }

@ -171,8 +169,7 @@ const edgeSSRLoader: webpack.LoaderDefinitionFunction<EdgeSSRLoaderQuery> =
      buildManifest,
      isAppPath: ${!!isAppDir},
      prerenderManifest,
-      appRenderToHTML,
-      pagesRenderToHTML,
+      renderToHTML,
      reactLoadableManifest,
      clientReferenceManifest: ${isServerComponent} ? rscManifest : null,
      serverActionsManifest: ${isServerComponent} ? rscServerManifest : null,
--- a/packages/next/src/build/webpack/loaders/next-edge-ssr-loader/render.ts
+++ b/packages/next/src/build/webpack/loaders/next-edge-ssr-loader/render.ts
@ -27,8 +27,7 @@ export function getRender({
  buildManifest,
  prerenderManifest,
  reactLoadableManifest,
-  appRenderToHTML,
-  pagesRenderToHTML,
+  renderToHTML,
  clientReferenceManifest,
  subresourceIntegrityManifest,
  serverActionsManifest,
@ -44,8 +43,7 @@ export function getRender({
  pageMod: any
  errorMod: any
  error500Mod: any
-  appRenderToHTML: any
-  pagesRenderToHTML: any
+  renderToHTML: any
  Document: DocumentType
  buildManifest: BuildManifest
  prerenderManifest: PrerenderManifest
@ -88,8 +86,7 @@ export function getRender({
        clientReferenceManifest,
        serverActionsManifest,
      },
-      appRenderToHTML,
-      pagesRenderToHTML,
+      renderToHTML,
      incrementalCacheHandler,
      loadComponent: async (pathname) => {
        if (pathname === page) {
@ -137,12 +134,15 @@ export function getRender({
      },
    },
  })
-  const requestHandler = server.getRequestHandler()
+
+  const handler = server.getRequestHandler()

  return async function render(request: Request) {
    const extendedReq = new WebNextRequest(request)
    const extendedRes = new WebNextResponse()
-    requestHandler(extendedReq, extendedRes)
+
+    handler(extendedReq, extendedRes)
+
    return await extendedRes.toResponse()
  }
 }
--- a/packages/next/src/server/base-server.ts
+++ b/packages/next/src/server/base-server.ts
@ -61,7 +61,11 @@ import * as Log from '../build/output/log'
 import escapePathDelimiters from '../shared/lib/router/utils/escape-path-delimiters'
 import { getUtils } from './server-utils'
 import isError, { getProperError } from '../lib/is-error'
-import { addRequestMeta, getRequestMeta } from './request-meta'
+import {
+  addRequestMeta,
+  getRequestMeta,
+  removeRequestMeta,
+} from './request-meta'

 import { ImageConfigComplete } from '../shared/lib/image-config'
 import { removePathPrefix } from '../shared/lib/router/utils/remove-path-prefix'
@ -1670,11 +1674,8 @@ export default abstract class Server<ServerOptions extends Options = Options> {
      // served by the server.
      let result: RenderResult

-      // We want to use the match when we're not trying to render an error page.
-      const match =
-        pathname !== '/_error' && !is404Page && !is500Page
-          ? getRequestMeta(req, '_nextMatch')
-          : undefined
+      // Get the match for the page if it exists.
+      const match = getRequestMeta(req, '_nextMatch')

      if (
        match &&
@ -1716,12 +1717,9 @@ export default abstract class Server<ServerOptions extends Options = Options> {
            if (!headers['content-type'] && blob.type) {
              headers['content-type'] = blob.type
            }
-            let revalidate: number | false | undefined =
-              context.staticGenerationContext.store?.revalidate

-            if (typeof revalidate == 'undefined') {
-              revalidate = false
-            }
+            const revalidate =
+              context.staticGenerationContext.store?.revalidate ?? false

            // Create the cache entry for the response.
            const cacheEntry: ResponseCacheEntry = {
@ -2540,6 +2538,17 @@ export default abstract class Server<ServerOptions extends Options = Options> {
        )
      }

+      // If the page has a route module, use it for the new match. If it doesn't
+      // have a route module, remove the match.
+      if (result.components.ComponentMod.routeModule) {
+        addRequestMeta(ctx.req, '_nextMatch', {
+          definition: result.components.ComponentMod.routeModule.definition,
+          params: undefined,
+        })
+      } else {
+        removeRequestMeta(ctx.req, '_nextMatch')
+      }
+
      try {
        return await this.renderToResponseWithComponents(
          {
--- a/packages/next/src/server/request-meta.ts
+++ b/packages/next/src/server/request-meta.ts
@ -42,6 +42,14 @@ export interface RequestMeta {
  _nextMinimalMode?: boolean
 }

+/**
+ * Gets the request metadata. If no key is provided, the entire metadata object
+ * is returned.
+ *
+ * @param req the request to get the metadata from
+ * @param key the key to get from the metadata (optional)
+ * @returns the value for the key or the entire metadata object
+ */
 export function getRequestMeta(
  req: NextIncomingMessage,
  key?: undefined
@ -58,11 +66,26 @@ export function getRequestMeta<K extends keyof RequestMeta>(
  return typeof key === 'string' ? meta[key] : meta
 }

+/**
+ * Sets the request metadata.
+ *
+ * @param req the request to set the metadata on
+ * @param meta the metadata to set
+ * @returns the mutated request metadata
+ */
 export function setRequestMeta(req: NextIncomingMessage, meta: RequestMeta) {
  req[NEXT_REQUEST_META] = meta
-  return getRequestMeta(req)
+  return meta
 }

+/**
+ * Adds a value to the request metadata.
+ *
+ * @param request the request to mutate
+ * @param key the key to set
+ * @param value the value to set
+ * @returns the mutated request metadata
+ */
 export function addRequestMeta<K extends keyof RequestMeta>(
  request: NextIncomingMessage,
  key: K,
@ -73,6 +96,22 @@ export function addRequestMeta<K extends keyof RequestMeta>(
  return setRequestMeta(request, meta)
 }

+/**
+ * Removes a key from the request metadata.
+ *
+ * @param request the request to mutate
+ * @param key the key to remove
+ * @returns the mutated request metadata
+ */
+export function removeRequestMeta<K extends keyof RequestMeta>(
+  request: NextIncomingMessage,
+  key: K
+) {
+  const meta = getRequestMeta(request)
+  delete meta[key]
+  return setRequestMeta(request, meta)
+}
+
 type NextQueryMetadata = {
  __nextNotFoundSrcPage?: string
  __nextDefaultLocale?: string
--- a/packages/next/src/server/web-server.ts
+++ b/packages/next/src/server/web-server.ts
@ -35,8 +35,9 @@ interface WebServerOptions extends Options {
    ) => Promise<LoadComponentsReturnType | null>
    extendRenderOpts: Partial<BaseServer['renderOpts']> &
      Pick<BaseServer['renderOpts'], 'buildId'>
-    pagesRenderToHTML?: typeof import('./render').renderToHTML
-    appRenderToHTML?: typeof import('./app-render/app-render').renderToHTMLOrFlight
+    renderToHTML:
+      | typeof import('./render').renderToHTML
+      | typeof import('./app-render/app-render').renderToHTMLOrFlight
    incrementalCacheHandler?: any
    prerenderManifest: PrerenderManifest | undefined
  }
@ -362,32 +363,27 @@ export default class NextWebServer extends BaseServer<WebServerOptions> {
    return false
  }

-  protected async renderHTML(
+  protected renderHTML(
    req: WebNextRequest,
    res: WebNextResponse,
    pathname: string,
    query: NextParsedUrlQuery,
    renderOpts: RenderOpts
  ): Promise<RenderResult> {
-    const { pagesRenderToHTML, appRenderToHTML } =
-      this.serverOptions.webServerConfig
-    const curRenderToHTML = pagesRenderToHTML || appRenderToHTML
+    const { renderToHTML } = this.serverOptions.webServerConfig

-    if (curRenderToHTML) {
-      return await curRenderToHTML(
-        req as any,
-        res as any,
-        pathname,
-        query,
-        Object.assign(renderOpts, {
-          disableOptimizedLoading: true,
-          runtime: 'experimental-edge',
-        })
-      )
-    } else {
-      throw new Error(`Invariant: curRenderToHTML is missing`)
-    }
+    return renderToHTML(
+      req as any,
+      res as any,
+      pathname,
+      query,
+      Object.assign(renderOpts, {
+        disableOptimizedLoading: true,
+        runtime: 'experimental-edge',
+      })
+    )
  }
+
  protected async sendRenderResult(
    _req: WebNextRequest,
    res: WebNextResponse,
--- a/test/e2e/app-dir/app-css/index.test.ts
+++ b/test/e2e/app-dir/app-css/index.test.ts
@ -21,56 +21,68 @@ createNextDescribe(
          const browser = await next.browser('/dashboard')

          // Should body text in red
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('.p')).color`
-            )
-          ).toBe('rgb(255, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('.p')).color`
+              ),
+            'rgb(255, 0, 0)'
+          )

          // Should inject global css for .green selectors
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('.green')).color`
-            )
-          ).toBe('rgb(0, 128, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('.green')).color`
+              ),
+            'rgb(0, 128, 0)'
+          )
        })

        it('should support css modules inside server layouts', async () => {
          const browser = await next.browser('/css/css-nested')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('#server-cssm')).color`
-            )
-          ).toBe('rgb(0, 128, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('#server-cssm')).color`
+              ),
+            'rgb(0, 128, 0)'
+          )
        })

        it('should support external css imports', async () => {
          const browser = await next.browser('/css/css-external')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('main')).paddingTop`
-            )
-          ).toBe('80px')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('main')).paddingTop`
+              ),
+            '80px'
+          )
        })
      })

      describe('server pages', () => {
        it('should support global css inside server pages', async () => {
          const browser = await next.browser('/css/css-page')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('h1')).color`
-            )
-          ).toBe('rgb(255, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('h1')).color`
+              ),
+            'rgb(255, 0, 0)'
+          )
        })

        it('should support css modules inside server pages', async () => {
          const browser = await next.browser('/css/css-page')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('#cssm')).color`
-            )
-          ).toBe('rgb(0, 0, 255)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('#cssm')).color`
+              ),
+            'rgb(0, 0, 255)'
+          )
        })

        it('should not contain pages css in app dir page', async () => {
@ -84,22 +96,26 @@ createNextDescribe(
          const browser = await next.browser('/client-nested')

          // Should render h1 in red
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('h1')).color`
-            )
-          ).toBe('rgb(255, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('h1')).color`
+              ),
+            'rgb(255, 0, 0)'
+          )
        })

        it('should support global css inside client layouts', async () => {
          const browser = await next.browser('/client-nested')

          // Should render button in red
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('button')).color`
-            )
-          ).toBe('rgb(255, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('button')).color`
+              ),
+            'rgb(255, 0, 0)'
+          )
        })
      })

@ -108,22 +124,26 @@ createNextDescribe(
          const browser = await next.browser('/client-component-route')

          // Should render p in red
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('p')).color`
-            )
-          ).toBe('rgb(255, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('p')).color`
+              ),
+            'rgb(255, 0, 0)'
+          )
        })

        it('should support global css inside client pages', async () => {
          const browser = await next.browser('/client-component-route')

          // Should render `b` in blue
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('b')).color`
-            )
-          ).toBe('rgb(0, 0, 255)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('b')).color`
+              ),
+            'rgb(0, 0, 255)'
+          )
        })
      })

@ -131,21 +151,25 @@ createNextDescribe(
        it('should support css modules inside client page', async () => {
          const browser = await next.browser('/css/css-client')

-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('#css-modules')).fontSize`
-            )
-          ).toBe('100px')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('#css-modules')).fontSize`
+              ),
+            '100px'
+          )
        })

        it('should support css modules inside client components', async () => {
          const browser = await next.browser('/css/css-client/inner')

-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('#client-component')).fontSize`
-            )
-          ).toBe('100px')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('#client-component')).fontSize`
+              ),
+            '100px'
+          )
        })
      })

@ -160,61 +184,75 @@ createNextDescribe(

        it('should include css imported in client template.js', async () => {
          const browser = await next.browser('/template/clientcomponent')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('button')).fontSize`
-            )
-          ).toBe('100px')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('button')).fontSize`
+              ),
+            '100px'
+          )
        })

        it('should include css imported in server template.js', async () => {
          const browser = await next.browser('/template/servercomponent')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('h1')).color`
-            )
-          ).toBe('rgb(255, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('h1')).color`
+              ),
+            'rgb(255, 0, 0)'
+          )
        })

        it('should include css imported in client not-found.js', async () => {
          const browser = await next.browser('/not-found/clientcomponent')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('h1')).color`
-            )
-          ).toBe('rgb(255, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('h1')).color`
+              ),
+            'rgb(255, 0, 0)'
+          )
        })

        it('should include css imported in server not-found.js', async () => {
          const browser = await next.browser('/not-found/servercomponent')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('h1')).color`
-            )
-          ).toBe('rgb(255, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('h1')).color`
+              ),
+            'rgb(255, 0, 0)'
+          )
        })

        it('should include root layout css for root not-found.js', async () => {
          const browser = await next.browser('/this-path-does-not-exist')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('h1')).color`
-            )
-          ).toBe('rgb(210, 105, 30)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('h1')).color`
+              ),
+            'rgb(210, 105, 30)'
+          )
        })

        it('should include css imported in root not-found.js', async () => {
          const browser = await next.browser('/random-non-existing-path')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('h1')).color`
-            )
-          ).toBe('rgb(210, 105, 30)')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('h1')).backgroundColor`
-            )
-          ).toBe('rgb(0, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('h1')).color`
+              ),
+            'rgb(210, 105, 30)'
+          )
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('h1')).backgroundColor`
+              ),
+            'rgb(0, 0, 0)'
+          )
        })

        it('should include css imported in error.js', async () => {
@ -224,22 +262,26 @@ createNextDescribe(
          // Wait for error page to render and CSS to be loaded
          await new Promise((resolve) => setTimeout(resolve, 2000))

-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('button')).fontSize`
-            )
-          ).toBe('50px')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('button')).fontSize`
+              ),
+            '50px'
+          )
        })
      })

      describe('page extensions', () => {
        it('should include css imported in MDX pages', async () => {
          const browser = await next.browser('/mdx')
-          expect(
-            await browser.eval(
-              `window.getComputedStyle(document.querySelector('h1')).color`
-            )
-          ).toBe('rgb(255, 0, 0)')
+          await check(
+            async () =>
+              await browser.eval(
+                `window.getComputedStyle(document.querySelector('h1')).color`
+              ),
+            'rgb(255, 0, 0)'
+          )
        })
      })

--- a/test/lib/next-test-utils.js
+++ b/test/lib/next-test-utils.js
@ -535,8 +535,15 @@ export async function startCleanStaticServer(dir) {
  return server
 }

-// check for content in 1 second intervals timing out after
-// 30 seconds
+/**
+ * Check for content in 1 second intervals timing out after 30 seconds.
+ *
+ * @param {() => Promise<unknown> | unknown} contentFn
+ * @param {RegExp | string | number} regex
+ * @param {boolean} hardError
+ * @param {number} maxRetries
+ * @returns {Promise<boolean>}
+ */
 export async function check(
  contentFn,
  regex,