Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 500.astro improvements #11134

Merged
merged 19 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions .changeset/rich-dolls-compete.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
---
"astro": minor
---

Improves DX around `500.astro`

The special `src/pages/500.astro` page now accepts an error as a prop. It can be anything so make sure to handle it properly:
florian-lefebvre marked this conversation as resolved.
Show resolved Hide resolved

```astro
---
// src/pages/500.astro
interface Props {
error: unknown
}

const { error } = Astro.props
---

<div>{error instanceof Error ? error.message : 'Unknown error'}</div>
```

Additionally, you can now use your custom 500 error page in development by setting the `ASTRO_CUSTOM_500` environment variable to `'true'`. For example using a `.env` file:

```ini
ASTRO_CUSTOM_500=true
```

Or inline:

```sh
ASTRO_CUSTOM_500=true astro dev
```

Note that if an error occur in this file, it will default to the host 500 error page in SSR, and the error overlay in development.
florian-lefebvre marked this conversation as resolved.
Show resolved Hide resolved
7 changes: 3 additions & 4 deletions packages/astro/playwright.config.js
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -37,6 +38,4 @@ const config = {
},
},
],
};

export default config;
});
19 changes: 14 additions & 5 deletions packages/astro/src/core/app/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export interface RenderErrorOptions {
* Whether to skip middleware while rendering the error page. Defaults to false.
*/
skipMiddleware?: boolean;
error?: unknown;
florian-lefebvre marked this conversation as resolved.
Show resolved Hide resolved
}

export class App {
Expand Down Expand Up @@ -290,8 +291,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);
}
Expand Down Expand Up @@ -329,7 +331,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 (
Expand All @@ -340,6 +342,7 @@ export class App {
locals,
response,
status: response.status as 404 | 500,
error: response.status === 500 ? null : undefined,
florian-lefebvre marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down Expand Up @@ -390,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<Response> {
const errorRoutePath = `/${status}${this.#manifest.trailingSlash === 'always' ? '/' : ''}`;
const errorRouteData = matchRoute(errorRoutePath, this.#manifestData);
Expand Down Expand Up @@ -421,7 +430,7 @@ export class App {
routeData: errorRouteData,
status,
});
const response = await renderContext.render(await mod.page());
const response = await renderContext.render(await mod.page(), undefined, { error });
return this.#mergeResponses(response, originalResponse);
} catch {
// Middleware may be the cause of the error, so we try rendering 404/500.astro without it.
Expand Down
22 changes: 13 additions & 9 deletions packages/astro/src/core/render-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,18 +93,22 @@ export class RenderContext {
*/
async render(
componentInstance: ComponentInstance | undefined,
slots: Record<string, any> = {}
slots: Record<string, any> = {},
props: Record<string, any> = {}
): Promise<Response> {
const { cookies, middleware, pathname, pipeline } = this;
const { logger, routeCache, serverLike, streaming } = pipeline;
const props = await getProps({
mod: componentInstance,
routeData: this.routeData,
routeCache,
pathname,
logger,
serverLike,
});
Object.assign(
props,
await getProps({
mod: componentInstance,
routeData: this.routeData,
routeCache,
pathname,
logger,
serverLike,
})
);
florian-lefebvre marked this conversation as resolved.
Show resolved Hide resolved
const apiContext = this.createAPIContext(props);

this.counter++;
Expand Down
27 changes: 25 additions & 2 deletions packages/astro/src/vite-plugin-astro-server/route.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type http from 'node:http';
import type { ComponentInstance, ManifestData, RouteData } from '../@types/astro.js';
import type { AstroConfig, ComponentInstance, ManifestData, RouteData } from '../@types/astro.js';
import {
DEFAULT_404_COMPONENT,
REROUTE_DIRECTIVE_HEADER,
Expand All @@ -16,6 +16,8 @@ import { normalizeTheLocale } from '../i18n/index.js';
import { getSortedPreloadedMatches } from '../prerender/routing.js';
import type { DevPipeline } from './pipeline.js';
import { default404Page, handle404Response, writeSSRResult, writeWebResponse } from './response.js';
import { loadEnv } from 'vite';
import { fileURLToPath } from 'node:url';

type AsyncReturnType<T extends (...args: any) => Promise<any>> = T extends (
...args: any
Expand Down Expand Up @@ -142,6 +144,15 @@ type HandleRoute = {
pipeline: DevPipeline;
};

function getCustom500Route(manifestData: ManifestData): RouteData | undefined {
const route500 = /^\/500\/?$/;
return manifestData.routes.find((r) => route500.test(r.route));
}

function shouldRenderCustom500(config: AstroConfig): boolean {
return loadEnv('development', fileURLToPath(config.root), '').ASTRO_CUSTOM_500 === 'true';
}

export async function handleRoute({
matchedRoute,
url,
Expand Down Expand Up @@ -272,7 +283,19 @@ export async function handleRoute({
});
}

let response = await renderContext.render(mod);
let response: Response;
try {
response = await renderContext.render(mod);
} catch (err) {
const custom500 = getCustom500Route(manifestData);
if (!shouldRenderCustom500(config) || !custom500) {
throw err;
}
const filePath = new URL(`./${custom500.component}`, config.root);
const preloadedComponent = await pipeline.preload(custom500, filePath);
response = await renderContext.render(preloadedComponent, undefined, { error: err });
status = 500;
}
if (isLoggedRequest(pathname)) {
const timeEnd = performance.now();
logger.info(
Expand Down
69 changes: 69 additions & 0 deletions packages/astro/test/custom-500-failing.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import assert from 'node:assert/strict';
import { afterEach, before, beforeEach, describe, it } from 'node:test';
import { loadFixture } from './test-utils.js';
import testAdapter from './test-adapter.js';

describe('Custom 500', () => {
/** @type {Awaited<ReturnType<typeof loadFixture>>} */
let fixture;

before(async () => {
fixture = await loadFixture({
root: './fixtures/custom-500-failing/',
output: 'server',
adapter: testAdapter(),
});
});

describe('dev', () => {
let devServer;

beforeEach(async () => {
devServer = await fixture.startDevServer();
});

afterEach(async () => {
await devServer.stop();
delete process.env.ASTRO_CUSTOM_500;
});

it('renders default error overlay', async () => {
const response = await fixture.fetch('/');
assert.equal(response.status, 500);

const html = await response.text();

assert.equal(html, '<title>Error</title><script type="module" src="/@vite/client"></script>');
});

it('renders default error overlay if custom 500 throws', async () => {
process.env.ASTRO_CUSTOM_500 = 'true';

const response = await fixture.fetch('/');
assert.equal(response.status, 500);

const html = await response.text();

assert.equal(html, '<title>Error</title><script type="module" src="/@vite/client"></script>');
});
});

describe('preview', () => {
/** @type {Awaited<ReturnType<(typeof fixture)["loadTestAdapterApp"]>>} */
let app;

before(async () => {
await fixture.build();
app = await fixture.loadTestAdapterApp();
});

it('renders nothing if custom 500 throws', async () => {
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, '');
});
});
});
75 changes: 75 additions & 0 deletions packages/astro/test/custom-500.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import assert from 'node:assert/strict';
import { afterEach, before, beforeEach, describe, it } from 'node:test';
import * as cheerio from 'cheerio';
import { loadFixture } from './test-utils.js';
import testAdapter from './test-adapter.js';

describe('Custom 500', () => {
/** @type {Awaited<ReturnType<typeof loadFixture>>} */
let fixture;

before(async () => {
fixture = await loadFixture({
root: './fixtures/custom-500/',
output: 'server',
adapter: testAdapter(),
});
});

describe('dev', () => {
let devServer;

beforeEach(async () => {
devServer = await fixture.startDevServer();
});

afterEach(async () => {
await devServer.stop();
delete process.env.ASTRO_CUSTOM_500;
});

it('renders default error overlay', async () => {
const response = await fixture.fetch('/');
assert.equal(response.status, 500);

const html = await response.text();

assert.equal(html, '<title>Error</title><script type="module" src="/@vite/client"></script>');
});

it('renders custom 500', async () => {
process.env.ASTRO_CUSTOM_500 = 'true';

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');
});
});

describe('preview', () => {
florian-lefebvre marked this conversation as resolved.
Show resolved Hide resolved
/** @type {Awaited<ReturnType<(typeof fixture)["loadTestAdapterApp"]>>} */
let app;

before(async () => {
await fixture.build();
app = await fixture.loadTestAdapterApp();
});

it('renders custom 500', async () => {
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');
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { defineConfig } from 'astro/config';

// https://astro.build/config
export default defineConfig({});
8 changes: 8 additions & 0 deletions packages/astro/test/fixtures/custom-500-failing/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "@test/custom-500-failing",
"version": "0.0.0",
"private": true,
"dependencies": {
"astro": "workspace:*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
interface Props {
error: unknown
}

const { error } = Astro.props

throw "custom 500 fail"
---

<html lang="en">
<head>
<title>Server error - Custom 500</title>
</head>
<body>
<h1>Server error</h1>
<p>{error}</p>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
throw "some error"
---

<html lang="en">
<head>
<title>Custom 500</title>
</head>
<body>
<h1>Home</h1>
</body>
</html>
4 changes: 4 additions & 0 deletions packages/astro/test/fixtures/custom-500/astro.config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { defineConfig } from 'astro/config';

// https://astro.build/config
export default defineConfig({});
Loading
Loading