Changes from review

.changeset/blue-spies-shave.md

@@ -4,4 +4,4 @@
 
 Redirects trailing slashes for on-demand pages
 
-When the `trailingSlash` option is set to `always` or `never`, on-demand rendered pages will now redirect to the correct URL when the trailing slash is incorrect. This was previously the case for static pages, but now works for on-demand pages as well.
+When the `trailingSlash` option is set to `always` or `never`, on-demand rendered pages will now redirect to the correct URL when the trailing slash is incorrect. This was previously the case for static pages, but now works for on-demand pages as well. It will also redirect when there are multiple trailing slashes for all trailing slash settings. For GET requests, the redirect will be a 301 (permanent) redirect, and for all other request methods, it will be a 308 (permanent, and preserve the request method) redirect.

packages/astro/src/core/app/index.ts

@@ -1,9 +1,8 @@
-import { hasFileExtension } from '@astrojs/internal-helpers/path';
+import { collapseDuplicateTrailingSlashes, hasFileExtension } from '@astrojs/internal-helpers/path';
 import { normalizeTheLocale } from '../../i18n/index.js';
 import type { ManifestData } from '../../types/astro.js';
 import type { RouteData, SSRManifest } from '../../types/public/internal.js';
 import {
-	MANY_SLASHES,
 	REROUTABLE_STATUS_CODES,
 	REROUTE_DIRECTIVE_HEADER,
 	clientAddressSymbol,
@@ -253,32 +252,32 @@ export class App {
 		return pathname;
 	}
 
-	#redirectTrailingSlash(pathname: string): string | undefined {
+	#redirectTrailingSlash(pathname: string): string {
 		const { trailingSlash } = this.#manifest;
 
 		// Ignore root and internal paths
 		if (pathname === '/' || pathname.startsWith('/_')) {
-			return;
+			return pathname;
 		}
 
 		// Redirect multiple trailing slashes to collapsed path
-		if (MANY_SLASHES.test(pathname)) {
-			const path = pathname.replace(MANY_SLASHES, trailingSlash === 'never' ? '' : '/');
-			return path ? path : '/';
+		const path = collapseDuplicateTrailingSlashes(pathname, trailingSlash !== 'never');
+		if (path !== pathname) {
+			return path;
 		}
 
 		if (trailingSlash === 'ignore') {
-			return;
+			return pathname;
 		}
 
-		// Add a trailing slash if it's not a file
-		const hasTrailingSlash = pathname.endsWith('/');
-		if (trailingSlash === 'always' && !hasTrailingSlash && !hasFileExtension(pathname)) {
-			return `${pathname}/`;
+		if (trailingSlash === 'always' && !hasFileExtension(pathname)) {
+			return appendForwardSlash(pathname);
 		}
-		if (trailingSlash === 'never' && hasTrailingSlash) {
-			return pathname.slice(0, -1);
+		if (trailingSlash === 'never') {
+			return removeTrailingForwardSlash(pathname);
 		}
+
+		return pathname;
 	}
 
 	async render(request: Request, renderOptions?: RenderOptions): Promise<Response> {
@@ -290,7 +289,7 @@ export class App {
 		const url = new URL(request.url);
 		const redirect = this.#redirectTrailingSlash(url.pathname);
 
-		if (redirect) {
+		if (redirect !== url.pathname) {
 			const status = request.method === 'GET' ? 301 : 308;
 			return new Response(redirectTemplate({ status, location: redirect, from: request.url }), {
 				status,

packages/astro/src/core/constants.ts

@@ -101,4 +101,3 @@ export const SUPPORTED_MARKDOWN_FILE_EXTENSIONS = [
 // The folder name where to find the middleware
 export const MIDDLEWARE_PATH_SEGMENT_NAME = 'middleware';
 
-export const MANY_SLASHES = /\/{2,}/g;

packages/astro/src/vite-plugin-astro-server/base.ts

@@ -3,14 +3,15 @@ import type { AstroSettings } from '../types/astro.js';
 
 import * as fs from 'node:fs';
 import path from 'node:path';
-import { appendForwardSlash } from '@astrojs/internal-helpers/path';
+import {
+	appendForwardSlash,
+	collapseDuplicateTrailingSlashes,
+} from '@astrojs/internal-helpers/path';
 import { bold } from 'kleur/colors';
 import type { Logger } from '../core/logger/core.js';
 import notFoundTemplate, { subpathNotUsedTemplate } from '../template/4xx.js';
 import { writeHtmlResponse, writeRedirectResponse } from './response.js';
 
-const manySlashes = /\/{2,}$/;
-
 export function baseMiddleware(
 	settings: AstroSettings,
 	logger: Logger,
@@ -23,8 +24,8 @@ export function baseMiddleware(
 
 	return function devBaseMiddleware(req, res, next) {
 		const url = req.url!;
-		if (manySlashes.test(url)) {
-			const destination = url.replace(manySlashes, '/');
+		const destination = collapseDuplicateTrailingSlashes(url, true);
+		if (destination !== url) {
 			return writeRedirectResponse(res, 301, destination);
 		}
 		let pathname: string;

packages/astro/test/ssr-trailing-slash.js

@@ -25,6 +25,14 @@ describe('Redirecting trailing slashes in SSR', () => {
 			assert.equal(response.headers.get('Location'), '/another/');
 		});
 
+		it('Redirects to collapse multiple trailing slashes', async () => {
+			const app = await fixture.loadTestAdapterApp();
+			const request = new Request('http://example.com/another///');
+			const response = await app.render(request);
+			assert.equal(response.status, 301);
+			assert.equal(response.headers.get('Location'), '/another/');
+		});
+
 		it('Does not redirect when trailing slash is present', async () => {
 			const app = await fixture.loadTestAdapterApp();
 			const request = new Request('http://example.com/another/');
@@ -121,6 +129,14 @@ describe('Redirecting trailing slashes in SSR', () => {
 			assert.equal(response.headers.get('Location'), '/another');
 		});
 
+		it('Redirects to collapse multiple trailing slashes', async () => {
+			const app = await fixture.loadTestAdapterApp();
+			const request = new Request('http://example.com/another///');
+			const response = await app.render(request);
+			assert.equal(response.status, 301);
+			assert.equal(response.headers.get('Location'), '/another');
+		});
+
 		it('Does not redirect when trailing slash is absent', async () => {
 			const app = await fixture.loadTestAdapterApp();
 			const request = new Request('http://example.com/another');
@@ -202,6 +218,14 @@ describe('Redirecting trailing slashes in SSR', () => {
 			await fixture.build();
 		});
 
+		it("Redirects to collapse multiple trailing slashes", async () => {
+			const app = await fixture.loadTestAdapterApp();
+			const request = new Request('http://example.com/another///');
+			const response = await app.render(request);
+			assert.equal(response.status, 301);
+			assert.equal(response.headers.get('Location'), '/another/');
+		});
+
 		it('Does not redirect when trailing slash is absent', async () => {
 			const app = await fixture.loadTestAdapterApp();
 			const request = new Request('http://example.com/another');

packages/internal-helpers/src/path.ts

@@ -19,6 +19,13 @@ export function collapseDuplicateSlashes(path: string) {
 	return path.replace(/(?<!:)\/{2,}/g, '/');
 }
 
+export const MANY_TRAILING_SLASHES = /\/{2,}/g;
+
+export function collapseDuplicateTrailingSlashes(path: string, trailingSlash: boolean) {
+	const pathname = path.replace(MANY_TRAILING_SLASHES, trailingSlash ? '/' : '');
+	return pathname ? pathname : '/';
+}
+
 export function removeTrailingForwardSlash(path: string) {
 	return path.endsWith('/') ? path.slice(0, path.length - 1) : path;
 }
@@ -105,8 +112,8 @@ export function removeBase(path: string, base: string) {
 	return path;
 }
 
-const withFileExt = /\/[^/]+\.\w+$/;
+const WITH_FILE_EXT = /\/[^/]+\.\w+$/;
 
 export function hasFileExtension(path: string) {
-	return withFileExt.test(path);
+	return WITH_FILE_EXT.test(path);
 }