Fix fallback false (opennextjs#858)

* fix 404 for fallback false

* fix fallback multiple dynamic route

* fix issue with trailing slash and better handling of dynamic fallback

* add e2e test

* changeset & lint

* fix unit-test

* update comment

Author: conico974

.changeset/large-forks-decide.md

@@ -0,0 +1,5 @@
+---
+"@opennextjs/aws": patch
+---
+
+fix 404 with fallback false on dynamic route

examples/pages-router/src/pages/fallback-intercepted/[...slugs].tsx

@@ -0,0 +1,20 @@
+import type { InferGetServerSidePropsType } from "next";
+
+export function getServerSideProps() {
+  return {
+    props: {
+      message: "This is a dynamic fallback page.",
+    },
+  };
+}
+
+export default function Page({
+  message,
+}: InferGetServerSidePropsType<typeof getServerSideProps>) {
+  return (
+    <div>
+      <h1>Dynamic Fallback Page</h1>
+      <p data-testid="message">{message}</p>
+    </div>
+  );
+}

examples/pages-router/src/pages/fallback-intercepted/[slug].tsx

@@ -0,0 +1,33 @@
+import type { InferGetStaticPropsType } from "next";
+
+export function getStaticPaths() {
+  return {
+    paths: [
+      {
+        params: {
+          slug: "fallback",
+        },
+      },
+    ],
+    fallback: false,
+  };
+}
+
+export function getStaticProps() {
+  return {
+    props: {
+      message: "This is a static fallback page.",
+    },
+  };
+}
+
+export default function Page({
+  message,
+}: InferGetStaticPropsType<typeof getStaticProps>) {
+  return (
+    <div>
+      <h1>Static Fallback Page</h1>
+      <p data-testid="message">{message}</p>
+    </div>
+  );
+}

examples/pages-router/src/pages/fallback-intercepted/ssg.tsx

@@ -0,0 +1,20 @@
+import type { InferGetStaticPropsType } from "next";
+
+export function getStaticProps() {
+  return {
+    props: {
+      message: "This is a static ssg page.",
+    },
+  };
+}
+
+export default function Page({
+  message,
+}: InferGetStaticPropsType<typeof getStaticProps>) {
+  return (
+    <div>
+      <h1>Static Fallback Page</h1>
+      <p data-testid="message">{message}</p>
+    </div>
+  );
+}

examples/pages-router/src/pages/fallback-intercepted/static.tsx

@@ -0,0 +1,8 @@
+export default function Page() {
+  return (
+    <div>
+      <h1>Static Fallback Page</h1>
+      <p data-testid="message">This is a fully static page.</p>
+    </div>
+  );
+}

examples/pages-router/src/pages/fallback/[slug].tsx

@@ -0,0 +1,33 @@
+import type { InferGetStaticPropsType } from "next";
+
+export function getStaticPaths() {
+  return {
+    paths: [
+      {
+        params: {
+          slug: "fallback",
+        },
+      },
+    ],
+    fallback: false,
+  };
+}
+
+export function getStaticProps() {
+  return {
+    props: {
+      message: "This is a static fallback page.",
+    },
+  };
+}
+
+export default function Page({
+  message,
+}: InferGetStaticPropsType<typeof getStaticProps>) {
+  return (
+    <div>
+      <h1>Static Fallback Page</h1>
+      <p data-testid="message">{message}</p>
+    </div>
+  );
+}

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

@@ -225,43 +225,73 @@ async function processRequest(
   // https://github.com/dougmoscrop/serverless-http/issues/227
   delete req.body;
 
+  // Here we try to apply as much request metadata as possible
+  // We apply every metadata from `resolve-routes` https://github.com/vercel/next.js/blob/916f105b97211de50f8580f0b39c9e7c60de4886/packages/next/src/server/lib/router-utils/resolve-routes.ts
+  // and `router-server` https://github.com/vercel/next.js/blob/916f105b97211de50f8580f0b39c9e7c60de4886/packages/next/src/server/lib/router-server.ts
+  const initialURL = new URL(routingResult.initialURL);
+  let invokeStatus: number | undefined;
+  if (routingResult.internalEvent.rawPath === "/500") {
+    invokeStatus = 500;
+  } else if (routingResult.internalEvent.rawPath === "/404") {
+    invokeStatus = 404;
+  }
+  const requestMetadata = {
+    isNextDataReq: routingResult.internalEvent.query.__nextDataReq === "1",
+    initURL: routingResult.initialURL,
+    initQuery: convertToQuery(initialURL.search),
+    initProtocol: initialURL.protocol,
+    defaultLocale: NextConfig.i18n?.defaultLocale,
+    locale: routingResult.locale,
+    middlewareInvoke: false,
+    // By setting invokePath and invokeQuery we can bypass some of the routing logic in Next.js
+    invokePath: routingResult.internalEvent.rawPath,
+    invokeQuery: routingResult.internalEvent.query,
+    // invokeStatus is only used for error pages
+    invokeStatus,
+  };
+
   try {
     //#override applyNextjsPrebundledReact
     setNextjsPrebundledReact(routingResult.internalEvent.rawPath);
     //#endOverride
 
-    // Here we try to apply as much request metadata as possible
-    // We apply every metadata from `resolve-routes` https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/router-utils/resolve-routes.ts
-    // and `router-server` https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/router-server.ts
-    const initialURL = new URL(routingResult.initialURL);
-    let invokeStatus: number | undefined;
-    if (routingResult.internalEvent.rawPath === "/500") {
-      invokeStatus = 500;
-    } else if (routingResult.internalEvent.rawPath === "/404") {
-      invokeStatus = 404;
-    }
-    const requestMetadata = {
-      isNextDataReq: routingResult.internalEvent.query.__nextDataReq === "1",
-      initURL: routingResult.initialURL,
-      initQuery: convertToQuery(initialURL.search),
-      initProtocol: initialURL.protocol,
-      defaultLocale: NextConfig.i18n?.defaultLocale,
-      locale: routingResult.locale,
-      middlewareInvoke: false,
-      // By setting invokePath and invokeQuery we can bypass some of the routing logic in Next.js
-      invokePath: routingResult.internalEvent.rawPath,
-      invokeQuery: routingResult.internalEvent.query,
-      // invokeStatus is only used for error pages
-      invokeStatus,
-    };
     // Next Server
     await requestHandler(requestMetadata)(req, res);
   } catch (e: any) {
     // This might fail when using bundled next, importing won't do the trick either
     if (e.constructor.name === "NoFallbackError") {
-      // Do we need to handle _not-found
-      // Ideally this should never get triggered and be intercepted by the routing handler
-      await tryRenderError("404", res, routingResult.internalEvent);
+      await handleNoFallbackError(req, res, routingResult, requestMetadata);
+    } else {
+      error("NextJS request failed.", e);
+      await tryRenderError("500", res, routingResult.internalEvent);
+    }
+  }
+}
+
+async function handleNoFallbackError(
+  req: IncomingMessage,
+  res: OpenNextNodeResponse,
+  routingResult: RoutingResult,
+  metadata: Record<string, unknown>,
+  index = 1,
+) {
+  if (index >= 5) {
+    await tryRenderError("500", res, routingResult.internalEvent);
+    return;
+  }
+  if (index >= routingResult.resolvedRoutes.length) {
+    await tryRenderError("404", res, routingResult.internalEvent);
+    return;
+  }
+  try {
+    await requestHandler({
+      ...routingResult,
+      invokeOutput: routingResult.resolvedRoutes[index].route,
+      ...metadata,
+    })(req, res);
+  } catch (e: any) {
+    if (e.constructor.name === "NoFallbackError") {
+      await handleNoFallbackError(req, res, routingResult, metadata, index + 1);
     } else {
       error("NextJS request failed.", e);
       await tryRenderError("500", res, routingResult.internalEvent);

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

@@ -13,6 +13,7 @@ import { emptyReadableStream, toReadableStream } from "utils/stream";
 
 import { debug } from "../../adapters/logger";
 import { handleLocaleRedirect, localizePath } from "./i18n";
+import { dynamicRouteMatcher, staticRouteMatcher } from "./routeMatcher";
 import {
   constructNextUrl,
   convertFromQueryString,
@@ -393,22 +394,39 @@ export function handleFallbackFalse(
 ): { event: InternalEvent; isISR: boolean } {
   const { rawPath } = internalEvent;
   const { dynamicRoutes, routes } = prerenderManifest;
-  const routeFallback = Object.entries(dynamicRoutes)
-    .filter(([, { fallback }]) => fallback === false)
-    .some(([, { routeRegex }]) => {
-      const routeRegexExp = new RegExp(routeRegex);
-      return routeRegexExp.test(rawPath);
-    });
+  const prerenderedFallbackRoutes = Object.entries(dynamicRoutes).filter(
+    ([, { fallback }]) => fallback === false,
+  );
+  const routeFallback = prerenderedFallbackRoutes.some(([, { routeRegex }]) => {
+    const routeRegexExp = new RegExp(routeRegex);
+    return routeRegexExp.test(rawPath);
+  });
   const locales = NextConfig.i18n?.locales;
   const routesAlreadyHaveLocale =
     locales?.includes(rawPath.split("/")[1]) ||
     // If we don't use locales, we don't need to add the default locale
     locales === undefined;
-  const localizedPath = routesAlreadyHaveLocale
+  let localizedPath = routesAlreadyHaveLocale
     ? rawPath
     : `/${NextConfig.i18n?.defaultLocale}${rawPath}`;
+  // We need to remove the trailing slash if it exists
+  if (NextConfig.trailingSlash && localizedPath.endsWith("/")) {
+    localizedPath = localizedPath.slice(0, -1);
+  }
+  const matchedStaticRoute = staticRouteMatcher(localizedPath);
+  const prerenderedFallbackRoutesName = prerenderedFallbackRoutes.map(
+    ([name]) => name,
+  );
+  const matchedDynamicRoute = dynamicRouteMatcher(localizedPath).filter(
+    ({ route }) => !prerenderedFallbackRoutesName.includes(route),
+  );
   const isPregenerated = Object.keys(routes).includes(localizedPath);
-  if (routeFallback && !isPregenerated) {
+  if (
+    routeFallback &&
+    !isPregenerated &&
+    matchedStaticRoute.length === 0 &&
+    matchedDynamicRoute.length === 0
+  ) {
     return {
       event: {
         ...internalEvent,

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

@@ -1,6 +1,6 @@
 import { AppPathRoutesManifest, RoutesManifest } from "config/index";
 import type { RouteDefinition } from "types/next-types";
-import type { RouteType } from "types/open-next";
+import type { ResolvedRoute, RouteType } from "types/open-next";
 
 // Add the locale prefix to the regex so we correctly match the rawPath
 const optionalLocalePrefixRegex = `^/(?:${RoutesManifest.locales.map((locale) => `${locale}/?`).join("|")})?`;
@@ -35,7 +35,7 @@ function routeMatcher(routeDefinitions: RouteDefinition[]) {
     }
   }
 
-  return function matchRoute(path: string) {
+  return function matchRoute(path: string): ResolvedRoute[] {
     const foundRoutes = regexp.filter((route) => route.regexp.test(path));
 
     return foundRoutes.map((foundRoute) => {

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

@@ -0,0 +1,42 @@
+import { expect, test } from "@playwright/test";
+
+test.describe("fallback", () => {
+  test("should work with fully static fallback", async ({ page }) => {
+    await page.goto("/fallback-intercepted/static/");
+    const h1 = page.locator("h1");
+    await expect(h1).toHaveText("Static Fallback Page");
+    const p = page.getByTestId("message");
+    await expect(p).toHaveText("This is a fully static page.");
+  });
+
+  test("should work with static fallback", async ({ page }) => {
+    await page.goto("/fallback-intercepted/ssg/");
+    const h1 = page.locator("h1");
+    await expect(h1).toHaveText("Static Fallback Page");
+    const p = page.getByTestId("message");
+    await expect(p).toHaveText("This is a static ssg page.");
+  });
+
+  test("should work with fallback intercepted by dynamic route", async ({
+    page,
+  }) => {
+    await page.goto("/fallback-intercepted/something/");
+    const h1 = page.locator("h1");
+    await expect(h1).toHaveText("Dynamic Fallback Page");
+    const p = page.getByTestId("message");
+    await expect(p).toHaveText("This is a dynamic fallback page.");
+  });
+
+  test("should work with fallback page pregenerated", async ({ page }) => {
+    await page.goto("/fallback-intercepted/fallback/");
+    const h1 = page.locator("h1");
+    await expect(h1).toHaveText("Static Fallback Page");
+    const p = page.getByTestId("message");
+    await expect(p).toHaveText("This is a static fallback page.");
+  });
+
+  test("should 404 on page not pregenerated", async ({ request }) => {
+    const res = await request.get("/fallback/not-generated");
+    expect(res.status()).toBe(404);
+  });
+});