From 9042be049157ce859355f911565bc0c3d68f0aa1 Mon Sep 17 00:00:00 2001 From: Florian Lefebvre Date: Wed, 19 Jun 2024 16:00:16 +0200 Subject: [PATCH] feat: 500.astro improvements (#11134) * feat: better error handling * feat: allow passing props to render context render * feat: work on tests * Update 500.astro * feat: test preview custom 500 * feat: test for custom 500 failing * feat: add changeset * Update rich-dolls-compete.md * Delete packages/astro/e2e/custom-500.test.js * Apply suggestions from code review Co-authored-by: Emanuele Stoppa * fix: merge * Update packages/astro/test/custom-500.test.js Co-authored-by: Emanuele Stoppa * Apply suggestions from code review Co-authored-by: Sarah Rainsberger * Update packages/astro/src/core/app/index.ts * feat: update --------- Co-authored-by: Emanuele Stoppa Co-authored-by: Sarah Rainsberger Co-authored-by: Matthew Phillips --- .changeset/rich-dolls-compete.md | 22 ++++ packages/astro/playwright.config.js | 7 +- packages/astro/src/core/app/index.ts | 23 +++- packages/astro/src/core/render-context.ts | 9 +- .../src/vite-plugin-astro-server/route.ts | 1 + packages/astro/test/custom-500.test.js | 122 ++++++++++++++++++ .../custom-500-failing/astro.config.mjs | 4 + .../fixtures/custom-500-failing/package.json | 8 ++ .../custom-500-failing/src/pages/500.astro | 19 +++ .../custom-500-failing/src/pages/index.astro | 12 ++ .../test/fixtures/custom-500/astro.config.mjs | 4 + .../test/fixtures/custom-500/package.json | 8 ++ .../fixtures/custom-500/src/pages/500.astro | 17 +++ .../fixtures/custom-500/src/pages/index.astro | 12 ++ pnpm-lock.yaml | 12 ++ 15 files changed, 270 insertions(+), 10 deletions(-) create mode 100644 .changeset/rich-dolls-compete.md create mode 100644 packages/astro/test/custom-500.test.js create mode 100644 packages/astro/test/fixtures/custom-500-failing/astro.config.mjs create mode 100644 packages/astro/test/fixtures/custom-500-failing/package.json create mode 100644 packages/astro/test/fixtures/custom-500-failing/src/pages/500.astro create mode 100644 packages/astro/test/fixtures/custom-500-failing/src/pages/index.astro create mode 100644 packages/astro/test/fixtures/custom-500/astro.config.mjs create mode 100644 packages/astro/test/fixtures/custom-500/package.json create mode 100644 packages/astro/test/fixtures/custom-500/src/pages/500.astro create mode 100644 packages/astro/test/fixtures/custom-500/src/pages/index.astro diff --git a/.changeset/rich-dolls-compete.md b/.changeset/rich-dolls-compete.md new file mode 100644 index 0000000000..1254be9f78 --- /dev/null +++ b/.changeset/rich-dolls-compete.md @@ -0,0 +1,22 @@ +--- +"astro": minor +--- + +Improves the developer experience of the `500.astro` file by passing it a new `error` prop. + +When an error is thrown, the special `src/pages/500.astro` page now automatically receives the error as a prop. This allows you to display more specific information about the error on a custom 500 page. + +```astro +--- +// src/pages/500.astro +interface Props { + error: unknown +} + +const { error } = Astro.props +--- + +
{error instanceof Error ? error.message : 'Unknown error'}
+``` + +If an error occurs rendering this page, your host's default 500 error page will be shown to your visitor in production, and Astro's default error overlay will be shown in development. diff --git a/packages/astro/playwright.config.js b/packages/astro/playwright.config.js index 1f6386f6ca..5aacd6d018 100644 --- a/packages/astro/playwright.config.js +++ b/packages/astro/playwright.config.js @@ -1,9 +1,10 @@ +import { defineConfig } from '@playwright/test'; // NOTE: Sometimes, tests fail with `TypeError: process.stdout.clearLine is not a function` // for some reason. This comes from Vite, and is conditionally called based on `isTTY`. // We set it to false here to skip this odd behavior. process.stdout.isTTY = false; -const config = { +export default defineConfig({ testMatch: 'e2e/*.test.js', /* Maximum time one test can run for. */ timeout: 40 * 1000, @@ -37,6 +38,4 @@ const config = { }, }, ], -}; - -export default config; +}); diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 098762852e..41b62c2b19 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -67,6 +67,10 @@ export interface RenderErrorOptions { * Whether to skip middleware while rendering the error page. Defaults to false. */ skipMiddleware?: boolean; + /** + * Allows passing an error to 500.astro. It will be available through `Astro.props.error`. + */ + error?: unknown; } export class App { @@ -289,8 +293,9 @@ export class App { } if (locals) { if (typeof locals !== 'object') { - this.#logger.error(null, new AstroError(AstroErrorData.LocalsNotAnObject).stack!); - return this.#renderError(request, { status: 500 }); + const error = new AstroError(AstroErrorData.LocalsNotAnObject); + this.#logger.error(null, error.stack!); + return this.#renderError(request, { status: 500, error }); } Reflect.set(request, clientLocalsSymbol, locals); } @@ -324,7 +329,7 @@ export class App { response = await renderContext.render(await mod.page()); } catch (err: any) { this.#logger.error(null, err.stack || err.message || String(err)); - return this.#renderError(request, { locals, status: 500 }); + return this.#renderError(request, { locals, status: 500, error: err }); } if ( @@ -335,6 +340,9 @@ export class App { locals, response, status: response.status as 404 | 500, + // We don't have an error to report here. Passing null means we pass nothing intentionally + // while undefined means there's no error + error: response.status === 500 ? null : undefined, }); } @@ -385,7 +393,13 @@ export class App { */ async #renderError( request: Request, - { locals, status, response: originalResponse, skipMiddleware = false }: RenderErrorOptions + { + locals, + status, + response: originalResponse, + skipMiddleware = false, + error, + }: RenderErrorOptions ): Promise { const errorRoutePath = `/${status}${this.#manifest.trailingSlash === 'always' ? '/' : ''}`; const errorRouteData = matchRoute(errorRoutePath, this.#manifestData); @@ -415,6 +429,7 @@ export class App { request, routeData: errorRouteData, status, + props: { error } }); const response = await renderContext.render(await mod.page()); return this.#mergeResponses(response, originalResponse); diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index cf9c4bf01f..d46f809882 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -74,8 +74,9 @@ export class RenderContext { request, routeData, status = 200, + props, }: Pick & - Partial>): RenderContext { + Partial>): RenderContext { return new RenderContext( pipeline, locals, @@ -83,7 +84,11 @@ export class RenderContext { pathname, request, routeData, - status + status, + undefined, + undefined, + undefined, + props ); } diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 0d5f5d0d76..e2caf930c4 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -230,6 +230,7 @@ export async function handleRoute({ logger.error('router', err.stack || err.message); const filePath500 = new URL(`./${custom500.component}`, config.root); const preloaded500Component = await pipeline.preload(custom500, filePath500); + renderContext.props.error = err; response = await renderContext.render(preloaded500Component); status = 500; } diff --git a/packages/astro/test/custom-500.test.js b/packages/astro/test/custom-500.test.js new file mode 100644 index 0000000000..6d5c46dd45 --- /dev/null +++ b/packages/astro/test/custom-500.test.js @@ -0,0 +1,122 @@ +import assert from 'node:assert/strict'; +import { afterEach, describe, it } from 'node:test'; +import * as cheerio from 'cheerio'; +import { loadFixture } from './test-utils.js'; +import testAdapter from './test-adapter.js'; +import { renameSync } from 'node:fs'; + +describe('Custom 500', () => { + /** @type {Awaited>} */ + let fixture; + + describe('dev', () => { + /** @type {Awaited>} */ + let devServer; + + afterEach(async () => { + await devServer?.stop(); + try { + renameSync( + new URL('./fixtures/custom-500/src/pages/_500.astro', import.meta.url), + new URL('./fixtures/custom-500/src/pages/500.astro', import.meta.url) + ); + } catch (_) {} + }); + + it('renders default error overlay', async () => { + renameSync( + new URL('./fixtures/custom-500/src/pages/500.astro', import.meta.url), + new URL('./fixtures/custom-500/src/pages/_500.astro', import.meta.url) + ); + fixture = await loadFixture({ + root: './fixtures/custom-500/', + output: 'server', + adapter: testAdapter(), + }); + devServer = await fixture.startDevServer(); + + const response = await fixture.fetch('/'); + assert.equal(response.status, 500); + + const html = await response.text(); + + assert.equal(html, 'Error'); + }); + + it('renders custom 500', async () => { + fixture = await loadFixture({ + root: './fixtures/custom-500/', + output: 'server', + adapter: testAdapter(), + }); + devServer = await fixture.startDevServer(); + + const response = await fixture.fetch('/'); + assert.equal(response.status, 500); + + const html = await response.text(); + const $ = cheerio.load(html); + + assert.equal($('h1').text(), 'Server error'); + assert.equal($('p').text(), 'some error'); + }); + + it('renders default error overlay if custom 500 throws', async () => { + fixture = await loadFixture({ + root: './fixtures/custom-500-failing/', + output: 'server', + adapter: testAdapter(), + }); + devServer = await fixture.startDevServer(); + + const response = await fixture.fetch('/'); + assert.equal(response.status, 500); + + const html = await response.text(); + + assert.equal(html, 'Error'); + }); + }); + + describe('SSR', () => { + /** @type {Awaited>} */ + let app; + + it('renders custom 500', async () => { + fixture = await loadFixture({ + root: './fixtures/custom-500/', + output: 'server', + adapter: testAdapter(), + }); + await fixture.build(); + app = await fixture.loadTestAdapterApp(); + + const request = new Request('http://example.com/'); + const response = await app.render(request); + assert.equal(response.status, 500); + + const html = await response.text(); + const $ = cheerio.load(html); + + assert.equal($('h1').text(), 'Server error'); + assert.equal($('p').text(), 'some error'); + }); + + it('renders nothing if custom 500 throws', async () => { + fixture = await loadFixture({ + root: './fixtures/custom-500-failing/', + output: 'server', + adapter: testAdapter(), + }); + await fixture.build(); + app = await fixture.loadTestAdapterApp(); + + const request = new Request('http://example.com/'); + const response = await app.render(request); + assert.equal(response.status, 500); + + const html = await response.text(); + assert.equal(html, ''); + }); + }); +}); diff --git a/packages/astro/test/fixtures/custom-500-failing/astro.config.mjs b/packages/astro/test/fixtures/custom-500-failing/astro.config.mjs new file mode 100644 index 0000000000..882e6515a6 --- /dev/null +++ b/packages/astro/test/fixtures/custom-500-failing/astro.config.mjs @@ -0,0 +1,4 @@ +import { defineConfig } from 'astro/config'; + +// https://astro.build/config +export default defineConfig({}); diff --git a/packages/astro/test/fixtures/custom-500-failing/package.json b/packages/astro/test/fixtures/custom-500-failing/package.json new file mode 100644 index 0000000000..57691b18ea --- /dev/null +++ b/packages/astro/test/fixtures/custom-500-failing/package.json @@ -0,0 +1,8 @@ +{ + "name": "@test/custom-500-failing", + "version": "0.0.0", + "private": true, + "dependencies": { + "astro": "workspace:*" + } +} diff --git a/packages/astro/test/fixtures/custom-500-failing/src/pages/500.astro b/packages/astro/test/fixtures/custom-500-failing/src/pages/500.astro new file mode 100644 index 0000000000..c1ab36e630 --- /dev/null +++ b/packages/astro/test/fixtures/custom-500-failing/src/pages/500.astro @@ -0,0 +1,19 @@ +--- +interface Props { + error: unknown +} + +const { error } = Astro.props + +throw "custom 500 fail" +--- + + + + Server error - Custom 500 + + +

Server error

+

{error}

+ + diff --git a/packages/astro/test/fixtures/custom-500-failing/src/pages/index.astro b/packages/astro/test/fixtures/custom-500-failing/src/pages/index.astro new file mode 100644 index 0000000000..025b57ef26 --- /dev/null +++ b/packages/astro/test/fixtures/custom-500-failing/src/pages/index.astro @@ -0,0 +1,12 @@ +--- +throw "some error" +--- + + + + Custom 500 + + +

Home

+ + diff --git a/packages/astro/test/fixtures/custom-500/astro.config.mjs b/packages/astro/test/fixtures/custom-500/astro.config.mjs new file mode 100644 index 0000000000..882e6515a6 --- /dev/null +++ b/packages/astro/test/fixtures/custom-500/astro.config.mjs @@ -0,0 +1,4 @@ +import { defineConfig } from 'astro/config'; + +// https://astro.build/config +export default defineConfig({}); diff --git a/packages/astro/test/fixtures/custom-500/package.json b/packages/astro/test/fixtures/custom-500/package.json new file mode 100644 index 0000000000..7d45038cd2 --- /dev/null +++ b/packages/astro/test/fixtures/custom-500/package.json @@ -0,0 +1,8 @@ +{ + "name": "@test/custom-500", + "version": "0.0.0", + "private": true, + "dependencies": { + "astro": "workspace:*" + } +} diff --git a/packages/astro/test/fixtures/custom-500/src/pages/500.astro b/packages/astro/test/fixtures/custom-500/src/pages/500.astro new file mode 100644 index 0000000000..c25ff6ccdd --- /dev/null +++ b/packages/astro/test/fixtures/custom-500/src/pages/500.astro @@ -0,0 +1,17 @@ +--- +interface Props { + error: unknown +} + +const { error } = Astro.props +--- + + + + Server error - Custom 500 + + +

Server error

+

{error}

+ + diff --git a/packages/astro/test/fixtures/custom-500/src/pages/index.astro b/packages/astro/test/fixtures/custom-500/src/pages/index.astro new file mode 100644 index 0000000000..025b57ef26 --- /dev/null +++ b/packages/astro/test/fixtures/custom-500/src/pages/index.astro @@ -0,0 +1,12 @@ +--- +throw "some error" +--- + + + + Custom 500 + + +

Home

+ + diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index cb405d5aa0..b5a6463015 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2895,6 +2895,18 @@ importers: specifier: workspace:* version: link:../../.. + packages/astro/test/fixtures/custom-500: + dependencies: + astro: + specifier: workspace:* + version: link:../../.. + + packages/astro/test/fixtures/custom-500-failing: + dependencies: + astro: + specifier: workspace:* + version: link:../../.. + packages/astro/test/fixtures/custom-assets-name: dependencies: '@astrojs/node':