diff --git a/.changeset/large-forks-decide.md b/.changeset/large-forks-decide.md new file mode 100644 index 000000000..afde55ed3 --- /dev/null +++ b/.changeset/large-forks-decide.md @@ -0,0 +1,5 @@ +--- +"@opennextjs/aws": patch +--- + +fix 404 with fallback false on dynamic route diff --git a/examples/pages-router/src/pages/fallback-intercepted/[...slugs].tsx b/examples/pages-router/src/pages/fallback-intercepted/[...slugs].tsx new file mode 100644 index 000000000..6294d629e --- /dev/null +++ b/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) { + return ( +
+

Dynamic Fallback Page

+

{message}

+
+ ); +} diff --git a/examples/pages-router/src/pages/fallback-intercepted/[slug].tsx b/examples/pages-router/src/pages/fallback-intercepted/[slug].tsx new file mode 100644 index 000000000..a8ee3d1b1 --- /dev/null +++ b/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) { + return ( +
+

Static Fallback Page

+

{message}

+
+ ); +} diff --git a/examples/pages-router/src/pages/fallback-intercepted/ssg.tsx b/examples/pages-router/src/pages/fallback-intercepted/ssg.tsx new file mode 100644 index 000000000..323a43849 --- /dev/null +++ b/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) { + return ( +
+

Static Fallback Page

+

{message}

+
+ ); +} diff --git a/examples/pages-router/src/pages/fallback-intercepted/static.tsx b/examples/pages-router/src/pages/fallback-intercepted/static.tsx new file mode 100644 index 000000000..4c2214787 --- /dev/null +++ b/examples/pages-router/src/pages/fallback-intercepted/static.tsx @@ -0,0 +1,8 @@ +export default function Page() { + return ( +
+

Static Fallback Page

+

This is a fully static page.

+
+ ); +} diff --git a/examples/pages-router/src/pages/fallback/[slug].tsx b/examples/pages-router/src/pages/fallback/[slug].tsx new file mode 100644 index 000000000..a8ee3d1b1 --- /dev/null +++ b/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) { + return ( +
+

Static Fallback Page

+

{message}

+
+ ); +} diff --git a/packages/open-next/src/core/requestHandler.ts b/packages/open-next/src/core/requestHandler.ts index 26f8df333..de9026ebd 100644 --- a/packages/open-next/src/core/requestHandler.ts +++ b/packages/open-next/src/core/requestHandler.ts @@ -221,43 +221,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, + 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); diff --git a/packages/open-next/src/core/routing/matcher.ts b/packages/open-next/src/core/routing/matcher.ts index 24748634a..2d58af8db 100644 --- a/packages/open-next/src/core/routing/matcher.ts +++ b/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, diff --git a/packages/open-next/src/core/routing/routeMatcher.ts b/packages/open-next/src/core/routing/routeMatcher.ts index c9568ada2..94cdf53ec 100644 --- a/packages/open-next/src/core/routing/routeMatcher.ts +++ b/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) => { diff --git a/packages/tests-e2e/tests/pagesRouter/fallback.test.ts b/packages/tests-e2e/tests/pagesRouter/fallback.test.ts new file mode 100644 index 000000000..d7dbb4ab3 --- /dev/null +++ b/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); + }); +}); diff --git a/packages/tests-unit/tests/core/routing/matcher.test.ts b/packages/tests-unit/tests/core/routing/matcher.test.ts index 697f56f48..a70962fd1 100644 --- a/packages/tests-unit/tests/core/routing/matcher.test.ts +++ b/packages/tests-unit/tests/core/routing/matcher.test.ts @@ -11,6 +11,66 @@ import { vi } from "vitest"; vi.mock("@opennextjs/aws/adapters/config/index.js", () => ({ NextConfig: {}, + AppPathRoutesManifest: { + "/api/app/route": "/api/app", + "/app/page": "/app", + "/catchAll/[...slug]/page": "/catchAll/[...slug]", + }, + RoutesManifest: { + version: 3, + pages404: true, + caseSensitive: false, + basePath: "", + locales: [], + redirects: [], + headers: [], + routes: { + dynamic: [ + { + page: "/catchAll/[...slug]", + regex: "^/catchAll/(.+?)(?:/)?$", + routeKeys: { + nxtPslug: "nxtPslug", + }, + namedRegex: "^/catchAll/(?.+?)(?:/)?$", + }, + { + page: "/page/catchAll/[...slug]", + regex: "^/page/catchAll/(.+?)(?:/)?$", + routeKeys: { + nxtPslug: "nxtPslug", + }, + namedRegex: "^/page/catchAll/(?.+?)(?:/)?$", + }, + ], + static: [ + { + page: "/app", + regex: "^/app(?:/)?$", + routeKeys: {}, + namedRegex: "^/app(?:/)?$", + }, + { + page: "/api/app", + regex: "^/api/app(?:/)?$", + routeKeys: {}, + namedRegex: "^/api/app(?:/)?$", + }, + { + page: "/page", + regex: "^/page(?:/)?$", + routeKeys: {}, + namedRegex: "^/page(?:/)?$", + }, + { + page: "/page/catchAll/static", + regex: "^/page/catchAll/static(?:/)?$", + routeKeys: {}, + namedRegex: "^/page/catchAll/static(?:/)?$", + }, + ], + }, + }, })); vi.mock("@opennextjs/aws/core/routing/i18n/index.js", () => ({ localizePath: (event: InternalEvent) => event.rawPath,