Merge branch 'main' into feature/support-launch-editor

.changeset/brave-cycles-suffer.md

@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Fixes a bug in the Astro rewrite logic, where rewriting the index with parameters - `next("/?foo=bar")` - didn't work as expected.

.changeset/cold-dolls-do.md

@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Handle image-size errors by displaying a clearer message

.changeset/lazy-rockets-raise.md

@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Fixes a bug where `astro build` didn't create custom `404.html` and `500.html` when a certain combination of i18n options was applied

.changeset/mean-geckos-sell.md

@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Update generator.ts to allow %23 (#) in dynamic urls

.changeset/silly-parents-repair.md

@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Improves debug logging for on-demand pages

packages/astro/src/assets/utils/metadata.ts

@@ -6,22 +6,28 @@ export async function imageMetadata(
 	data: Uint8Array,
 	src?: string
 ): Promise<Omit<ImageMetadata, 'src' | 'fsPath'>> {
-	const result = probe(data);
+	try {
+		const result = probe(data);
+		if (!result.height || !result.width || !result.type) {
+			throw new AstroError({
+				...AstroErrorData.NoImageMetadata,
+				message: AstroErrorData.NoImageMetadata.message(src),
+			});
+		}
 
-	if (!result.height || !result.width || !result.type) {
+		const { width, height, type, orientation } = result;
+		const isPortrait = (orientation || 0) >= 5;
+
+		return {
+			width: isPortrait ? height : width,
+			height: isPortrait ? width : height,
+			format: type as ImageInputFormat,
+			orientation,
+		};
+	} catch (e) {
 		throw new AstroError({
 			...AstroErrorData.NoImageMetadata,
 			message: AstroErrorData.NoImageMetadata.message(src),
 		});
 	}
-
-	const { width, height, type, orientation } = result;
-	const isPortrait = (orientation || 0) >= 5;
-
-	return {
-		width: isPortrait ? height : width,
-		height: isPortrait ? width : height,
-		format: type as ImageInputFormat,
-		orientation,
-	};
 }

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

@@ -280,6 +280,14 @@ export class App {
 				this.#logRenderOptionsDeprecationWarning();
 			}
 		}
+		if (routeData) {
+			this.#logger.debug(
+				'router',
+				'The adapter ' + this.#manifest.adapterName + ' provided a custom RouteData for ',
+				request.url
+			);
+			this.#logger.debug('router', 'RouteData:\n' + routeData);
+		}
 		if (locals) {
 			if (typeof locals !== 'object') {
 				this.#logger.error(null, new AstroError(AstroErrorData.LocalsNotAnObject).stack!);
@@ -296,8 +304,12 @@ export class App {
 		}
 		if (!routeData) {
 			routeData = this.match(request);
+			this.#logger.debug('router', 'Astro matched the following route for ' + request.url);
+			this.#logger.debug('router', 'RouteData:\n' + routeData);
 		}
 		if (!routeData) {
+			this.#logger.debug('router', "Astro hasn't found routes that match " + request.url);
+			this.#logger.debug('router', "Here's the available routes:\n", this.#manifestData);
 			return this.#renderError(request, { locals, status: 404 });
 		}
 		const pathname = this.#getPathnameFromRequest(request);

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

@@ -71,7 +71,10 @@ export class AppPipeline extends Pipeline {
 		return module.page();
 	}
 
-	async tryRewrite(payload: RewritePayload): Promise<[RouteData, ComponentInstance]> {
+	async tryRewrite(
+		payload: RewritePayload,
+		request: Request
+	): Promise<[RouteData, ComponentInstance]> {
 		let foundRoute;
 
 		for (const route of this.#manifestData!.routes) {
@@ -86,9 +89,12 @@ export class AppPipeline extends Pipeline {
 					foundRoute = route;
 					break;
 				}
-			} else if (route.pattern.test(decodeURI(payload))) {
-				foundRoute = route;
-				break;
+			} else {
+				const newUrl = new URL(payload, new URL(request.url).origin);
+				if (route.pattern.test(decodeURI(newUrl.pathname))) {
+					foundRoute = route;
+					break;
+				}
 			}
 		}
 

packages/astro/src/core/base-pipeline.ts

@@ -71,7 +71,10 @@ export abstract class Pipeline {
 	 *
 	 * @param {RewritePayload} rewritePayload
 	 */
-	abstract tryRewrite(rewritePayload: RewritePayload): Promise<[RouteData, ComponentInstance]>;
+	abstract tryRewrite(
+		rewritePayload: RewritePayload,
+		request: Request
+	): Promise<[RouteData, ComponentInstance]>;
 
 	/**
 	 * Tells the pipeline how to retrieve a component give a `RouteData`

packages/astro/src/core/build/pipeline.ts

@@ -276,7 +276,10 @@ export class BuildPipeline extends Pipeline {
 		}
 	}
 
-	async tryRewrite(payload: RewritePayload): Promise<[RouteData, ComponentInstance]> {
+	async tryRewrite(
+		payload: RewritePayload,
+		request: Request
+	): Promise<[RouteData, ComponentInstance]> {
 		let foundRoute: RouteData | undefined;
 		// options.manifest is the actual type that contains the information
 		for (const route of this.options.manifest.routes) {
@@ -291,9 +294,12 @@ export class BuildPipeline extends Pipeline {
 					foundRoute = route;
 					break;
 				}
-			} else if (route.pattern.test(decodeURI(payload))) {
-				foundRoute = route;
-				break;
+			} else {
+				const newUrl = new URL(payload, new URL(request.url).origin);
+				if (route.pattern.test(decodeURI(newUrl.pathname))) {
+					foundRoute = route;
+					break;
+				}
 			}
 		}
 		if (foundRoute) {

packages/astro/src/core/render-context.ts

@@ -4,7 +4,6 @@ import type {
 	AstroGlobalPartial,
 	ComponentInstance,
 	MiddlewareHandler,
-	MiddlewareNext,
 	RewritePayload,
 	RouteData,
 	SSRResult,
@@ -118,7 +117,7 @@ export class RenderContext {
 			if (payload) {
 				if (this.pipeline.manifest.rewritingEnabled) {
 					try {
-						const [routeData, component] = await pipeline.tryRewrite(payload);
+						const [routeData, component] = await pipeline.tryRewrite(payload, this.request);
 						this.routeData = routeData;
 						componentInstance = component;
 					} catch (e) {
@@ -212,7 +211,7 @@ export class RenderContext {
 		const rewrite = async (reroutePayload: RewritePayload) => {
 			pipeline.logger.debug('router', 'Called rewriting to:', reroutePayload);
 			try {
-				const [routeData, component] = await pipeline.tryRewrite(reroutePayload);
+				const [routeData, component] = await pipeline.tryRewrite(reroutePayload, this.request);
 				this.routeData = routeData;
 				if (reroutePayload instanceof Request) {
 					this.request = reroutePayload;
@@ -398,7 +397,7 @@ export class RenderContext {
 		const rewrite = async (reroutePayload: RewritePayload) => {
 			try {
 				pipeline.logger.debug('router', 'Calling rewrite: ', reroutePayload);
-				const [routeData, component] = await pipeline.tryRewrite(reroutePayload);
+				const [routeData, component] = await pipeline.tryRewrite(reroutePayload, this.request);
 				this.routeData = routeData;
 				if (reroutePayload instanceof Request) {
 					this.request = reroutePayload;

packages/astro/src/core/routing/manifest/generator.ts

@@ -2,6 +2,24 @@ import type { AstroConfig, RoutePart } from '../../../@types/astro.js';
 
 import { compile } from 'path-to-regexp';
 
+/**
+ * Sanitizes the parameters object by normalizing string values and replacing certain characters with their URL-encoded equivalents.
+ * @param {Record<string, string | number | undefined>} params - The parameters object to be sanitized.
+ * @returns {Record<string, string | number | undefined>} The sanitized parameters object.
+ */
+function sanitizeParams(
+	params: Record<string, string | number | undefined>
+): Record<string, string | number | undefined> {
+	return Object.fromEntries(
+		Object.entries(params).map(([key, value]) => {
+			if (typeof value === 'string') {
+				return [key, value.normalize().replace(/#/g, '%23').replace(/\?/g, '%3F')];
+			}
+			return [key, value];
+		})
+	);
+}
+
 export function getRouteGenerator(
 	segments: RoutePart[][],
 	addTrailingSlash: AstroConfig['trailingSlash']
@@ -37,8 +55,9 @@ export function getRouteGenerator(
 		trailing = '/';
 	}
 	const toPath = compile(template + trailing);
-	return (params: object): string => {
-		const path = toPath(params);
+	return (params: Record<string, string | number | undefined>): string => {
+		const sanitizedParams = sanitizeParams(params);
+		const path = toPath(sanitizedParams);
 
 		// When generating an index from a rest parameter route, `path-to-regexp` will return an
 		// empty string instead "/". This causes an inconsistency with static indexes that may result

packages/astro/src/i18n/index.ts

@@ -19,6 +19,11 @@ export function requestHasLocale(locales: Locales) {
 	};
 }
 
+export function requestIs404Or500(request: Request, base = '') {
+	const url = new URL(request.url);
+
+	return url.pathname.startsWith(`${base}/404`) || url.pathname.startsWith(`${base}/500`);
+}
 // Checks if the pathname has any locale
 export function pathHasLocale(path: string, locales: Locales): boolean {
 	const segments = path.split('/');
@@ -317,7 +322,7 @@ export function notFound({ base, locales }: MiddlewarePayload) {
 		if (!(isRoot || pathHasLocale(url.pathname, locales))) {
 			if (response) {
 				response.headers.set(REROUTE_DIRECTIVE_HEADER, 'no');
-				return new Response(null, {
+				return new Response(response.body, {
 					status: 404,
 					headers: response.headers,
 				});

packages/astro/src/i18n/middleware.ts

@@ -8,6 +8,7 @@ import {
 	redirectToDefaultLocale,
 	redirectToFallback,
 	requestHasLocale,
+	requestIs404Or500,
 } from './index.js';
 
 export function createI18nMiddleware(
@@ -69,6 +70,11 @@ export function createI18nMiddleware(
 			return response;
 		}
 
+		// 404 and 500 are **known** routes (users can have their custom pages), so we need to let them be
+		if (requestIs404Or500(context.request, base)) {
+			return response;
+		}
+
 		const { currentLocale } = context;
 
 		switch (i18n.strategy) {

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

@@ -191,7 +191,10 @@ export class DevPipeline extends Pipeline {
 		}
 	}
 
-	async tryRewrite(payload: RewritePayload): Promise<[RouteData, ComponentInstance]> {
+	async tryRewrite(
+		payload: RewritePayload,
+		request: Request
+	): Promise<[RouteData, ComponentInstance]> {
 		let foundRoute;
 		if (!this.manifestData) {
 			throw new Error('Missing manifest data. This is an internal error, please file an issue.');
@@ -209,9 +212,12 @@ export class DevPipeline extends Pipeline {
 					foundRoute = route;
 					break;
 				}
-			} else if (route.pattern.test(decodeURI(payload))) {
-				foundRoute = route;
-				break;
+			} else {
+				const newUrl = new URL(payload, new URL(request.url).origin);
+				if (route.pattern.test(decodeURI(newUrl.pathname))) {
+					foundRoute = route;
+					break;
+				}
 			}
 		}
 

packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/500.astro

@@ -0,0 +1,4 @@
+<html>
+    <head><title>Error</title></head>
+    <body>Unexpected error.</body>
+</html>

packages/astro/test/fixtures/reroute/src/middleware.js

@@ -18,6 +18,10 @@ export const second = async (context, next) => {
 		if (context.url.pathname.includes('/auth/base')) {
 			return await next('/');
 		}
+
+		if (context.url.pathname.includes('/auth/params')) {
+			return  next('/?foo=bar');
+		}
 	}
 	return next();
 };

packages/astro/test/fixtures/reroute/src/pages/auth/params.astro

@@ -0,0 +1,10 @@
+---
+---
+<html>
+<head>
+	<title>Index with params</title>
+</head>
+<body>
+<h1>Index with params</h1>
+</body>
+</html>

packages/astro/test/i18n-routing-manual-with-default-middleware.test.js

@@ -26,7 +26,7 @@ describe('Dev server manual routing', () => {
 		const response = await fixture.fetch('/blog');
 		const text = await response.text();
 		assert.equal(response.status, 404);
-		assert.equal(text.includes('Blog should not render'), false);
+		assert.match(text, /Blog should not render/);
 	});
 
 	it('should return a 200 because the custom middleware allows it', async () => {
@@ -83,7 +83,8 @@ describe('SSR manual routing', () => {
 		let request = new Request('http://example.com/blog');
 		let response = await app.render(request);
 		assert.equal(response.status, 404);
-		assert.equal((await response.text()).includes('Blog should not render'), false);
+		const text = await response.text();
+		assert.match(text, /Blog should not render/);
 	});
 
 	it('should return a 200 because the custom middleware allows it', async () => {

packages/astro/test/i18n-routing.test.js

@@ -580,6 +580,16 @@ describe('[SSG] i18n routing', () => {
 			assert.equal($('body').text().includes('Lo siento'), true);
 		});
 
+		it('should create a custom 404.html and 505.html', async () => {
+			let html = await fixture.readFile('/404.html');
+			let $ = cheerio.load(html);
+			assert.equal($('body').text().includes("Can't find the page you're looking for."), true);
+
+			html = await fixture.readFile('/500.html');
+			$ = cheerio.load(html);
+			assert.equal($('body').text().includes('Unexpected error.'), true);
+		});
+
 		it("should NOT render the default locale if there isn't a fallback and the route is missing", async () => {
 			try {
 				await fixture.readFile('/it/start/index.html');

packages/astro/test/rewrite.test.js

@@ -220,4 +220,11 @@ describe('Middleware', () => {
 		assert.equal($('h1').text(), 'Index');
 		assert.equal($('p').text(), '');
 	});
+
+	it('should render the index when rewriting with params', async () => {
+		const html = await fixture.fetch('/auth/params').then((res) => res.text());
+		const $ = cheerioLoad(html);
+
+		assert.match($('h1').text(), /Index/);
+	});
 });

packages/astro/test/ssr-params.test.js

@@ -42,11 +42,11 @@ describe('Astro.params in SSR', () => {
 
 	it('No double URL decoding', async () => {
 		const app = await fixture.loadTestAdapterApp();
-		const request = new Request('http://example.com/users/houston/%25');
+		const request = new Request('http://example.com/users/houston/%25%23%3F');
 		const response = await app.render(request);
 		assert.equal(response.status, 200);
 		const html = await response.text();
 		const $ = cheerio.load(html);
-		assert.equal($('.category').text(), '%');
+		assert.equal($('.category').text(), '%#?');
 	});
 });