fix(rewrite): allow to rewrite 404 and take base into consideration (…

…#11272) * fix(rewrite): allow to rewrite 404 * add changesets * rebase * apply suggestion * Update .changeset/honest-shirts-trade.md Co-authored-by: Florian Lefebvre <[email protected]> --------- Co-authored-by: Florian Lefebvre <[email protected]>
withastro · Jun 19, 2024 · ea987d7 · ea987d7
1 parent bc3c329
commit ea987d7
Show file tree

Hide file tree

Showing 15 changed files with 215 additions and 197 deletions.
diff --git a/.changeset/five-owls-check.md b/.changeset/five-owls-check.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes a case where rewriting `/` would cause an issue, when `trailingSlash` was set to `"never"`.
diff --git a/.changeset/honest-shirts-trade.md b/.changeset/honest-shirts-trade.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Reverts a logic where it wasn't possible to rewrite `/404` in static mode. It's **now possible** again
diff --git a/packages/astro/src/container/pipeline.ts b/packages/astro/src/container/pipeline.ts
@@ -7,13 +7,14 @@ import type {
 } from '../@types/astro.js';
 import { type HeadElements, Pipeline } from '../core/base-pipeline.js';
 import type { SinglePageBuiltModule } from '../core/build/types.js';
-import { InvalidRewrite404, RouteNotFound } from '../core/errors/errors-data.js';
+import { RouteNotFound } from '../core/errors/errors-data.js';
 import { AstroError } from '../core/errors/index.js';
 import {
 	createModuleScriptElement,
 	createStylesheetElementSet,
 } from '../core/render/ssr-element.js';
-import { DEFAULT_404_ROUTE, default404Page } from '../core/routing/astro-designed-error-pages.js';
+import { DEFAULT_404_ROUTE } from '../core/routing/astro-designed-error-pages.js';
+import {findRouteToRewrite} from "../core/routing/rewrite.js";
 
 export class ContainerPipeline extends Pipeline {
 	/**
@@ -74,31 +75,17 @@ export class ContainerPipeline extends Pipeline {
 		payload: RewritePayload,
 		request: Request
 	): Promise<[RouteData, ComponentInstance, URL]> {
-		let foundRoute: RouteData | undefined;
-		// options.manifest is the actual type that contains the information
-		let finalUrl: URL | undefined = undefined;
-		for (const route of this.manifest.routes) {
-			if (payload instanceof URL) {
-				finalUrl = payload;
-			} else if (payload instanceof Request) {
-				finalUrl = new URL(payload.url);
-			} else {
-				finalUrl = new URL(payload, new URL(request.url).origin);
-			}
-			if (route.routeData.pattern.test(decodeURI(finalUrl.pathname))) {
-				foundRoute = route.routeData;
-				break;
-			} else if (finalUrl.pathname === '/404') {
-				foundRoute = DEFAULT_404_ROUTE;
-				break;
-			}
-		}
-		if (foundRoute && finalUrl) {
-			const componentInstance = await this.getComponentByRoute(foundRoute);
-			return [foundRoute, componentInstance, finalUrl];
-		} else {
-			throw new AstroError(RouteNotFound);
-		}
+		const [foundRoute, finalUrl] = findRouteToRewrite({
+			payload,
+			request,
+			routes: this.manifest?.routes.map((r) => r.routeData),
+			trailingSlash: this.manifest.trailingSlash,
+			buildFormat: this.manifest.buildFormat,
+			base: this.manifest.base,
+		});
+
+		const componentInstance = await this.getComponentByRoute(foundRoute);
+		return [foundRoute, componentInstance, finalUrl];
 	}
 
 	insertRoute(route: RouteData, componentInstance: ComponentInstance): void {
@@ -114,12 +101,4 @@ export class ContainerPipeline extends Pipeline {
 	// At the moment it's not used by the container via any public API
 	// @ts-expect-error It needs to be implemented.
 	async getComponentByRoute(_routeData: RouteData): Promise<ComponentInstance> {}
-
-	rewriteKnownRoute(pathname: string, _sourceRoute: RouteData): ComponentInstance {
-		if (pathname === '/404') {
-			return { default: default404Page } as any as ComponentInstance;
-		}
-
-		throw new AstroError(InvalidRewrite404);
-	}
 }
diff --git a/packages/astro/src/core/app/pipeline.ts b/packages/astro/src/core/app/pipeline.ts
@@ -14,6 +14,7 @@ import { AstroError } from '../errors/index.js';
 import { RedirectSinglePageBuiltModule } from '../redirects/component.js';
 import { createModuleScriptElement, createStylesheetElementSet } from '../render/ssr-element.js';
 import { DEFAULT_404_ROUTE } from '../routing/astro-designed-error-pages.js';
+import { findRouteToRewrite } from '../routing/rewrite.js';
 
 export class AppPipeline extends Pipeline {
 	#manifestData: ManifestData | undefined;
@@ -86,42 +87,19 @@ export class AppPipeline extends Pipeline {
 	async tryRewrite(
 		payload: RewritePayload,
 		request: Request,
-		sourceRoute: RouteData
+		_sourceRoute: RouteData
 	): Promise<[RouteData, ComponentInstance, URL]> {
-		let foundRoute;
-
-		let finalUrl: URL | undefined = undefined;
-		for (const route of this.#manifestData!.routes) {
-			if (payload instanceof URL) {
-				finalUrl = payload;
-			} else if (payload instanceof Request) {
-				finalUrl = new URL(payload.url);
-			} else {
-				finalUrl = new URL(payload, new URL(request.url).origin);
-			}
-
-			if (route.pattern.test(decodeURI(finalUrl.pathname))) {
-				foundRoute = route;
-				break;
-			} else if (finalUrl.pathname === '/404') {
-				foundRoute = DEFAULT_404_ROUTE;
-				break;
-			}
-		}
-
-		if (foundRoute && finalUrl) {
-			if (foundRoute.pathname === '/404') {
-				const componentInstance = this.rewriteKnownRoute(foundRoute.pathname, sourceRoute);
-				return [foundRoute, componentInstance, finalUrl];
-			} else {
-				const componentInstance = await this.getComponentByRoute(foundRoute);
-				return [foundRoute, componentInstance, finalUrl];
-			}
-		}
-		throw new AstroError({
-			...RewriteEncounteredAnError,
-			message: RewriteEncounteredAnError.message(payload.toString()),
+		const [foundRoute, finalUrl] = findRouteToRewrite({
+			payload,
+			request,
+			routes: this.manifest?.routes.map((r) => r.routeData),
+			trailingSlash: this.manifest.trailingSlash,
+			buildFormat: this.manifest.buildFormat,
+			base: this.manifest.base,
 		});
+
+		const componentInstance = await this.getComponentByRoute(foundRoute);
+		return [foundRoute, componentInstance, finalUrl];
 	}
 
 	async getModuleForRoute(route: RouteData): Promise<SinglePageBuiltModule> {
@@ -151,13 +129,4 @@ export class AppPipeline extends Pipeline {
 			);
 		}
 	}
-
-	// We don't need to check the source route, we already are in SSR
-	rewriteKnownRoute(pathname: string, _sourceRoute: RouteData): ComponentInstance {
-		if (pathname === '/404') {
-			return { default: () => new Response(null, { status: 404 }) } as ComponentInstance;
-		} else {
-			return { default: () => new Response(null, { status: 500 }) } as ComponentInstance;
-		}
-	}
 }
diff --git a/packages/astro/src/core/base-pipeline.ts b/packages/astro/src/core/base-pipeline.ts
@@ -95,16 +95,6 @@ export abstract class Pipeline {
 	 * @param routeData
 	 */
 	abstract getComponentByRoute(routeData: RouteData): Promise<ComponentInstance>;
-
-	/**
-	 * Attempts to execute a rewrite of a known Astro route:
-	 * - /404 -> src/pages/404.astro -> template
-	 * - /500 -> src/pages/500.astro
-	 *
-	 * @param pathname The pathname where the user wants to rewrite e.g. "/404"
-	 * @param sourceRoute The original route where the first request started. This is needed in case a pipeline wants to check if the current route is pre-rendered or not
-	 */
-	abstract rewriteKnownRoute(pathname: string, sourceRoute: RouteData): ComponentInstance;
 }
 
 // eslint-disable-next-line @typescript-eslint/no-empty-interface

diff --git a/packages/astro/src/core/build/pipeline.ts b/packages/astro/src/core/build/pipeline.ts
@@ -8,8 +8,6 @@ import type {
 import { getOutputDirectory } from '../../prerender/utils.js';
 import { BEFORE_HYDRATION_SCRIPT_ID, PAGE_SCRIPT_ID } from '../../vite-plugin-scripts/index.js';
 import type { SSRManifest } from '../app/types.js';
-import { InvalidRewrite404, RewriteEncounteredAnError } from '../errors/errors-data.js';
-import { AstroError } from '../errors/index.js';
 import { routeIsFallback, routeIsRedirect } from '../redirects/helpers.js';
 import { RedirectSinglePageBuiltModule } from '../redirects/index.js';
 import { Pipeline } from '../render/index.js';
@@ -18,7 +16,6 @@ import {
 	createModuleScriptsSet,
 	createStylesheetElementSet,
 } from '../render/ssr-element.js';
-import { DEFAULT_404_ROUTE } from '../routing/astro-designed-error-pages.js';
 import { isServerLikeOutput } from '../util.js';
 import { getOutDirWithinCwd } from './common.js';
 import { type BuildInternals, cssOrder, getPageData, mergeInlineCss } from './internal.js';
@@ -27,6 +24,9 @@ import { RESOLVED_SPLIT_MODULE_ID } from './plugins/plugin-ssr.js';
 import { getPagesFromVirtualModulePageName, getVirtualModulePageName } from './plugins/util.js';
 import type { PageBuildData, SinglePageBuiltModule, StaticBuildOptions } from './types.js';
 import { i18nHasFallback } from './util.js';
+import { findRouteToRewrite } from '../routing/rewrite.js';
+import {DEFAULT_404_COMPONENT} from "../constants.js";
+import {default404Page} from "../routing/astro-designed-error-pages.js";
 
 /**
  * The build pipeline is responsible to gather the files emitted by the SSR build and generate the pages by executing these files.
@@ -269,6 +269,8 @@ export class BuildPipeline extends Pipeline {
 			// SAFETY: checked before
 			const entry = this.#componentsInterner.get(routeData)!;
 			return await entry.page();
+		} else if (routeData.component === DEFAULT_404_COMPONENT) {
+			return { default: default404Page }
 		} else {
 			// SAFETY: the pipeline calls `retrieveRoutesToGenerate`, which is in charge to fill the cache.
 			const filePath = this.#routesByFilePath.get(routeData)!;
@@ -280,42 +282,19 @@ export class BuildPipeline extends Pipeline {
 	async tryRewrite(
 		payload: RewritePayload,
 		request: Request,
-		sourceRoute: RouteData
+		_sourceRoute: RouteData
 	): Promise<[RouteData, ComponentInstance, URL]> {
-		let foundRoute: RouteData | undefined;
-		// options.manifest is the actual type that contains the information
-		let finalUrl: URL | undefined = undefined;
-		for (const route of this.options.manifest.routes) {
-			if (payload instanceof URL) {
-				finalUrl = payload;
-			} else if (payload instanceof Request) {
-				finalUrl = new URL(payload.url);
-			} else {
-				finalUrl = new URL(payload, new URL(request.url).origin);
-			}
+		const [foundRoute, finalUrl] = findRouteToRewrite({
+			payload,
+			request,
+			routes: this.options.manifest.routes,
+			trailingSlash: this.config.trailingSlash,
+			buildFormat: this.config.build.format,
+			base: this.config.base
+		});
 
-			if (route.pattern.test(decodeURI(finalUrl.pathname))) {
-				foundRoute = route;
-				break;
-			} else if (finalUrl.pathname === '/404') {
-				foundRoute = DEFAULT_404_ROUTE;
-				break;
-			}
-		}
-		if (foundRoute && finalUrl) {
-			if (foundRoute.pathname === '/404') {
-				const componentInstance = await this.rewriteKnownRoute(foundRoute.pathname, sourceRoute);
-				return [foundRoute, componentInstance, finalUrl];
-			} else {
-				const componentInstance = await this.getComponentByRoute(foundRoute);
-				return [foundRoute, componentInstance, finalUrl];
-			}
-		} else {
-			throw new AstroError({
-				...RewriteEncounteredAnError,
-				message: RewriteEncounteredAnError.message(payload.toString()),
-			});
-		}
+		const componentInstance = await this.getComponentByRoute(foundRoute);
+		return [foundRoute, componentInstance, finalUrl];
 	}
 
 	async retrieveSsrEntry(route: RouteData, filePath: string): Promise<SinglePageBuiltModule> {
@@ -375,13 +354,6 @@ export class BuildPipeline extends Pipeline {
 
 		return RedirectSinglePageBuiltModule;
 	}
-
-	rewriteKnownRoute(_pathname: string, sourceRoute: RouteData): ComponentInstance {
-		if (!isServerLikeOutput(this.config) || sourceRoute.prerender) {
-			throw new AstroError(InvalidRewrite404);
-		}
-		throw new Error(`Unreachable, in SSG this route shouldn't be generated`);
-	}
 }
 
 function createEntryURL(filePath: string, outFolder: URL) {

diff --git a/packages/astro/src/core/errors/errors-data.ts b/packages/astro/src/core/errors/errors-data.ts
@@ -1155,18 +1155,6 @@ export const RewriteEncounteredAnError = {
 		}.`,
 } satisfies ErrorData;
 
-/**
- * @docs
- * @description
- *
- * The user tried to rewrite a 404 page inside a static page.
- */
-export const InvalidRewrite404 = {
-	name: 'InvalidRewrite404',
-	title: "You attempted to rewrite a 404 inside a static page, and this isn't allowed.",
-	message: 'Rewriting a 404 is only allowed inside on-demand pages.',
-} satisfies ErrorData;
-
 /**
  * @docs
  * @description

diff --git a/packages/astro/src/core/routing/rewrite.ts b/packages/astro/src/core/routing/rewrite.ts
@@ -0,0 +1,55 @@
+import type { AstroConfig, RewritePayload, RouteData } from '../../@types/astro.js';
+import { shouldAppendForwardSlash } from '../build/util.js';
+import { DEFAULT_404_ROUTE } from './astro-designed-error-pages.js';
+import { appendForwardSlash, removeTrailingForwardSlash } from '../path.js';
+
+export type FindRouteToRewrite = {
+	payload: RewritePayload;
+	routes: RouteData[];
+	request: Request;
+	trailingSlash: AstroConfig['trailingSlash'];
+	buildFormat: AstroConfig['build']['format'];
+	base: AstroConfig['base'];
+};
+
+export function findRouteToRewrite({
+	payload,
+	routes,
+	request,
+	trailingSlash,
+	buildFormat,
+	base,
+}: FindRouteToRewrite): [RouteData, URL] {
+	let finalUrl: URL | undefined = undefined;
+	if (payload instanceof URL) {
+		finalUrl = payload;
+	} else if (payload instanceof Request) {
+		finalUrl = new URL(payload.url);
+	} else {
+		finalUrl = new URL(payload, new URL(request.url).origin);
+	}
+
+	let foundRoute;
+	for (const route of routes) {
+		const pathname = shouldAppendForwardSlash(trailingSlash, buildFormat)
+			? appendForwardSlash(finalUrl.pathname)
+			: base !== '/'
+				? removeTrailingForwardSlash(finalUrl.pathname)
+				: finalUrl.pathname;
+		if (route.pattern.test(decodeURI(pathname))) {
+			foundRoute = route;
+			break;
+		}
+	}
+
+	if (foundRoute) {
+		return [foundRoute, finalUrl];
+	} else {
+		const custom404 = routes.find((route) => route.route === '/404');
+		if (custom404) {
+			return [custom404, finalUrl];
+		} else {
+			return [DEFAULT_404_ROUTE, finalUrl];
+		}
+	}
+}