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

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

Merged
merged 16 commits into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from 10 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
5 changes: 4 additions & 1 deletion packages/astro/src/container/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import {
createModuleScriptElement,
createStylesheetElementSet,
} from '../core/render/ssr-element.js';
import { DEFAULT_404_ROUTE, default404Page } from '../core/routing/astro-designed-error-pages.js';
import {
DEFAULT_404_ROUTE,
default404Page,
} from '../core/routing/astro-designed-error-pages.js';

export class ContainerPipeline extends Pipeline {
/**
Expand Down
5 changes: 5 additions & 0 deletions packages/astro/src/core/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 404 page, which is used when there is no user-provided 404.astro page.
ematipico marked this conversation as resolved.
Show resolved Hide resolved
*/
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.
Expand Down
16 changes: 15 additions & 1 deletion packages/astro/src/core/routing/astro-designed-error-pages.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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: '/404',
ematipico marked this conversation as resolved.
Show resolved Hide resolved
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);
Expand Down
5 changes: 4 additions & 1 deletion packages/astro/src/vite-plugin-astro-server/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ import { AggregateError, AstroError, CSSError, MarkdownError } from '../core/err
import type { Logger } from '../core/logger/core.js';
import type { ModuleLoader } from '../core/module-loader/index.js';
import { Pipeline, loadRenderer } from '../core/render/index.js';
import { DEFAULT_404_ROUTE, default404Page } from '../core/routing/astro-designed-error-pages.js';
import {
DEFAULT_404_ROUTE,
default404Page,
} from '../core/routing/astro-designed-error-pages.js';
import { isPage, isServerLikeOutput, resolveIdToUrl, viteID } from '../core/util.js';
import { PAGE_SCRIPT_ID } from '../vite-plugin-scripts/index.js';
import { getStylesForURL } from './css.js';
Expand Down
6 changes: 3 additions & 3 deletions packages/astro/src/vite-plugin-astro-server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ export default function createVitePluginAstroServer({
configureServer(viteServer) {
const loader = createViteLoader(viteServer);
const manifest = createDevelopmentManifest(settings);
let manifestData: ManifestData = ensure404Route(
createRouteManifest({ settings, fsMod }, logger)
);
let manifestData: ManifestData =
ensure404Route(createRouteManifest({ settings, fsMod }, logger))
;
const pipeline = DevPipeline.create(manifestData, { loader, logger, manifest, settings });
const controller = createController({ loader });
const localStorage = new AsyncLocalStorage();
Expand Down
22 changes: 21 additions & 1 deletion packages/astro/src/vite-plugin-astro-server/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -273,7 +278,21 @@ 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;
}
logger.error('router', err.stack || err.message);
ematipico marked this conversation as resolved.
Show resolved Hide resolved
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(
Expand Down Expand Up @@ -321,6 +340,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)) {
Expand Down
4 changes: 2 additions & 2 deletions packages/astro/test/core-image.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand All @@ -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);
});

Expand Down
2 changes: 2 additions & 0 deletions packages/astro/test/error-non-error.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ describe('Can handle errors that are not instanceof Error', () => {
let res = await fixture.fetch('/');
let html = await res.text();

console.log("html", html)

assert.equal(html.includes('Error'), true);
res = await fixture.fetch('/');
await res.text();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { defineConfig } from 'astro/config';

// https://astro.build/config
export default defineConfig({
experimental: {
rewriting: true
},
site: "https://example.com"
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "@test/rewrite-runtime-errror-custom500",
"version": "0.0.0",
"private": true,
"dependencies": {
"astro": "workspace:*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
---

<h1>I am the custom 500</h1>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
return Astro.rewrite("/errors/throw")
---
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
throw new Error("Fancy error")
---
81 changes: 77 additions & 4 deletions packages/astro/test/rewrite.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ describe('Middleware', () => {
});
});

describe('Runtime error', () => {
describe('Runtime error, default 500', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
let devServer;
Expand All @@ -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');
});
});
6 changes: 6 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading