From d07d2f7ac9d87af907beaca700ba4116dc1d6f37 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Thu, 13 Jun 2024 10:41:01 +0100 Subject: [PATCH] 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 * Update packages/astro/src/core/constants.ts Co-authored-by: Florian Lefebvre * changeset * relax the assertion * Update packages/astro/src/vite-plugin-astro-server/route.ts Co-authored-by: Florian Lefebvre * relax the assertion --------- Co-authored-by: Florian Lefebvre --- .changeset/olive-feet-eat.md | 9 +++ packages/astro/src/core/constants.ts | 5 ++ .../routing/astro-designed-error-pages.ts | 16 +++- .../src/vite-plugin-astro-server/route.ts | 23 +++++- packages/astro/test/core-image.test.js | 4 +- .../astro.config.mjs | 9 +++ .../package.json | 8 ++ .../src/pages/500.astro | 4 + .../src/pages/errors/start.astro | 3 + .../src/pages/errors/throw.astro | 3 + packages/astro/test/rewrite.test.js | 81 ++++++++++++++++++- pnpm-lock.yaml | 6 ++ 12 files changed, 163 insertions(+), 8 deletions(-) create mode 100644 .changeset/olive-feet-eat.md create mode 100644 packages/astro/test/fixtures/rewrite-runtime-error-custom500/astro.config.mjs create mode 100644 packages/astro/test/fixtures/rewrite-runtime-error-custom500/package.json create mode 100644 packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/500.astro create mode 100644 packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/start.astro create mode 100644 packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/throw.astro diff --git a/.changeset/olive-feet-eat.md b/.changeset/olive-feet-eat.md new file mode 100644 index 000000000000..6984b29563b7 --- /dev/null +++ b/.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. diff --git a/packages/astro/src/core/constants.ts b/packages/astro/src/core/constants.ts index a4d32abe503c..0930ea6f0f7d 100644 --- a/packages/astro/src/core/constants.ts +++ b/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. diff --git a/packages/astro/src/core/routing/astro-designed-error-pages.ts b/packages/astro/src/core/routing/astro-designed-error-pages.ts index 6a047f6d55d9..f6785cbdcb00 100644 --- a/packages/astro/src/core/routing/astro-designed-error-pages.ts +++ b/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); diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index b451a8ff6395..b05412314309 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/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)) { diff --git a/packages/astro/test/core-image.test.js b/packages/astro/test/core-image.test.js index 17c08db21ed2..a4fd13fcbdcb 100644 --- a/packages/astro/test/core-image.test.js +++ b/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); }); diff --git a/packages/astro/test/fixtures/rewrite-runtime-error-custom500/astro.config.mjs b/packages/astro/test/fixtures/rewrite-runtime-error-custom500/astro.config.mjs new file mode 100644 index 000000000000..af13ef19b477 --- /dev/null +++ b/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" +}); diff --git a/packages/astro/test/fixtures/rewrite-runtime-error-custom500/package.json b/packages/astro/test/fixtures/rewrite-runtime-error-custom500/package.json new file mode 100644 index 000000000000..6d844adc4e14 --- /dev/null +++ b/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:*" + } +} diff --git a/packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/500.astro b/packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/500.astro new file mode 100644 index 000000000000..7df3fc458c43 --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/500.astro @@ -0,0 +1,4 @@ +--- +--- + +

I am the custom 500

diff --git a/packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/start.astro b/packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/start.astro new file mode 100644 index 000000000000..0fff9a7e7ee0 --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/start.astro @@ -0,0 +1,3 @@ +--- +return Astro.rewrite("/errors/throw") +--- diff --git a/packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/throw.astro b/packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/throw.astro new file mode 100644 index 000000000000..1f879a9b4d21 --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-runtime-error-custom500/src/pages/errors/throw.astro @@ -0,0 +1,3 @@ +--- +throw new Error("Fancy error") +--- diff --git a/packages/astro/test/rewrite.test.js b/packages/astro/test/rewrite.test.js index db2830608889..d0d23763983d 100644 --- a/packages/astro/test/rewrite.test.js +++ b/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'); }); }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a47a30e5bd15..ef034d50c901 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -3493,6 +3493,12 @@ importers: specifier: workspace:* version: link:../../.. + packages/astro/test/fixtures/rewrite-runtime-error-custom500: + dependencies: + astro: + specifier: workspace:* + version: link:../../.. + packages/astro/test/fixtures/rewrite-server: dependencies: astro: