Fix pages i18n catch-all issue (#582)

* fix pages i18n catch-all issue

* collect locales during the build process

* add e2e for pages-issue-578

packages/next-on-pages/build-metadata.d.ts

@@ -0,0 +1,7 @@
+/**
+ * Metadata generated by the next-on-pages build process
+ */
+type NextOnPagesBuildMetadata = {
+	/** Locales used by the application (collected from the Vercel output) */
+	collectedLocales: string[];
+};

packages/next-on-pages/src/buildApplication/buildWorkerFile.ts

@@ -74,6 +74,9 @@ export async function buildWorkerFile(
 		define: {
 			__CONFIG__: JSON.stringify(vercelConfig),
 			__NODE_ENV__: JSON.stringify(getNodeEnv()),
+			__BUILD_METADATA__: JSON.stringify({
+				collectedLocales: collectLocales(vercelConfig.routes),
+			}),
 		},
 		outfile: outputFile,
 		minify,
@@ -99,3 +102,22 @@ type BuildWorkerFileOpts = {
 	nopDistDir: string;
 	templatesDir: string;
 };
+
+/**
+ * Collects all the locales present in the processed Vercel routes
+ *
+ * @param routes The Vercel routes to collect the locales from
+ * @returns an array containing all the found locales (without duplicates)
+ */
+function collectLocales(routes: ProcessedVercelRoutes): string[] {
+	const locales = Object.values(routes)
+		.flat()
+		.flatMap(source => {
+			if (source.locale?.redirect) {
+				return Object.keys(source.locale.redirect);
+			}
+			return [];
+		})
+		.filter(Boolean);
+	return [...new Set(locales)];
+}

packages/next-on-pages/templates/_worker.js/handleRequest.ts

@@ -14,17 +14,20 @@ import type { RequestContext } from '../../src/utils/requestContext';
  * @param reqCtx Request Context object (contains all we need in to know regarding the request in order to handle it).
  * @param config The processed Vercel build output config.
  * @param output Vercel build output.
+ * @param buildMetadata Metadata generated by the next-on-pages build process.
  * @returns An instance of the router.
  */
 export async function handleRequest(
 	reqCtx: RequestContext,
 	config: ProcessedVercelConfig,
 	output: VercelBuildOutput,
+	buildMetadata: NextOnPagesBuildMetadata,
 ): Promise<Response> {
 	const matcher = new RoutesMatcher(
 		config.routes,
 		output,
 		reqCtx,
+		buildMetadata,
 		config.wildcard,
 	);
 	const match = await findMatch(matcher);

packages/next-on-pages/templates/_worker.js/index.ts

@@ -13,6 +13,8 @@ declare const __CONFIG__: ProcessedVercelConfig;
 
 declare const __BUILD_OUTPUT__: VercelBuildOutput;
 
+declare const __BUILD_METADATA__: NextOnPagesBuildMetadata;
+
 declare const __ENV_ALS_PROMISE__: Promise<null | AsyncLocalStorage<unknown>>;
 
 export default {
@@ -54,6 +56,7 @@ export default {
 					},
 					__CONFIG__,
 					__BUILD_OUTPUT__,
+					__BUILD_METADATA__,
 				);
 			},
 		);

packages/next-on-pages/templates/_worker.js/routes-matcher.ts

@@ -43,7 +43,7 @@ export class RoutesMatcher {
 	/** Tracker for the middleware that have been invoked in a phase */
 	private middlewareInvoked: string[];
 	/** Locales found during routing */
-	public locales: Record<string, string> | undefined;
+	public locales: Set<string>;
 
 	/**
 	 * Creates a new instance of a request matcher.
@@ -53,6 +53,7 @@ export class RoutesMatcher {
 	 * @param routes The processed Vercel build output config routes.
 	 * @param output Vercel build output.
 	 * @param reqCtx Request context object; request object, assets fetcher, and execution context.
+	 * @param buildMetadata Metadata generated by the next-on-pages build process.
 	 * @param wildcardConfig Wildcard options from the Vercel build output config.
 	 * @returns The matched set of path, status, headers, and search params.
 	 */
@@ -63,6 +64,7 @@ export class RoutesMatcher {
 		private output: VercelBuildOutput,
 		/** Request Context object for the request to match */
 		private reqCtx: RequestContext,
+		buildMetadata: NextOnPagesBuildMetadata,
 		wildcardConfig?: VercelWildcardConfig,
 	) {
 		this.url = new URL(reqCtx.request.url);
@@ -79,6 +81,8 @@ export class RoutesMatcher {
 		this.wildcardMatch = wildcardConfig?.find(
 			w => w.domain === this.url.hostname,
 		);
+
+		this.locales = new Set(buildMetadata.collectedLocales);
 	}
 
 	/**
@@ -375,9 +379,6 @@ export class RoutesMatcher {
 	private applyLocaleRedirects(route: VercelSource): void {
 		if (!route.locale?.redirect) return;
 
-		if (!this.locales) this.locales = {};
-		Object.assign(this.locales, route.locale.redirect);
-
 		// Automatic locale detection is only supposed to occur at the root. However, the build output
 		// sometimes uses `/` as the regex instead of `^/$`. So, we should check if the `route.src` is
 		// equal to the path if it is not a regular expression, to determine if we are at the root.
@@ -589,6 +590,22 @@ export class RoutesMatcher {
 			return 'done';
 		}
 
+		if (phase === 'none') {
+			// applications using the Pages router with i18n plus a catch-all root route
+			// redirect all requests (including /api/ ones) to the catch-all route, the only
+			// way to prevent this erroneous behavior is to remove the locale here if the
+			// path without the locale exists in the vercel build output
+			for (const locale of this.locales) {
+				const localeRegExp = new RegExp(`/${locale}(/.*)`);
+				const match = this.path.match(localeRegExp);
+				const pathWithoutLocale = match?.[1];
+				if (pathWithoutLocale && pathWithoutLocale in this.output) {
+					this.path = pathWithoutLocale;
+					break;
+				}
+			}
+		}
+
 		let pathExistsInOutput = this.path in this.output;
 
 		// If a path with a trailing slash entered the `rewrite` phase and didn't find a match, it might

packages/next-on-pages/templates/_worker.js/utils/routing.ts

@@ -144,10 +144,7 @@ export async function runOrFetchBuildOutputItem(
  * @param locales Known available locales.
  * @returns Whether the source route matches the regex for a locale with a trailing slash.
  */
-export function isLocaleTrailingSlashRegex(
-	src: string,
-	locales: Record<string, string>,
-) {
+export function isLocaleTrailingSlashRegex(src: string, locales: Set<string>) {
 	const prefix = '^//?(?:';
 	const suffix = ')/(.*)$';
 
@@ -156,5 +153,5 @@ export function isLocaleTrailingSlashRegex(
 	}
 
 	const foundLocales = src.slice(prefix.length, -suffix.length).split('|');
-	return foundLocales.every(locale => locale in locales);
+	return foundLocales.every(locale => locales.has(locale));
 }

packages/next-on-pages/tsconfig.json

@@ -5,6 +5,7 @@
 		"templates",
 		"tests",
 		"vercel.types.d.ts",
+		"build-metadata.d.ts",
 		"env.d.ts",
 		".eslintrc.js",
 		"build-no-nodejs-compat-flag-static-error-page.mjs"

pages-e2e/features/pages-issue-578/issue.test.ts

@@ -0,0 +1,34 @@
+import { getAssertVisible } from '@features-utils/getAssertVisible';
+import { describe, test } from 'vitest';
+
+describe('issue-578', () => {
+	test('navigating to /api/hello should return a Hello world response', async ({
+		expect,
+	}) => {
+		const response = await fetch(`${DEPLOYMENT_URL}/api/hello`);
+		expect(await response.text()).toBe('Hello world');
+	});
+
+	test('/ should display the catch-all page', async () => {
+		const page = await BROWSER.newPage();
+		const assertVisible = getAssertVisible(page);
+		const pageUrl = `${DEPLOYMENT_URL}/`;
+		await page.goto(pageUrl);
+		await assertVisible('h1', { hasText: '[[...path]].tsx Page' });
+	});
+
+	test('/a/random/path should display the catch-all page', async () => {
+		const page = await BROWSER.newPage();
+		const assertVisible = getAssertVisible(page);
+		const pageUrl = `${DEPLOYMENT_URL}/a/random/path`;
+		await page.goto(pageUrl);
+		await assertVisible('h1', { hasText: '[[...path]].tsx Page' });
+	});
+
+	test('static assets should be correctly served', async ({ expect }) => {
+		const response = await fetch(`${DEPLOYMENT_URL}/next.svg`);
+		expect(await response.text()).toMatch(
+			/^<svg xmlns="http:\/\/www.w3.org\/2000\/svg" fill="none" viewBox="0 0 394 80">.*<\/svg>/,
+		);
+	});
+});

pages-e2e/features/pages-issue-578/main.feature

@@ -0,0 +1,3 @@
+{
+	"setup": "node --loader tsm setup.ts"
+}

pages-e2e/features/pages-issue-578/setup.ts

@@ -0,0 +1 @@
+// no setup required

pages-e2e/fixtures/pages-issue-578/.gitignore

@@ -0,0 +1,36 @@
+# See https://help.github.com/articles/ignoring-files/ for more about ignoring files.
+
+# dependencies
+/node_modules
+/.pnp
+.pnp.js
+.yarn/install-state.gz
+
+# testing
+/coverage
+
+# next.js
+/.next/
+/out/
+
+# production
+/build
+
+# misc
+.DS_Store
+*.pem
+
+# debug
+npm-debug.log*
+yarn-debug.log*
+yarn-error.log*
+
+# local env files
+.env*.local
+
+# vercel
+.vercel
+
+# typescript
+*.tsbuildinfo
+next-env.d.ts

pages-e2e/fixtures/pages-issue-578/README.md

@@ -0,0 +1 @@
+Reproduction of https://github.com/cloudflare/next-on-pages/issues/578

pages-e2e/fixtures/pages-issue-578/main.fixture

@@ -0,0 +1,13 @@
+{
+	"features": [
+		"pages-issue-578"
+	],
+	"localSetup": "./setup.sh",
+	"buildConfig": {
+		"buildCommand": "npx --force ../../../packages/next-on-pages",
+		"buildOutputDirectory": ".vercel/output/static"
+	},
+	"deploymentConfig": {
+		"compatibilityFlags": ["nodejs_compat"]
+	}
+}

pages-e2e/fixtures/pages-issue-578/next.config.js

@@ -0,0 +1,10 @@
+/** @type {import('next').NextConfig} */
+const nextConfig = {
+	reactStrictMode: true,
+	i18n: {
+		locales: ['en', 'dario', 'testttttt'],
+		defaultLocale: 'en',
+	},
+};
+
+module.exports = nextConfig;