Route Module Cleanup (#50936)

This cleans up some setup functions within the route modules, improves logic around headers, and handles the case where the URL is invalid better for routes using the request adapters.

packages/next/src/server/base-server.ts

@@ -11,11 +11,7 @@ import type { NextConfig, NextConfigComplete } from './config-shared'
 import type { NextParsedUrlQuery, NextUrlWithParsedQuery } from './request-meta'
 import type { ParsedUrlQuery } from 'querystring'
 import type { RenderOpts, RenderOptsPartial } from './render'
-import type {
-  ResponseCacheBase,
-  ResponseCacheEntry,
-  ResponseCacheValue,
-} from './response-cache'
+import type { ResponseCacheBase, ResponseCacheEntry } from './response-cache'
 import type { UrlWithParsedQuery } from 'url'
 import {
   NormalizeError,
@@ -1787,9 +1783,7 @@ export default abstract class Server<ServerOptions extends Options = Options> {
         result = await module.render(
           (req as NodeNextRequest).originalRequest,
           (res as NodeNextResponse).originalResponse,
-          pathname,
-          query,
-          renderOpts
+          { page: pathname, params: match.params, query, renderOpts }
         )
       } else {
         // If we didn't match a page, we should fallback to using the legacy
@@ -1841,23 +1835,40 @@ export default abstract class Server<ServerOptions extends Options = Options> {
         throw err
       }
 
-      let value: ResponseCacheValue | null
+      // Based on the metadata, we can determine what kind of cache result we
+      // should return.
+
+      // Handle `isNotFound`.
       if (metadata.isNotFound) {
-        value = null
-      } else if (metadata.isRedirect) {
-        value = { kind: 'REDIRECT', props: metadata.pageData }
-      } else {
-        if (result.isNull()) {
-          return null
+        return { value: null, revalidate: metadata.revalidate }
+      }
+
+      // Handle `isRedirect`.
+      if (metadata.isRedirect) {
+        return {
+          value: {
+            kind: 'REDIRECT',
+            props: metadata.pageData,
+          },
+          revalidate: metadata.revalidate,
         }
-        value = {
+      }
+
+      // Handle `isNull`.
+      if (result.isNull()) {
+        return { value: null, revalidate: metadata.revalidate }
+      }
+
+      // We now have a valid HTML result that we can return to the user.
+      return {
+        value: {
           kind: 'PAGE',
           html: result,
           pageData: metadata.pageData,
           headers,
-        }
+        },
+        revalidate: metadata.revalidate,
       }
-      return { revalidate: metadata.revalidate, value }
     }
 
     const cacheEntry = await this.responseCache.get(

packages/next/src/server/future/route-handler-managers/route-handler-manager.ts

@@ -32,11 +32,6 @@ export class RouteHandlerManager {
       this.moduleLoader
     )
 
-    // Setup the handler. It is the responsibility of the module to ensure that
-    // this is only called once. If this is in development mode, the require
-    // cache will be cleared and the module will be re-created.
-    module.setup()
-
     // Convert the BaseNextRequest to a NextRequest.
     const request = NextRequestAdapter.fromBaseNextRequest(req)
 

packages/next/src/server/future/route-modules/app-route/module.ts

@@ -187,27 +187,6 @@ export class AppRouteRouteModule extends RouteModule<
         )
       }
     }
-  }
-
-  /**
-   * When true, indicates that the global interfaces have been patched via the
-   * `patch()` method.
-   */
-  private hasSetup: boolean = false
-
-  /**
-   * Validates the userland module to ensure the exported methods and properties
-   * are valid.
-   */
-  public async setup() {
-    // If we've already setup, then return.
-    if (this.hasSetup) return
-
-    // Mark the module as setup. The following warnings about the userland
-    // module will run if we're in development. If the module files are modified
-    // when in development, then the require cache will be busted for it and
-    // this method will be called again (resetting the `hasSetup` flag).
-    this.hasSetup = true
 
     // We only warn in development after here, so return if we're not in
     // development.

packages/next/src/server/future/route-modules/pages/module.ts

@@ -11,7 +11,11 @@ import type { NextParsedUrlQuery } from '../../../request-meta'
 import type { RenderOpts } from '../../../render'
 import type RenderResult from '../../../render-result'
 
-import { RouteModule, type RouteModuleOptions } from '../route-module'
+import {
+  RouteModule,
+  type RouteModuleHandleContext,
+  type RouteModuleOptions,
+} from '../route-module'
 import { renderToHTML } from '../../../render'
 
 /**
@@ -46,6 +50,29 @@ export type PagesUserlandModule = {
   readonly getServerSideProps?: GetServerSideProps
 }
 
+/**
+ * AppRouteRouteHandlerContext is the context that is passed to the route
+ * handler for app routes.
+ */
+export interface PagesRouteHandlerContext extends RouteModuleHandleContext {
+  /**
+   * The page for the given route.
+   */
+  page: string
+
+  /**
+   * The parsed URL query for the given request.
+   */
+  query: NextParsedUrlQuery
+
+  /**
+   * The RenderOpts for the given request which include the specific modules to
+   * use for rendering.
+   */
+  // TODO: (wyattjoh) break this out into smaller parts, it currently includes the userland components
+  renderOpts: RenderOpts
+}
+
 export type PagesRouteModuleOptions = RouteModuleOptions<
   PagesRouteDefinition,
   PagesUserlandModule
@@ -55,23 +82,22 @@ export class PagesRouteModule extends RouteModule<
   PagesRouteDefinition,
   PagesUserlandModule
 > {
-  public setup(): Promise<void> {
-    throw new Error('Method not implemented.')
-  }
-
   public handle(): Promise<Response> {
     throw new Error('Method not implemented.')
   }
 
-  public async render(
+  public render(
     req: IncomingMessage,
     res: ServerResponse,
-    pathname: string,
-    query: NextParsedUrlQuery,
-    renderOpts: RenderOpts
+    context: PagesRouteHandlerContext
   ): Promise<RenderResult> {
-    const result = await renderToHTML(req, res, pathname, query, renderOpts)
-    return result
+    return renderToHTML(
+      req,
+      res,
+      context.page,
+      context.query,
+      context.renderOpts
+    )
   }
 }
 

packages/next/src/server/future/route-modules/route-module.ts

@@ -45,13 +45,6 @@ export abstract class RouteModule<
    */
   public readonly definition: Readonly<D>
 
-  /**
-   * Setup will setup the route handler. This could patch any globals or perform
-   * validation of the userland module. It is the responsibility of the module
-   * to ensure that this is only called once.
-   */
-  public abstract setup(): Promise<void>
-
   /**
    * Handle will handle the request and return a response.
    */

packages/next/src/server/render-result.ts

@@ -12,13 +12,15 @@ export type RenderResultMetadata = {
   isRedirect?: boolean
 }
 
+type RenderResultResponse = string | ReadableStream<Uint8Array> | null
+
 export default class RenderResult {
-  private readonly _response: string | ReadableStream<Uint8Array> | null
+  private readonly _response: RenderResultResponse
   private readonly _contentType: ContentTypeOption
   private readonly _metadata: RenderResultMetadata
 
   constructor(
-    response: string | ReadableStream<Uint8Array> | null,
+    response: RenderResultResponse,
     {
       contentType,
       ...metadata
@@ -31,6 +33,10 @@ export default class RenderResult {
     this._metadata = metadata
   }
 
+  get body(): Readonly<RenderResultResponse> {
+    return this._response
+  }
+
   get metadata(): Readonly<RenderResultMetadata> {
     return this._metadata
   }

packages/next/src/server/render.tsx

@@ -75,7 +75,6 @@ import {
   streamFromArray,
   streamToString,
   chainStreams,
-  createBufferedTransformStream,
   renderToInitialStream,
   continueFromInitialStream,
 } from './stream-utils/node-web-streams-helper'
@@ -1177,8 +1176,6 @@ export async function renderToHTML(
     return inAmpMode ? children : <div id="__next">{children}</div>
   }
 
-  // Always disable streaming for pages rendering
-  const generateStaticHTML = true
   const renderDocument = async () => {
     // For `Document`, there are two cases that we don't support:
     // 1. Using `Document.getInitialProps` in the Edge runtime.
@@ -1316,7 +1313,7 @@ export async function renderToHTML(
         return continueFromInitialStream(initialStream, {
           suffix,
           dataStream: serverComponentsInlinedTransformStream?.readable,
-          generateStaticHTML,
+          generateStaticHTML: true,
           getServerInsertedHTML,
           serverInsertedHTMLToHead: false,
         })
@@ -1539,18 +1536,9 @@ export async function renderToHTML(
   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, renderResultMeta)
-  }
-
-  return new RenderResult(
-    chainStreams(streams).pipeThrough(
-      createBufferedTransformStream(postOptimize)
-    ),
-    renderResultMeta
-  )
+  const html = await streamToString(chainStreams(streams))
+  const optimizedHtml = await postOptimize(html)
+  return new RenderResult(optimizedHtml, renderResultMeta)
 }
 
 export type RenderToHTMLResult = typeof renderToHTML

packages/next/src/server/send-payload/revalidate-headers.ts

@@ -7,10 +7,10 @@ export function setRevalidateHeaders(
   options: PayloadOptions
 ) {
   if (options.private || options.stateful) {
-    if (options.private || !res.getHeader('Cache-Control')) {
+    if (options.private || !res.hasHeader('Cache-Control')) {
       res.setHeader(
         'Cache-Control',
-        `private, no-cache, no-store, max-age=0, must-revalidate`
+        'private, no-cache, no-store, max-age=0, must-revalidate'
       )
     }
   } else if (typeof options.revalidate === 'number') {
@@ -25,6 +25,6 @@ export function setRevalidateHeaders(
       `s-maxage=${options.revalidate}, stale-while-revalidate`
     )
   } else if (options.revalidate === false) {
-    res.setHeader('Cache-Control', `s-maxage=31536000, stale-while-revalidate`)
+    res.setHeader('Cache-Control', 's-maxage=31536000, stale-while-revalidate')
   }
 }

packages/next/src/server/send-response.ts

@@ -28,8 +28,10 @@ export async function sendResponse(
         for (const cookie of splitCookiesString(value)) {
           res.appendHeader(name, cookie)
         }
-      } else {
+      } else if (res.hasHeader(name)) {
         res.appendHeader(name, value)
+      } else {
+        res.setHeader(name, value)
       }
     })
 

packages/next/src/server/web/edge-route-module-wrapper.ts

@@ -62,10 +62,6 @@ export class EdgeRouteModuleWrapper {
   }
 
   private async handler(request: NextRequest): Promise<Response> {
-    // Setup the handler if it hasn't been setup yet. It is the responsibility
-    // of the module to ensure that this is only called once.
-    this.routeModule.setup()
-
     // Get the pathname for the matcher. Pathnames should not have trailing
     // slashes for matching.
     const pathname = removeTrailingSlash(new URL(request.url).pathname)

packages/next/src/server/web/spec-extension/adapters/next-request.ts

@@ -8,8 +8,7 @@ import { NextRequest } from '../request'
 
 export class NextRequestAdapter {
   public static fromBaseNextRequest(request: BaseNextRequest): NextRequest {
-    // TODO: look at refining this check
-    if ('request' in request && (request as WebNextRequest).request) {
+    if (request.constructor.name === 'WebNextRequest') {
       return NextRequestAdapter.fromWebNextRequest(request as WebNextRequest)
     }
 
@@ -30,9 +29,14 @@ export class NextRequestAdapter {
     } else {
       // Grab the full URL from the request metadata.
       const base = getRequestMeta(request, '__NEXT_INIT_URL')
-      if (!base) throw new Error('Invariant: missing url on request')
-
-      url = new URL(request.url, base)
+      if (!base || !base.startsWith('http')) {
+        // Because the URL construction relies on the fact that the URL provided
+        // is absolute, we need to provide a base URL. We can't use the request
+        // URL because it's relative, so we use a dummy URL instead.
+        url = new URL(request.url, 'http://n')
+      } else {
+        url = new URL(request.url, base)
+      }
     }
 
     return new NextRequest(url, {

test/integration/app-document-style-fragment/pages/index.js

@@ -11,6 +11,10 @@ function Hi() {
   )
 }
 
-Hi.getInitialProps = () => ({})
+Hi.getInitialProps = () => ({
+  // To prevent the warning related to an empty object from getInitialProps, we
+  // need to return something.
+  foo: 'bar',
+})
 
 export default Hi