trailingslashes and redirects improvements/fixes (#697)

* improve current trailing slash e2e tests (by checking that the responses are actual redirects)

* fix issue in which redirect responses become rewrites when a middleware is involved

* add e2e tests for issue #696

.changeset/khaki-pumas-whisper.md

@@ -0,0 +1,5 @@
+---
+'@cloudflare/next-on-pages': patch
+---
+
+fix issue in which redirect responses become rewrites when a middleware is involved

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

@@ -547,6 +547,13 @@ export class RoutesMatcher {
 			return 'done';
 		}
 
+		// If the route is a redirect then we're actually done
+		const isRedirect =
+			route.status && route.status >= 300 && route.status <= 399;
+		if (isRedirect) {
+			return 'done';
+		}
+
 		return 'next';
 	}
 

pages-e2e/features/appConfigsTrailingSlashFalse/trailingSlash.test.ts

@@ -1,17 +1,30 @@
 import { describe, test } from 'vitest';
 
 describe('default trailing slashes redirection', () => {
-	test(`fetching with an added trailing slashed results in a redirected response`, async ({
-		expect,
-	}) => {
-		const response = await fetch(
-			`${DEPLOYMENT_URL}/api/routing-trailing-slash-test/`,
-		);
+	describe('fetching with an added trailing slashed results in a redirected response', () => {
+		test('the user gets the requested page', async ({ expect }) => {
+			const response = await fetch(
+				`${DEPLOYMENT_URL}/api/routing-trailing-slash-test/`,
+			);
 
-		expect(response.redirected).toBe(true);
-		expect(response.status).toBe(200);
-		expect(response.url).toEqual(
-			`${DEPLOYMENT_URL}/api/routing-trailing-slash-test`,
-		);
+			expect(response.redirected).toBe(true);
+			expect(response.status).toBe(200);
+			expect(response.url).toEqual(
+				`${DEPLOYMENT_URL}/api/routing-trailing-slash-test`,
+			);
+		});
+
+		test('the response is actually a redirect (and not a rewrite)', async ({
+			expect,
+		}) => {
+			const err = await fetch(
+				`${DEPLOYMENT_URL}/api/routing-trailing-slash-test/`,
+				{ redirect: 'error' },
+			)
+				.then(() => null)
+				.catch(e => e.cause.message);
+
+			expect(err).toEqual('unexpected redirect');
+		});
 	});
 });

pages-e2e/features/appConfigsTrailingSlashTrue/trailingSlash.test.ts

@@ -1,17 +1,30 @@
 import { describe, test } from 'vitest';
 
 describe('default trailing slashes redirection', () => {
-	test(`fetching without an added trailing slash results in a redirected response`, async ({
-		expect,
-	}) => {
-		const response = await fetch(
-			`${DEPLOYMENT_URL}/api/routing-trailing-slash-test`,
-		);
+	describe('fetching without an added trailing slash results in a redirected response', () => {
+		test('the user gets the requested page', async ({ expect }) => {
+			const response = await fetch(
+				`${DEPLOYMENT_URL}/api/routing-trailing-slash-test`,
+			);
 
-		expect(response.redirected).toBe(true);
-		expect(response.status).toBe(200);
-		expect(response.url).toEqual(
-			`${DEPLOYMENT_URL}/api/routing-trailing-slash-test/`,
-		);
+			expect(response.redirected).toBe(true);
+			expect(response.status).toBe(200);
+			expect(response.url).toEqual(
+				`${DEPLOYMENT_URL}/api/routing-trailing-slash-test/`,
+			);
+		});
+
+		test('the response is actually a redirect (and not a rewrite)', async ({
+			expect,
+		}) => {
+			const err = await fetch(
+				`${DEPLOYMENT_URL}/api/routing-trailing-slash-test`,
+				{ redirect: 'error' },
+			)
+				.then(() => null)
+				.catch(e => e.cause.message);
+
+			expect(err).toEqual('unexpected redirect');
+		});
 	});
 });

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

@@ -0,0 +1,27 @@
+import { describe, test } from 'vitest';
+
+describe('issue-696', () => {
+	describe('default trailing slashes redirection', () => {
+		describe('fetching without an added trailing slash results in a redirected response', () => {
+			test('the user gets the requested page', async ({ expect }) => {
+				const response = await fetch(`${DEPLOYMENT_URL}/api/hello`);
+
+				expect(response.redirected).toBe(true);
+				expect(response.status).toBe(200);
+				expect(response.url).toEqual(`${DEPLOYMENT_URL}/api/hello/`);
+			});
+
+			test('the response is actually a redirect (and not a rewrite)', async ({
+				expect,
+			}) => {
+				const err = await fetch(`${DEPLOYMENT_URL}/api/hello`, {
+					redirect: 'error',
+				})
+					.then(() => null)
+					.catch(e => e.cause.message);
+
+				expect(err).toEqual('unexpected redirect');
+			});
+		});
+	});
+});

pages-e2e/features/issue696/main.feature

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

pages-e2e/features/issue696/setup.ts

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

pages-e2e/features/pagesRouting/trailingSlash.test.ts

@@ -1,17 +1,30 @@
 import { describe, test } from 'vitest';
 
 describe('default trailing slashes redirection', () => {
-	test(`fetching with an added trailing slashed results in a redirected response`, async ({
-		expect,
-	}) => {
-		const response = await fetch(
-			`${DEPLOYMENT_URL}/api/routing-trailing-slash-test/`,
-		);
+	describe('fetching with an added trailing slashed results in a redirected response', () => {
+		test('the user gets the requested page', async ({ expect }) => {
+			const response = await fetch(
+				`${DEPLOYMENT_URL}/api/routing-trailing-slash-test/`,
+			);
 
-		expect(response.redirected).toBe(true);
-		expect(response.status).toBe(200);
-		expect(response.url).toEqual(
-			`${DEPLOYMENT_URL}/api/routing-trailing-slash-test`,
-		);
+			expect(response.redirected).toBe(true);
+			expect(response.status).toBe(200);
+			expect(response.url).toEqual(
+				`${DEPLOYMENT_URL}/api/routing-trailing-slash-test`,
+			);
+		});
+
+		test('the response is actually a redirect (and not a rewrite)', async ({
+			expect,
+		}) => {
+			const err = await fetch(
+				`${DEPLOYMENT_URL}/api/routing-trailing-slash-test/`,
+				{ redirect: 'error' },
+			)
+				.then(() => null)
+				.catch(e => e.cause.message);
+
+			expect(err).toEqual('unexpected redirect');
+		});
 	});
 });

pages-e2e/fixtures/issue696App/.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/issue696App/README.md

@@ -0,0 +1,40 @@
+This is a [Next.js](https://nextjs.org/) project bootstrapped with [`create-next-app`](https://github.com/vercel/next.js/tree/canary/packages/create-next-app).
+
+## Getting Started
+
+First, run the development server:
+
+```bash
+npm run dev
+# or
+yarn dev
+# or
+pnpm dev
+# or
+bun dev
+```
+
+Open [http://localhost:3000](http://localhost:3000) with your browser to see the result.
+
+You can start editing the page by modifying `pages/index.tsx`. The page auto-updates as you edit the file.
+
+[API routes](https://nextjs.org/docs/api-routes/introduction) can be accessed on [http://localhost:3000/api/hello](http://localhost:3000/api/hello). This endpoint can be edited in `pages/api/hello.ts`.
+
+The `pages/api` directory is mapped to `/api/*`. Files in this directory are treated as [API routes](https://nextjs.org/docs/api-routes/introduction) instead of React pages.
+
+This project uses [`next/font`](https://nextjs.org/docs/basic-features/font-optimization) to automatically optimize and load Inter, a custom Google Font.
+
+## Learn More
+
+To learn more about Next.js, take a look at the following resources:
+
+- [Next.js Documentation](https://nextjs.org/docs) - learn about Next.js features and API.
+- [Learn Next.js](https://nextjs.org/learn) - an interactive Next.js tutorial.
+
+You can check out [the Next.js GitHub repository](https://github.com/vercel/next.js/) - your feedback and contributions are welcome!
+
+## Deploy on Vercel
+
+The easiest way to deploy your Next.js app is to use the [Vercel Platform](https://vercel.com/new?utm_medium=default-template&filter=next.js&utm_source=create-next-app&utm_campaign=create-next-app-readme) from the creators of Next.js.
+
+Check out our [Next.js deployment documentation](https://nextjs.org/docs/deployment) for more details.

pages-e2e/fixtures/issue696App/app/api/hello/route.ts

@@ -0,0 +1,5 @@
+export const runtime = 'edge';
+
+export async function GET(req: Request) {
+	return Response.json({ text: 'Hello world!' });
+}

pages-e2e/fixtures/issue696App/app/layout.tsx

@@ -0,0 +1,11 @@
+export default function RootLayout({
+	children,
+}: {
+	children: React.ReactNode;
+}) {
+	return (
+		<html lang="en">
+			<body>{children}</body>
+		</html>
+	);
+}

pages-e2e/fixtures/issue696App/app/page.tsx

@@ -0,0 +1,7 @@
+export default function Home() {
+	return (
+		<main>
+			<h1>home page</h1>
+		</main>
+	);
+}

pages-e2e/fixtures/issue696App/main.fixture

@@ -0,0 +1,12 @@
+{
+	"features": [
+		"issue696"
+	],
+	"buildConfig": {
+		"buildCommand": "npx --force ../../../packages/next-on-pages",
+		"buildOutputDirectory": ".vercel/output/static"
+	},
+	"deploymentConfig": {
+		"compatibilityFlags": ["nodejs_compat"]
+	}
+}

pages-e2e/fixtures/issue696App/middleware.ts

@@ -0,0 +1,3 @@
+import { NextRequest } from 'next/server';
+
+export async function middleware(req: NextRequest) {}

pages-e2e/fixtures/issue696App/next.config.mjs

@@ -0,0 +1,7 @@
+/** @type {import('next').NextConfig} */
+const nextConfig = {
+	trailingSlash: true,
+	reactStrictMode: true,
+};
+
+export default nextConfig;