Skip to content

Commit f25c249

Browse files
conico974conico974
authored
Fix fallback false (#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
1 parent d5d137f commit f25c249

File tree

11 files changed

+306
-37
lines changed

11 files changed

+306
-37
lines changed

‎.changeset/large-forks-decide.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@opennextjs/aws": patch
3+
---
4+
5+
fix 404 with fallback false on dynamic route

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import type { InferGetServerSidePropsType } from "next";
2+
3+
export function getServerSideProps() {
4+
return {
5+
props: {
6+
message: "This is a dynamic fallback page.",
7+
},
8+
};
9+
}
10+
11+
export default function Page({
12+
message,
13+
}: InferGetServerSidePropsType<typeof getServerSideProps>) {
14+
return (
15+
<div>
16+
<h1>Dynamic Fallback Page</h1>
17+
<p data-testid="message">{message}</p>
18+
</div>
19+
);
20+
}

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import type { InferGetStaticPropsType } from "next";
2+
3+
export function getStaticPaths() {
4+
return {
5+
paths: [
6+
{
7+
params: {
8+
slug: "fallback",
9+
},
10+
},
11+
],
12+
fallback: false,
13+
};
14+
}
15+
16+
export function getStaticProps() {
17+
return {
18+
props: {
19+
message: "This is a static fallback page.",
20+
},
21+
};
22+
}
23+
24+
export default function Page({
25+
message,
26+
}: InferGetStaticPropsType<typeof getStaticProps>) {
27+
return (
28+
<div>
29+
<h1>Static Fallback Page</h1>
30+
<p data-testid="message">{message}</p>
31+
</div>
32+
);
33+
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import type { InferGetStaticPropsType } from "next";
2+
3+
export function getStaticProps() {
4+
return {
5+
props: {
6+
message: "This is a static ssg page.",
7+
},
8+
};
9+
}
10+
11+
export default function Page({
12+
message,
13+
}: InferGetStaticPropsType<typeof getStaticProps>) {
14+
return (
15+
<div>
16+
<h1>Static Fallback Page</h1>
17+
<p data-testid="message">{message}</p>
18+
</div>
19+
);
20+
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
export default function Page() {
2+
return (
3+
<div>
4+
<h1>Static Fallback Page</h1>
5+
<p data-testid="message">This is a fully static page.</p>
6+
</div>
7+
);
8+
}

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import type { InferGetStaticPropsType } from "next";
2+
3+
export function getStaticPaths() {
4+
return {
5+
paths: [
6+
{
7+
params: {
8+
slug: "fallback",
9+
},
10+
},
11+
],
12+
fallback: false,
13+
};
14+
}
15+
16+
export function getStaticProps() {
17+
return {
18+
props: {
19+
message: "This is a static fallback page.",
20+
},
21+
};
22+
}
23+
24+
export default function Page({
25+
message,
26+
}: InferGetStaticPropsType<typeof getStaticProps>) {
27+
return (
28+
<div>
29+
<h1>Static Fallback Page</h1>
30+
<p data-testid="message">{message}</p>
31+
</div>
32+
);
33+
}

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

Lines changed: 57 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -225,43 +225,73 @@ async function processRequest(
225225
// https://github.com/dougmoscrop/serverless-http/issues/227
226226
delete req.body;
227227

228+
// Here we try to apply as much request metadata as possible
229+
// 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
230+
// and `router-server` https://github.com/vercel/next.js/blob/916f105b97211de50f8580f0b39c9e7c60de4886/packages/next/src/server/lib/router-server.ts
231+
const initialURL = new URL(routingResult.initialURL);
232+
let invokeStatus: number | undefined;
233+
if (routingResult.internalEvent.rawPath === "/500") {
234+
invokeStatus = 500;
235+
} else if (routingResult.internalEvent.rawPath === "/404") {
236+
invokeStatus = 404;
237+
}
238+
const requestMetadata = {
239+
isNextDataReq: routingResult.internalEvent.query.__nextDataReq === "1",
240+
initURL: routingResult.initialURL,
241+
initQuery: convertToQuery(initialURL.search),
242+
initProtocol: initialURL.protocol,
243+
defaultLocale: NextConfig.i18n?.defaultLocale,
244+
locale: routingResult.locale,
245+
middlewareInvoke: false,
246+
// By setting invokePath and invokeQuery we can bypass some of the routing logic in Next.js
247+
invokePath: routingResult.internalEvent.rawPath,
248+
invokeQuery: routingResult.internalEvent.query,
249+
// invokeStatus is only used for error pages
250+
invokeStatus,
251+
};
252+
228253
try {
229254
//#override applyNextjsPrebundledReact
230255
setNextjsPrebundledReact(routingResult.internalEvent.rawPath);
231256
//#endOverride
232257

233-
// Here we try to apply as much request metadata as possible
234-
// 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
235-
// and `router-server` https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/router-server.ts
236-
const initialURL = new URL(routingResult.initialURL);
237-
let invokeStatus: number | undefined;
238-
if (routingResult.internalEvent.rawPath === "/500") {
239-
invokeStatus = 500;
240-
} else if (routingResult.internalEvent.rawPath === "/404") {
241-
invokeStatus = 404;
242-
}
243-
const requestMetadata = {
244-
isNextDataReq: routingResult.internalEvent.query.__nextDataReq === "1",
245-
initURL: routingResult.initialURL,
246-
initQuery: convertToQuery(initialURL.search),
247-
initProtocol: initialURL.protocol,
248-
defaultLocale: NextConfig.i18n?.defaultLocale,
249-
locale: routingResult.locale,
250-
middlewareInvoke: false,
251-
// By setting invokePath and invokeQuery we can bypass some of the routing logic in Next.js
252-
invokePath: routingResult.internalEvent.rawPath,
253-
invokeQuery: routingResult.internalEvent.query,
254-
// invokeStatus is only used for error pages
255-
invokeStatus,
256-
};
257258
// Next Server
258259
await requestHandler(requestMetadata)(req, res);
259260
} catch (e: any) {
260261
// This might fail when using bundled next, importing won't do the trick either
261262
if (e.constructor.name === "NoFallbackError") {
262-
// Do we need to handle _not-found
263-
// Ideally this should never get triggered and be intercepted by the routing handler
264-
await tryRenderError("404", res, routingResult.internalEvent);
263+
await handleNoFallbackError(req, res, routingResult, requestMetadata);
264+
} else {
265+
error("NextJS request failed.", e);
266+
await tryRenderError("500", res, routingResult.internalEvent);
267+
}
268+
}
269+
}
270+
271+
async function handleNoFallbackError(
272+
req: IncomingMessage,
273+
res: OpenNextNodeResponse,
274+
routingResult: RoutingResult,
275+
metadata: Record<string, unknown>,
276+
index = 1,
277+
) {
278+
if (index >= 5) {
279+
await tryRenderError("500", res, routingResult.internalEvent);
280+
return;
281+
}
282+
if (index >= routingResult.resolvedRoutes.length) {
283+
await tryRenderError("404", res, routingResult.internalEvent);
284+
return;
285+
}
286+
try {
287+
await requestHandler({
288+
...routingResult,
289+
invokeOutput: routingResult.resolvedRoutes[index].route,
290+
...metadata,
291+
})(req, res);
292+
} catch (e: any) {
293+
if (e.constructor.name === "NoFallbackError") {
294+
await handleNoFallbackError(req, res, routingResult, metadata, index + 1);
265295
} else {
266296
error("NextJS request failed.", e);
267297
await tryRenderError("500", res, routingResult.internalEvent);

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

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { emptyReadableStream, toReadableStream } from "utils/stream";
1313

1414
import { debug } from "../../adapters/logger";
1515
import { handleLocaleRedirect, localizePath } from "./i18n";
16+
import { dynamicRouteMatcher, staticRouteMatcher } from "./routeMatcher";
1617
import {
1718
constructNextUrl,
1819
convertFromQueryString,
@@ -393,22 +394,39 @@ export function handleFallbackFalse(
393394
): { event: InternalEvent; isISR: boolean } {
394395
const { rawPath } = internalEvent;
395396
const { dynamicRoutes, routes } = prerenderManifest;
396-
const routeFallback = Object.entries(dynamicRoutes)
397-
.filter(([, { fallback }]) => fallback === false)
398-
.some(([, { routeRegex }]) => {
399-
const routeRegexExp = new RegExp(routeRegex);
400-
return routeRegexExp.test(rawPath);
401-
});
397+
const prerenderedFallbackRoutes = Object.entries(dynamicRoutes).filter(
398+
([, { fallback }]) => fallback === false,
399+
);
400+
const routeFallback = prerenderedFallbackRoutes.some(([, { routeRegex }]) => {
401+
const routeRegexExp = new RegExp(routeRegex);
402+
return routeRegexExp.test(rawPath);
403+
});
402404
const locales = NextConfig.i18n?.locales;
403405
const routesAlreadyHaveLocale =
404406
locales?.includes(rawPath.split("/")[1]) ||
405407
// If we don't use locales, we don't need to add the default locale
406408
locales === undefined;
407-
const localizedPath = routesAlreadyHaveLocale
409+
let localizedPath = routesAlreadyHaveLocale
408410
? rawPath
409411
: `/${NextConfig.i18n?.defaultLocale}${rawPath}`;
412+
// We need to remove the trailing slash if it exists
413+
if (NextConfig.trailingSlash && localizedPath.endsWith("/")) {
414+
localizedPath = localizedPath.slice(0, -1);
415+
}
416+
const matchedStaticRoute = staticRouteMatcher(localizedPath);
417+
const prerenderedFallbackRoutesName = prerenderedFallbackRoutes.map(
418+
([name]) => name,
419+
);
420+
const matchedDynamicRoute = dynamicRouteMatcher(localizedPath).filter(
421+
({ route }) => !prerenderedFallbackRoutesName.includes(route),
422+
);
410423
const isPregenerated = Object.keys(routes).includes(localizedPath);
411-
if (routeFallback && !isPregenerated) {
424+
if (
425+
routeFallback &&
426+
!isPregenerated &&
427+
matchedStaticRoute.length === 0 &&
428+
matchedDynamicRoute.length === 0
429+
) {
412430
return {
413431
event: {
414432
...internalEvent,

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { AppPathRoutesManifest, RoutesManifest } from "config/index";
22
import type { RouteDefinition } from "types/next-types";
3-
import type { RouteType } from "types/open-next";
3+
import type { ResolvedRoute, RouteType } from "types/open-next";
44

55
// Add the locale prefix to the regex so we correctly match the rawPath
66
const optionalLocalePrefixRegex = `^/(?:${RoutesManifest.locales.map((locale) => `${locale}/?`).join("|")})?`;
@@ -35,7 +35,7 @@ function routeMatcher(routeDefinitions: RouteDefinition[]) {
3535
}
3636
}
3737

38-
return function matchRoute(path: string) {
38+
return function matchRoute(path: string): ResolvedRoute[] {
3939
const foundRoutes = regexp.filter((route) => route.regexp.test(path));
4040

4141
return foundRoutes.map((foundRoute) => {

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import { expect, test } from "@playwright/test";
2+
3+
test.describe("fallback", () => {
4+
test("should work with fully static fallback", async ({ page }) => {
5+
await page.goto("/fallback-intercepted/static/");
6+
const h1 = page.locator("h1");
7+
await expect(h1).toHaveText("Static Fallback Page");
8+
const p = page.getByTestId("message");
9+
await expect(p).toHaveText("This is a fully static page.");
10+
});
11+
12+
test("should work with static fallback", async ({ page }) => {
13+
await page.goto("/fallback-intercepted/ssg/");
14+
const h1 = page.locator("h1");
15+
await expect(h1).toHaveText("Static Fallback Page");
16+
const p = page.getByTestId("message");
17+
await expect(p).toHaveText("This is a static ssg page.");
18+
});
19+
20+
test("should work with fallback intercepted by dynamic route", async ({
21+
page,
22+
}) => {
23+
await page.goto("/fallback-intercepted/something/");
24+
const h1 = page.locator("h1");
25+
await expect(h1).toHaveText("Dynamic Fallback Page");
26+
const p = page.getByTestId("message");
27+
await expect(p).toHaveText("This is a dynamic fallback page.");
28+
});
29+
30+
test("should work with fallback page pregenerated", async ({ page }) => {
31+
await page.goto("/fallback-intercepted/fallback/");
32+
const h1 = page.locator("h1");
33+
await expect(h1).toHaveText("Static Fallback Page");
34+
const p = page.getByTestId("message");
35+
await expect(p).toHaveText("This is a static fallback page.");
36+
});
37+
38+
test("should 404 on page not pregenerated", async ({ request }) => {
39+
const res = await request.get("/fallback/not-generated");
40+
expect(res.status()).toBe(404);
41+
});
42+
});

0 commit comments

Comments
 (0)