fix: better DX for 500.astro in local development (#11244)

* wip

* catch error in route.ts

* catch error in route.ts

* chore: tidy up error cases

* log the original error

* rebase

* chore: reduce the scope of the 500 handling

* we should not have a default 500

* remove props

* remove unsure  function, not needed

* Update packages/astro/src/core/routing/astro-designed-error-pages.ts

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>

* Update packages/astro/src/core/constants.ts

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>

* changeset

* relax the assertion

* Update packages/astro/src/vite-plugin-astro-server/route.ts

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>

* relax the assertion

---------

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>

Author: ematipico

.changeset/olive-feet-eat.md

@@ -0,0 +1,9 @@
+---
+'astro': patch
+---
+
+Improves the developer experience of the custom `500.astro` page in development mode.
+
+Before, in development, an error thrown during the rendering phase would display the default error overlay, even when users had the `500.astro` page.
+
+Now, the development server will display the `500.astro` and the original error is logged in the console.

packages/astro/src/core/constants.ts

@@ -32,6 +32,11 @@ export const ROUTE_TYPE_HEADER = 'X-Astro-Route-Type';
  */
 export const DEFAULT_404_COMPONENT = 'astro-default-404.astro';
 
+/**
+ * The value of the `component` field of the default 500 page, which is used when there is no user-provided 404.astro page.
+ */
+export const DEFAULT_500_COMPONENT = 'astro-default-500.astro';
+
 /**
  * A response with one of these status codes will be rewritten
  * with the result of rendering the respective error page.

packages/astro/src/core/routing/astro-designed-error-pages.ts

@@ -1,6 +1,6 @@
 import type { ManifestData, RouteData } from '../../@types/astro.js';
 import notFoundTemplate from '../../template/4xx.js';
-import { DEFAULT_404_COMPONENT } from '../constants.js';
+import { DEFAULT_404_COMPONENT, DEFAULT_500_COMPONENT } from '../constants.js';
 
 export const DEFAULT_404_ROUTE: RouteData = {
 	component: DEFAULT_404_COMPONENT,
@@ -16,6 +16,20 @@ export const DEFAULT_404_ROUTE: RouteData = {
 	isIndex: false,
 };
 
+export const DEFAULT_500_ROUTE: RouteData = {
+	component: DEFAULT_500_COMPONENT,
+	generate: () => '',
+	params: [],
+	pattern: /\/500/,
+	prerender: false,
+	pathname: '/500',
+	segments: [[{ content: '500', dynamic: false, spread: false }]],
+	type: 'page',
+	route: '/500',
+	fallbackRoutes: [],
+	isIndex: false,
+};
+
 export function ensure404Route(manifest: ManifestData) {
 	if (!manifest.routes.some((route) => route.route === '/404')) {
 		manifest.routes.push(DEFAULT_404_ROUTE);

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

@@ -41,6 +41,11 @@ function getCustom404Route(manifestData: ManifestData): RouteData | undefined {
 	return manifestData.routes.find((r) => route404.test(r.route));
 }
 
+function getCustom500Route(manifestData: ManifestData): RouteData | undefined {
+	const route500 = /^\/500\/?$/;
+	return manifestData.routes.find((r) => route500.test(r.route));
+}
+
 export async function matchRoute(
 	pathname: string,
 	manifestData: ManifestData,
@@ -273,7 +278,22 @@ export async function handleRoute({
 		});
 	}
 
-	let response = await renderContext.render(mod);
+	let response;
+	try {
+		response = await renderContext.render(mod);
+	} catch (err: any) {
+		const custom500 = getCustom500Route(manifestData);
+		if (!custom500) {
+			throw err;
+		}
+		// Log useful information that the custom 500 page may not display unlike the default error overlay
+		logger.error('router', err.stack || err.message);
+		const filePath = new URL(`./${custom500.component}`, config.root);
+		const preloadedComponent = await pipeline.preload(custom500, filePath);
+		response = await renderContext.render(preloadedComponent);
+		status = 500;
+	}
+
 	if (isLoggedRequest(pathname)) {
 		const timeEnd = performance.now();
 		logger.info(
@@ -321,6 +341,7 @@ export async function handleRoute({
 		await writeSSRResult(request, response, incomingResponse);
 		return;
 	}
+
 	// Apply the `status` override to the response object before responding.
 	// Response.status is read-only, so a clone is required to override.
 	if (status && response.status !== status && (status === 404 || status === 500)) {

packages/astro/test/core-image.test.js

@@ -712,7 +712,7 @@ describe('astro:image', () => {
 			let res = await fixture.fetch('/get-image-empty');
 			await res.text();
 
-			assert.equal(logs.length, 1);
+			assert.equal(logs.length >= 1, true);
 			assert.equal(logs[0].message.includes('Expected getImage() parameter'), true);
 		});
 
@@ -721,7 +721,7 @@ describe('astro:image', () => {
 			let res = await fixture.fetch('/get-image-undefined');
 			await res.text();
 
-			assert.equal(logs.length, 1);
+			assert.equal(logs.length >= 1, true);
 			assert.equal(logs[0].message.includes('Expected `src` property'), true);
 		});
 

packages/astro/test/fixtures/rewrite-runtime-error-custom500/astro.config.mjs

@@ -0,0 +1,9 @@
+import { defineConfig } from 'astro/config';
+
+// https://astro.build/config
+export default defineConfig({
+	experimental: {
+		rewriting: true
+	},
+	site: "https://example.com"
+});

packages/astro/test/fixtures/rewrite-runtime-error-custom500/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@test/rewrite-runtime-errror-custom500",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "astro": "workspace:*"
+  }
+}

packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/500.astro

@@ -0,0 +1,4 @@
+---
+---
+
+<h1>I am the custom 500</h1>

packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/start.astro

@@ -0,0 +1,3 @@
+---
+return Astro.rewrite("/errors/throw")
+---

packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/throw.astro

@@ -0,0 +1,3 @@
+---
+throw new Error("Fancy error")
+---

packages/astro/test/rewrite.test.js

@@ -314,7 +314,7 @@ describe('Middleware', () => {
 	});
 });
 
-describe('Runtime error', () => {
+describe('Runtime error, default 500', () => {
 	/** @type {import('./test-utils').Fixture} */
 	let fixture;
 	let devServer;
@@ -330,10 +330,83 @@ describe('Runtime error', () => {
 		await devServer.stop();
 	});
 
-	it('should render the index page when navigating /reroute ', async () => {
-		const html = await fixture.fetch('/errors/from').then((res) => res.text());
+	it('should return a 500 status code, but not render the custom 500', async () => {
+		const response = await fixture.fetch('/errors/from');
+		assert.equal(response.status, 500);
+		const text = await response.text();
+		assert.match(text, /@vite\/client/);
+	});
+});
+
+describe('Runtime error in SSR, default 500', () => {
+	/** @type {import('./test-utils').Fixture} */
+	let fixture;
+	let app;
+
+	before(async () => {
+		fixture = await loadFixture({
+			root: './fixtures/rewrite-runtime-error/',
+			output: 'server',
+			adapter: testAdapter(),
+		});
+		await fixture.build();
+		app = await fixture.loadTestAdapterApp();
+	});
+
+	it('should return a 500 status code, but not render the custom 500', async () => {
+		const request = new Request('http://example.com/errors/from');
+		const response = await app.render(request);
+		const text = await response.text();
+		assert.equal(text, '');
+	});
+});
+
+describe('Runtime error in dev, custom 500', () => {
+	/** @type {import('./test-utils').Fixture} */
+	let fixture;
+	let devServer;
+
+	before(async () => {
+		fixture = await loadFixture({
+			root: './fixtures/rewrite-runtime-error-custom500/',
+		});
+		devServer = await fixture.startDevServer();
+	});
+
+	after(async () => {
+		await devServer.stop();
+	});
+
+	it('should render the custom 500 when rewriting a page that throws an error', async () => {
+		const response = await fixture.fetch('/errors/start');
+		assert.equal(response.status, 500);
+		const html = await response.text();
+		assert.match(html, /I am the custom 500/);
+	});
+});
+
+describe('Runtime error in SSR, custom 500', () => {
+	/** @type {import('./test-utils').Fixture} */
+	let fixture;
+	let app;
+
+	before(async () => {
+		fixture = await loadFixture({
+			root: './fixtures/rewrite-runtime-error-custom500/',
+			output: 'server',
+			adapter: testAdapter(),
+		});
+		await fixture.build();
+		app = await fixture.loadTestAdapterApp();
+	});
+
+	it('should render the custom 500 when rewriting a page that throws an error', async () => {
+		const request = new Request('http://example.com/errors/start');
+		const response = await app.render(request);
+		const html = await response.text();
+
 		const $ = cheerioLoad(html);
 
-		assert.equal($('title').text(), 'Error');
+		assert.equal($('h1').text(), 'I am the custom 500');
 	});
 });