fix: populate static API routes for our staticRouteMatcher (opennextjs#875)

* fix: populate static api routes for our staticRouteMatcher

* add e2e

* changeset

* fix unit test

* rm api route from RoutesManifest in mock

* improve e2e

* test file

* refactor test title

* lint

* revert sst config

* review

* unit test

* rm console log

* rm key from config in unit test

* rm comment

* migrate to pages-router

* migrate test

* fix e2e

* remove skip

* fix value in api test

* review

Author: sommeeeer

.changeset/famous-kids-hope.md

@@ -0,0 +1,5 @@
+---
+"@opennextjs/aws": patch
+---
+
+fix: populate static API routes for our staticRouteMatcher

examples/pages-router/src/pages/index.tsx renamed to examples/pages-router/src/components/home.tsx

@@ -1,15 +1,6 @@
 import Nav from "@example/shared/components/Nav";
 import Head from "next/head";
 
-// Not used, but necessary to get prefetching to work
-export function getStaticProps() {
-  return {
-    props: {
-      hello: "world",
-    },
-  };
-}
-
 export default function Home() {
   return (
     <>

examples/pages-router/src/pages/[[...page]].tsx

@@ -0,0 +1,69 @@
+import Home from "@/components/home";
+import type {
+  GetStaticPathsResult,
+  GetStaticPropsContext,
+  InferGetStaticPropsType,
+} from "next";
+
+const validRootPages = ["conico974", "kheuzy", "sommeeer"];
+const validLongPaths = ["super/long/path/to/secret/page"];
+
+export async function getStaticPaths(): Promise<GetStaticPathsResult> {
+  const rootPaths = validRootPages.map((page) => ({
+    params: { page: [page] },
+  }));
+
+  const longPaths = validLongPaths.map((path) => ({
+    params: { page: path.split("/") },
+  }));
+
+  const paths = [{ params: { page: [] } }, ...rootPaths, ...longPaths];
+
+  return {
+    paths,
+    fallback: false,
+  };
+}
+
+export async function getStaticProps(context: GetStaticPropsContext) {
+  const page = (context.params?.page as string[]) || [];
+
+  if (page.length === 0) {
+    return {
+      props: {
+        subpage: [],
+        pageType: "home",
+      },
+    };
+  }
+  if (page.length === 1 && validRootPages.includes(page[0])) {
+    return {
+      props: {
+        subpage: page,
+        pageType: "root",
+      },
+    };
+  }
+
+  const pagePath = page.join("/");
+  if (validLongPaths.includes(pagePath)) {
+    return { props: { subpage: page, pageType: "long-path" } };
+  }
+  return { notFound: true };
+}
+
+export default function Page({
+  subpage,
+  pageType,
+}: InferGetStaticPropsType<typeof getStaticProps>) {
+  if (subpage.length === 0 && pageType === "home") {
+    return <Home />;
+  }
+  return (
+    <div>
+      <h1 data-testid="page">{`Page: ${subpage}`}</h1>
+      <p>Page type: {pageType}</p>
+      <p>Path: {subpage.join("/")}</p>
+    </div>
+  );
+}

packages/open-next/src/adapters/config/index.ts

@@ -11,6 +11,7 @@ import {
   loadFunctionsConfigManifest,
   loadHtmlPages,
   loadMiddlewareManifest,
+  loadPagesManifest,
   loadPrerenderManifest,
   loadRoutesManifest,
 } from "./util.js";
@@ -20,7 +21,6 @@ export const OPEN_NEXT_DIR = path.join(__dirname, ".open-next");
 
 debug({ NEXT_DIR, OPEN_NEXT_DIR });
 
-//TODO: inject these values at build time
 export const NextConfig = /* @__PURE__ */ loadConfig(NEXT_DIR);
 export const BuildId = /* @__PURE__ */ loadBuildId(NEXT_DIR);
 export const HtmlPages = /* @__PURE__ */ loadHtmlPages(NEXT_DIR);
@@ -29,6 +29,7 @@ export const RoutesManifest = /* @__PURE__ */ loadRoutesManifest(NEXT_DIR);
 export const ConfigHeaders = /* @__PURE__ */ loadConfigHeaders(NEXT_DIR);
 export const PrerenderManifest =
   /* @__PURE__ */ loadPrerenderManifest(NEXT_DIR);
+export const PagesManifest = /* @__PURE__ */ loadPagesManifest(NEXT_DIR);
 export const AppPathsManifestKeys =
   /* @__PURE__ */ loadAppPathsManifestKeys(NEXT_DIR);
 export const MiddlewareManifest =

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

@@ -24,7 +24,7 @@ export function loadBuildId(nextDir: string) {
 export function loadPagesManifest(nextDir: string) {
   const filePath = path.join(nextDir, "server/pages-manifest.json");
   const json = fs.readFileSync(filePath, "utf-8");
-  return JSON.parse(json);
+  return JSON.parse(json) as Record<string, string>;
 }
 
 export function loadHtmlPages(nextDir: string) {

packages/open-next/src/core/routing/matcher.ts

@@ -437,7 +437,12 @@ export function handleFallbackFalse(
     ? rawPath
     : `/${NextConfig.i18n?.defaultLocale}${rawPath}`;
   // We need to remove the trailing slash if it exists
-  if (NextConfig.trailingSlash && localizedPath.endsWith("/")) {
+  if (
+    // Not if localizedPath is "/" tho, because that would not make it find `isPregenerated` below since it would be try to match an empty string.
+    localizedPath !== "/" &&
+    NextConfig.trailingSlash &&
+    localizedPath.endsWith("/")
+  ) {
     localizedPath = localizedPath.slice(0, -1);
   }
   const matchedStaticRoute = staticRouteMatcher(localizedPath);
@@ -447,6 +452,7 @@ export function handleFallbackFalse(
   const matchedDynamicRoute = dynamicRouteMatcher(localizedPath).filter(
     ({ route }) => !prerenderedFallbackRoutesName.includes(route),
   );
+
   const isPregenerated = Object.keys(routes).includes(localizedPath);
   if (
     routeFallback &&

packages/open-next/src/core/routing/routeMatcher.ts

@@ -1,4 +1,8 @@
-import { AppPathRoutesManifest, RoutesManifest } from "config/index";
+import {
+  AppPathRoutesManifest,
+  PagesManifest,
+  RoutesManifest,
+} from "config/index";
 import type { RouteDefinition } from "types/next-types";
 import type { ResolvedRoute, RouteType } from "types/open-next";
 
@@ -10,9 +14,6 @@ const optionalBasepathPrefixRegex = RoutesManifest.basePath
   ? `^${RoutesManifest.basePath}/?`
   : "^/";
 
-// Add the basePath prefix to the api routes
-export const apiPrefix = `${RoutesManifest.basePath ?? ""}/api`;
-
 const optionalPrefix = optionalLocalePrefixRegex.replace(
   "^/",
   optionalBasepathPrefixRegex,
@@ -53,5 +54,42 @@ function routeMatcher(routeDefinitions: RouteDefinition[]) {
   };
 }
 
-export const staticRouteMatcher = routeMatcher(RoutesManifest.routes.static);
+export const staticRouteMatcher = routeMatcher([
+  ...RoutesManifest.routes.static,
+  ...getStaticAPIRoutes(),
+]);
 export const dynamicRouteMatcher = routeMatcher(RoutesManifest.routes.dynamic);
+
+/**
+ * Returns static API routes for both app and pages router cause Next will filter them out in staticRoutes in `routes-manifest.json`.
+ * We also need to filter out page files that are under `app/api/*` as those would not be present in the routes manifest either.
+ * This line from Next.js skips it:
+ * https://github.com/vercel/next.js/blob/ded56f952154a40dcfe53bdb38c73174e9eca9e5/packages/next/src/build/index.ts#L1299
+ *
+ * Without it handleFallbackFalse will 404 on static API routes if there is a catch-all route on root level.
+ */
+function getStaticAPIRoutes(): RouteDefinition[] {
+  const createRouteDefinition = (route: string) => ({
+    page: route,
+    regex: `^${route}(?:/)?$`,
+  });
+  const dynamicRoutePages = new Set(
+    RoutesManifest.routes.dynamic.map(({ page }) => page),
+  );
+  const pagesStaticAPIRoutes = Object.keys(PagesManifest)
+    .filter(
+      (route) => route.startsWith("/api/") && !dynamicRoutePages.has(route),
+    )
+    .map(createRouteDefinition);
+
+  // We filter out both static API and page routes from the app paths manifest
+  const appPathsStaticAPIRoutes = Object.values(AppPathRoutesManifest)
+    .filter(
+      (route) =>
+        route.startsWith("/api/") ||
+        (route === "/api" && !dynamicRoutePages.has(route)),
+    )
+    .map(createRouteDefinition);
+
+  return [...pagesStaticAPIRoutes, ...appPathsStaticAPIRoutes];
+}

packages/open-next/src/core/routingHandler.ts

@@ -24,7 +24,6 @@ import {
 } from "./routing/matcher";
 import { handleMiddleware } from "./routing/middleware";
 import {
-  apiPrefix,
   dynamicRouteMatcher,
   staticRouteMatcher,
 } from "./routing/routeMatcher";
@@ -143,12 +142,17 @@ export default async function routingHandler(
       isExternalRewrite = afterRewrites.isExternalRewrite;
     }
 
+    let isISR = false;
     // We want to run this just before the dynamic route check
-    const { event: fallbackEvent, isISR } = handleFallbackFalse(
-      internalEvent,
-      PrerenderManifest,
-    );
-    internalEvent = fallbackEvent;
+    // We can skip it if its an external rewrite
+    if (!isExternalRewrite) {
+      const fallbackResult = handleFallbackFalse(
+        internalEvent,
+        PrerenderManifest,
+      );
+      internalEvent = fallbackResult.event;
+      isISR = fallbackResult.isISR;
+    }
 
     const foundDynamicRoute = dynamicRouteMatcher(internalEvent.rawPath);
     const isDynamicRoute = !isExternalRewrite && foundDynamicRoute.length > 0;
@@ -163,13 +167,6 @@ export default async function routingHandler(
       isExternalRewrite = fallbackRewrites.isExternalRewrite;
     }
 
-    // Api routes are not present in the routes manifest except if they're not behind /api
-    // /api even if it's a page route doesn't get generated in the manifest
-    // Ideally we would need to properly check api routes here
-    const isApiRoute =
-      internalEvent.rawPath === apiPrefix ||
-      internalEvent.rawPath.startsWith(`${apiPrefix}/`);
-
     const isNextImageRoute = internalEvent.rawPath.startsWith("/_next/image");
 
     const isRouteFoundBeforeAllRewrites =
@@ -180,7 +177,6 @@ export default async function routingHandler(
     if (
       !(
         isRouteFoundBeforeAllRewrites ||
-        isApiRoute ||
         isNextImageRoute ||
         // We need to check again once all rewrites have been applied
         staticRouteMatcher(internalEvent.rawPath).length > 0 ||

packages/tests-e2e/tests/pagesRouter/catch-all-optional.test.ts

@@ -0,0 +1,31 @@
+import { expect, test } from "@playwright/test";
+
+// Going to `/`, `/conico974`, `/kheuzy` and `/sommeeer` should be catched by our `[[...page]]` route.
+// Also the /super/long/path/to/secret/page should be pregenerated by the `getStaticPaths` function.
+test.describe("Catch-all optional route in root should work", () => {
+  test("should be possible to visit home and a pregenerated subpage", async ({
+    page,
+  }) => {
+    await page.goto("/");
+    await page.locator("h1").getByText("Pages Router").isVisible();
+
+    await page.goto("/conico974");
+    const pElement = page.getByText("Path: conico974", { exact: true });
+    await pElement.isVisible();
+  });
+  test("should be possible to visit a long pregenerated path", async ({
+    page,
+  }) => {
+    await page.goto("/super/long/path/to/secret/page");
+    const h1Text = await page.getByTestId("page").textContent();
+    expect(h1Text).toBe("Page: super,long,path,to,secret,page");
+  });
+  test("should be possible to request an API route when you have a catch-all in root", async ({
+    request,
+  }) => {
+    const response = await request.get("/api/hello");
+    expect(response.status()).toBe(200);
+    const body = await response.json();
+    expect(body).toEqual({ hello: "OpenNext rocks!" });
+  });
+});

packages/tests-e2e/tests/pagesRouter/data.test.ts

@@ -17,5 +17,8 @@ test("fix _next/data", async ({ page }) => {
   expect(response2.request().url()).toMatch(/\/_next\/data\/.*\/en\.json$/);
   await page.waitForURL("/");
   const body = await response2.json();
-  expect(body).toEqual({ pageProps: { hello: "world" }, __N_SSG: true });
+  expect(body).toEqual({
+    pageProps: { subpage: [], pageType: "home" },
+    __N_SSG: true,
+  });
 });