fix: group routes react resolution (#131)

* fix: group routes react resolution

* Sync

---------

Co-authored-by: Frank <wangfanjie@gmail.com>

Author: conico974

.changeset/six-vans-decide.md

@@ -0,0 +1,5 @@
+---
+"open-next": patch
+---
+
+server: fix react resolution for group routes

packages/open-next/src/adapters/next-types.ts

@@ -36,7 +36,7 @@ export interface NextConfig {
   images: ImageConfig;
 }
 
-interface RouteDefinition {
+export interface RouteDefinition {
   page: string;
   regex: string;
 }

packages/open-next/src/adapters/require-hooks.ts

@@ -11,7 +11,17 @@ const resolveFilename = mod._resolveFilename;
 const hookPropertyMapApp = new Map();
 const hookPropertyMapPage = new Map();
 
-export function addHookAliases(
+export function overrideHooks(config: NextConfig) {
+  try {
+    overrideDefault();
+    overrideReact(config);
+  } catch (e) {
+    console.error("Failed to override Next.js require hooks.", e);
+    throw e;
+  }
+}
+
+function addHookAliases(
   aliases: [string, string][] = [],
   type: "app" | "page"
 ) {
@@ -23,7 +33,7 @@ export function addHookAliases(
 }
 
 // Add default aliases
-export function overrideDefault() {
+function overrideDefault() {
   addHookAliases(
     [
       // Use `require.resolve` explicitly to make them statically analyzable
@@ -37,7 +47,7 @@ export function overrideDefault() {
 }
 
 // Override built-in React packages if necessary
-export function overrideReact(config: NextConfig) {
+function overrideReact(config: NextConfig) {
   addHookAliases(
     [
       ["react", require.resolve(`react`)],
@@ -196,20 +206,22 @@ function isApp() {
   );
 }
 
-mod._resolveFilename = function (
-  originalResolveFilename: typeof resolveFilename,
-  requestMapApp: Map<string, string>,
-  requestMapPage: Map<string, string>,
-  request: string,
-  parent: any,
-  isMain: boolean,
-  options: any
-) {
-  const hookResolved = isApp()
-    ? requestMapApp.get(request)
-    : requestMapPage.get(request);
-  if (hookResolved) request = hookResolved;
-  return originalResolveFilename.call(mod, request, parent, isMain, options);
+export function applyOverride() {
+  mod._resolveFilename = function (
+    originalResolveFilename: typeof resolveFilename,
+    requestMapApp: Map<string, string>,
+    requestMapPage: Map<string, string>,
+    request: string,
+    parent: any,
+    isMain: boolean,
+    options: any
+  ) {
+    const hookResolved = isApp()
+      ? requestMapApp.get(request)
+      : requestMapPage.get(request);
+    if (hookResolved) request = hookResolved;
+    return originalResolveFilename.call(mod, request, parent, isMain, options);
 
-  // We use `bind` here to avoid referencing outside variables to create potential memory leaks.
-}.bind(null, resolveFilename, hookPropertyMapApp, hookPropertyMapPage);
+    // We use `bind` here to avoid referencing outside variables to create potential memory leaks.
+  }.bind(null, resolveFilename, hookPropertyMapApp, hookPropertyMapPage);
+}

packages/open-next/src/adapters/server-adapter.ts

@@ -8,7 +8,7 @@ import type {
 } from "aws-lambda";
 import {
   generateUniqueId,
-  loadAppPathsManifest,
+  loadAppPathsManifestKeys,
   loadConfig,
   loadHtmlPages,
   loadPublicAssets,
@@ -18,7 +18,10 @@ import {
 import { isBinaryContentType } from "./binary.js";
 import { debug } from "./logger.js";
 import { convertFrom, convertTo } from "./event-mapper.js";
-import { overrideDefault, overrideReact } from "./require-hooks.js";
+import {
+  overrideHooks as overrideNextjsRequireHooks,
+  applyOverride as applyNextjsRequireHooksOverride,
+} from "./require-hooks.js";
 import type { WarmerEvent, WarmerResponse } from "./warmer-function.js";
 
 const NEXT_DIR = path.join(__dirname, ".next");
@@ -30,17 +33,22 @@ setNextjsServerWorkingDirectory();
 const config = loadConfig(NEXT_DIR);
 const htmlPages = loadHtmlPages(NEXT_DIR);
 const routesManifest = loadRoutesManifest(NEXT_DIR);
-const appPathsManifest = loadAppPathsManifest(NEXT_DIR);
+const appPathsManifestKeys = loadAppPathsManifestKeys(NEXT_DIR);
 const publicAssets = loadPublicAssets(OPEN_NEXT_DIR);
 // Generate a 6 letter unique server ID
 const serverId = `server-${generateUniqueId()}`;
 
-// Need to override the require hooks for React before Next.js server
-// overrides them with prebundled ones in the case of app dir
+// WORKAROUND: Set `__NEXT_PRIVATE_PREBUNDLED_REACT` to use prebundled React — https://github.com/serverless-stack/open-next#workaround-set-__next_private_prebundled_react-to-use-prebundled-react
+// Step 1: Need to override the require hooks for React before Next.js server
+//         overrides them with prebundled ones in the case of app dir
+// Step 2: Import Next.js server
+// Step 3: Apply the override after Next.js server is imported since the
+//         override that Next.js does is done at import time
 overrideNextjsRequireHooks(config);
-
 // @ts-ignore
 import NextServer from "next/dist/server/next-server.js";
+applyNextjsRequireHooksOverride();
+
 const requestHandler = new NextServer.default({
   hostname: "localhost",
   port: Number(process.env.PORT) || 3000,
@@ -96,7 +104,7 @@ export async function handler(
   debug("IncomingMessage constructor props", reqProps);
   const req = new IncomingMessage(reqProps);
   const res = new ServerResponse({ method: reqProps.method });
-  setNextjsPrebundledReact(req, config);
+  setNextjsPrebundledReact(internalEvent.rawPath, config);
   await processRequest(req, res);
 
   // Format Next.js response to Lambda response
@@ -135,28 +143,16 @@ function setNextjsServerWorkingDirectory() {
   process.chdir(__dirname);
 }
 
-function overrideNextjsRequireHooks(config: any) {
-  // WORKAROUND: Set `__NEXT_PRIVATE_PREBUNDLED_REACT` to use prebundled React — https://github.com/serverless-stack/open-next#workaround-set-__next_private_prebundled_react-to-use-prebundled-react
-  try {
-    overrideDefault();
-    overrideReact(config);
-  } catch (e) {
-    console.error("Failed to override Next.js require hooks.", e);
-    throw e;
-  }
-}
-
-function setNextjsPrebundledReact(req: IncomingMessage, config: any) {
+function setNextjsPrebundledReact(rawPath: string, config: any) {
   // WORKAROUND: Set `__NEXT_PRIVATE_PREBUNDLED_REACT` to use prebundled React — https://github.com/serverless-stack/open-next#workaround-set-__next_private_prebundled_react-to-use-prebundled-react
 
   // Get route pattern
-  const route = [
-    ...routesManifest.staticRoutes,
-    ...routesManifest.dynamicRoutes,
-  ].find((route) => new RegExp(route.regex).test(req.url ?? ""));
+  const route = routesManifest.find((route) =>
+    new RegExp(route.regex).test(rawPath ?? "")
+  );
 
-  const isApp = appPathsManifest[`${route?.page}/page`];
-  debug("setNextjsPrebundledReact", { url: req.url, isApp });
+  const isApp = appPathsManifestKeys.includes(route?.page ?? "");
+  debug("setNextjsPrebundledReact", { url: rawPath, isApp, route });
 
   // app routes => use prebundled React
   if (isApp) {

packages/open-next/src/adapters/util.ts

@@ -35,10 +35,12 @@ export function loadPublicAssets(openNextDir: string) {
 export function loadRoutesManifest(nextDir: string) {
   const filePath = path.join(nextDir, "routes-manifest.json");
   const json = fs.readFileSync(filePath, "utf-8");
-  return JSON.parse(json) as RoutesManifest;
+  const routesManifest = JSON.parse(json) as RoutesManifest;
+  // Static routes take precedence over dynamic routes
+  return [...routesManifest.staticRoutes, ...routesManifest.dynamicRoutes];
 }
 
-export function loadAppPathsManifest(nextDir: string) {
+export function loadAppPathsManifestKeys(nextDir: string) {
   const appPathsManifestPath = path.join(
     nextDir,
     "server",
@@ -47,5 +49,14 @@ export function loadAppPathsManifest(nextDir: string) {
   const appPathsManifestJson = fs.existsSync(appPathsManifestPath)
     ? fs.readFileSync(appPathsManifestPath, "utf-8")
     : "{}";
-  return JSON.parse(appPathsManifestJson) as Record<string, string>;
+  const appPathsManifest = JSON.parse(appPathsManifestJson) as Record<
+    string,
+    string
+  >;
+  return Object.keys(appPathsManifest).map((key) => {
+    // Remove group route params and /page suffix
+    const cleanedKey = key.replace(/\/\(\w+\)|\/page$/g, "");
+    // We need to check if the cleaned key is empty because it means it's the root path
+    return cleanedKey === "" ? "/" : cleanedKey;
+  });
 }