michael/astro
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Refactor 404 and 500 approach (#7754)

Browse source 
* fix(app): refactor 404 and 500 approach

* chore: refactor logic

* fix: always treat error as page

* test: migrate ssr-prerender-404 to node adapter

* feat: merge original response metadata with error response

* chore: update lockfile

* chore: trigger ci

* chore(lint): fix lint issue

* fix: ensure merged request has proper status

* fix(node): prerender test

* chore: update test label

* fix(node): improve 404 behavior in middleware mode

* fix(vercel): improve 404 behavior

* fix(netlify): improve 404 behavior

* chore: update test labels

* chore: force ci

* chore: fix lint

* fix: avoid infinite loops

* test: fix failing test in Node 18

* chore: remove volta
This commit is contained in:
Nate Moore 2023-08-01 09:52:16 -05:00 committed by GitHub
parent 0b8375fe82
commit 298dbb89f2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
22 changed files with 384 additions and 196 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
.changeset/big-suns-wave.md Normal file
View file

@ -0,0 +1,5 @@
---
'astro': patch
---
Refactor `404` and `500` route handling for consistency and improved prerendering support

5
.changeset/heavy-cooks-laugh.md Normal file
View file

@ -0,0 +1,5 @@
---
'@astrojs/node': patch
---
Improve `404` behavior in middleware mode

6
.changeset/rich-toys-jog.md Normal file
View file

@ -0,0 +1,6 @@
---
'@astrojs/netlify': patch
'@astrojs/vercel': patch
---
Improve `404` behavior for `serverless` and `edge`

146
packages/astro/src/core/app/index.ts
View file

@ -34,9 +34,16 @@ const clientLocalsSymbol = Symbol.for('astro.locals');
const responseSentSymbol = Symbol.for('astro.responseSent');
const STATUS_CODES = new Set([404, 500]);
export interface MatchOptions {
matchNotFound?: boolean | undefined;
}
export interface RenderErrorOptions {
routeData?: RouteData;
response?: Response;
status: 404 | 500;
}
export class App {
/**
@ -113,50 +120,29 @@ export class App {
}
return pathname;
}
match(request: Request, { matchNotFound = false }: MatchOptions = {}): RouteData | undefined {
// Disable no-unused-vars to avoid breaking signature change
// eslint-disable-next-line @typescript-eslint/no-unused-vars
match(request: Request, _opts: MatchOptions = {}): RouteData | undefined {
const url = new URL(request.url);
// ignore requests matching public assets
if (this.#manifest.assets.has(url.pathname)) {
return undefined;
}
if (this.#manifest.assets.has(url.pathname)) return undefined;
let pathname = prependForwardSlash(this.removeBase(url.pathname));
let routeData = matchRoute(pathname, this.#manifestData);
if (routeData) {
if (routeData.prerender) return undefined;
return routeData;
} else if (matchNotFound) {
const notFoundRouteData = matchRoute('/404', this.#manifestData);
if (notFoundRouteData?.prerender) return undefined;
return notFoundRouteData;
} else {
return undefined;
}
// missing routes fall-through, prerendered are handled by static layer
if (!routeData || routeData.prerender) return undefined;
return routeData;
}
async render(request: Request, routeData?: RouteData, locals?: object): Promise<Response> {
let defaultStatus = 200;
if (!routeData) {
routeData = this.match(request);
if (!routeData) {
defaultStatus = 404;
routeData = this.match(request, { matchNotFound: true });
}
if (!routeData) {
return new Response(null, {
status: 404,
statusText: 'Not found',
});
}
}
if (!routeData) {
return this.#renderError(request, { routeData, status: 404 });
}
Reflect.set(request, clientLocalsSymbol, locals ?? {});
// Use the 404 status code for 404.astro components
if (routeData.route === '/404') {
defaultStatus = 404;
}
let mod = await this.#getModuleForRoute(routeData);
const defaultStatus = this.#getDefaultStatusCode(routeData.route);
const mod = await this.#getModuleForRoute(routeData);
const pageModule = (await mod.page()) as any;
const url = new URL(request.url);
@ -179,47 +165,19 @@ export class App {
);
} catch (err: any) {
error(this.#logging, 'ssr', err.stack || err.message || String(err));
response = new Response(null, {
status: 500,
statusText: 'Internal server error',
});
return this.#renderError(request, { routeData, status: 500 });
}
if (isResponse(response, routeData.type)) {
// If there was a known error code, try sending the according page (e.g. 404.astro / 500.astro).
if (response.status === 500 || response.status === 404) {
const errorRouteData = matchRoute('/' + response.status, this.#manifestData);
if (errorRouteData && errorRouteData.route !== routeData.route) {
mod = await this.#getModuleForRoute(errorRouteData);
try {
const newRenderContext = await this.#createRenderContext(
url,
request,
routeData,
mod,
response.status
);
const page = (await mod.page()) as any;
const errorResponse = await tryRenderRoute(
routeData.type,
newRenderContext,
this.#env,
page
);
return errorResponse as Response;
} catch {}
}
if (STATUS_CODES.has(response.status)) {
return this.#renderError(request, { routeData, response, status: response.status as 404 | 500 } );
}
Reflect.set(response, responseSentSymbol, true);
return response;
} else {
if (response.type === 'response') {
if (response.response.headers.get('X-Astro-Response') === 'Not-Found') {
const fourOhFourRequest = new Request(new URL('/404', request.url));
const fourOhFourRouteData = this.match(fourOhFourRequest);
if (fourOhFourRouteData) {
return this.render(fourOhFourRequest, fourOhFourRouteData);
}
return this.#renderError(request, { routeData, response: response.response, status: 404 });
}
return response.response;
} else {
@ -238,7 +196,6 @@ export class App {
status: 200,
headers,
});
attachToResponse(newResponse, response.cookies);
return newResponse;
}
@ -307,6 +264,63 @@ export class App {
}
}
/**
* If is a known error code, try sending the according page (e.g. 404.astro / 500.astro).
* This also handles pre-rendered /404 or /500 routes
*/
async #renderError(request: Request, { routeData, status, response: originalResponse }: RenderErrorOptions) {
const errorRouteData = matchRoute('/' + status, this.#manifestData);
const url = new URL(request.url);
if (errorRouteData) {
if (errorRouteData.prerender && !errorRouteData.route.endsWith(`/${status}`)) {
const statusURL = new URL(`${this.#baseWithoutTrailingSlash}/${status}`, url);
const response = await fetch(statusURL.toString());
return this.#mergeResponses(response, originalResponse);
}
const finalRouteData = routeData ?? errorRouteData;
const mod = await this.#getModuleForRoute(errorRouteData);
try {
const newRenderContext = await this.#createRenderContext(
url,
request,
finalRouteData,
mod,
status
);
const page = (await mod.page()) as any;
const response = await tryRenderRoute(
'page', // this is hardcoded to ensure proper behavior for missing endpoints
newRenderContext,
this.#env,
page
) as Response;
return this.#mergeResponses(response, originalResponse);
} catch {}
}
const response = this.#mergeResponses(new Response(null, { status }), originalResponse);
Reflect.set(response, responseSentSymbol, true);
return response;
}
#mergeResponses(newResponse: Response, oldResponse?: Response) {
if (!oldResponse) return newResponse;
const { status, statusText, headers } = oldResponse;
return new Response(newResponse.body, {
status: status === 200 ? newResponse.status : status,
statusText,
headers: new Headers(Array.from(headers))
})
}
#getDefaultStatusCode(route: string): number {
route = removeTrailingForwardSlash(route)
if (route.endsWith('/404')) return 404;
if (route.endsWith('/500')) return 500;
return 200;
}
async #getModuleForRoute(route: RouteData): Promise<SinglePageBuiltModule> {
if (route.type === 'redirect') {
return RedirectSinglePageBuiltModule;

8
packages/astro/test/fixtures/ssr-prerender-404/package.json vendored
View file

@ -1,8 +0,0 @@
{
"name": "@test/ssr-prerender-404",
"version": "0.0.0",
"private": true,
"dependencies": {
"astro": "workspace:*"
}
}

5
packages/astro/test/fixtures/ssr-prerender-404/src/pages/404.astro vendored
View file

@ -1,5 +0,0 @@
---
export const prerender = true
---
Page does not exist

18
packages/astro/test/fixtures/ssr-prerender-404/src/pages/static.astro vendored
View file

@ -1,18 +0,0 @@
---
export const prerender = true;
const { searchParams } = Astro.url;
---
<html>
<head>
<title>Static Page</title>
<script>
console.log('hello world');
</script>
</head>
<body>
<h1 id="greeting">Hello world!</h1>
<div id="searchparams">{searchParams.get('q')}</div>
</body>
</html>

1
packages/astro/test/fixtures/ssr-response/src/pages/status-code.astro vendored
View file

@ -1,6 +1,7 @@
---
Astro.response.status = 404;
Astro.response.statusText = 'Oops';
Astro.response.headers.set('One-Two', 'three');
---
<html>
<head>

30
packages/astro/test/ssr-prerender-404.test.js
View file

@ -1,30 +0,0 @@
import { expect } from 'chai';
import { loadFixture } from './test-utils.js';
import testAdapter from './test-adapter.js';
describe('SSR: prerender 404', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
before(async () => {
fixture = await loadFixture({
root: './fixtures/ssr-prerender-404/',
output: 'server',
adapter: testAdapter(),
});
await fixture.build();
});
describe('Prerendering', () => {
it('Prerendered 404.astro page is not rendered', async () => {
const app = await fixture.loadTestAdapterApp();
const request = new Request('http://example.com/non-existent-page');
const response = await app.render(request);
expect(response.status).to.equal(404);
expect(response.statusText).to.equal(
'Not found',
'should be actual 404 response, not 404 page'
);
});
});
});

8
packages/astro/test/ssr-response.test.js
View file

@ -29,6 +29,14 @@ describe('Using Astro.response in SSR', () => {
expect(response.statusText).to.equal('Oops');
});
it('Can set headers for 404 page', async () => {
const app = await fixture.loadTestAdapterApp();
const request = new Request('http://example.com/status-code');
const response = await app.render(request);
const headers = response.headers;
expect(headers.get('one-two')).to.equal('three');
});
it('Can add headers', async () => {
const app = await fixture.loadTestAdapterApp();
const request = new Request('http://example.com/some-header');

28
packages/integrations/netlify/src/netlify-edge-functions.ts
View file

@ -15,25 +15,19 @@ export function createExports(manifest: SSRManifest) {
if (manifest.assets.has(url.pathname)) {
return;
}
if (app.match(request)) {
const ip =
request.headers.get('x-nf-client-connection-ip') ||
context?.ip ||
(context as any)?.remoteAddr?.hostname;
Reflect.set(request, clientAddressSymbol, ip);
const response = await app.render(request);
if (app.setCookieHeaders) {
for (const setCookieHeader of app.setCookieHeaders(response)) {
response.headers.append('Set-Cookie', setCookieHeader);
}
const routeData = app.match(request)
const ip =
request.headers.get('x-nf-client-connection-ip') ||
context?.ip ||
(context as any)?.remoteAddr?.hostname;
Reflect.set(request, clientAddressSymbol, ip);
const response = await app.render(request, routeData);
if (app.setCookieHeaders) {
for (const setCookieHeader of app.setCookieHeaders(response)) {
response.headers.append('Set-Cookie', setCookieHeader);
}
return response;
}
return new Response(null, {
status: 404,
statusText: 'Not found',
});
return response;
};
return { default: handler };

10
packages/integrations/netlify/src/netlify-functions.ts
View file

@ -70,15 +70,7 @@ export const createExports = (manifest: SSRManifest, args: Args) => {
}
const request = new Request(rawUrl, init);
let routeData = app.match(request, { matchNotFound: true });
if (!routeData) {
return {
statusCode: 404,
body: 'Not found',
};
}
const routeData = app.match(request);
const ip = headers['x-nf-client-connection-ip'];
Reflect.set(request, clientAddressSymbol, ip);
let locals = {};

11
packages/integrations/node/src/nodeMiddleware.ts
View file

@ -5,7 +5,9 @@ import { createOutgoingHttpHeaders } from './createOutgoingHttpHeaders';
import { responseIterator } from './response-iterator';
import type { Options } from './types';
export default function (app: NodeApp, mode: Options['mode']) {
// Disable no-unused-vars to avoid breaking signature change
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export default function (app: NodeApp, _mode: Options['mode']) {
return async function (
req: IncomingMessage,
res: ServerResponse,
@ -13,8 +15,7 @@ export default function (app: NodeApp, mode: Options['mode']) {
locals?: object
) {
try {
const route =
mode === 'standalone' ? app.match(req, { matchNotFound: true }) : app.match(req);
const route = app.match(req);
if (route) {
try {
const response = await app.render(req, route, locals);
@ -29,8 +30,8 @@ export default function (app: NodeApp, mode: Options['mode']) {
} else if (next) {
return next();
} else {
res.writeHead(404);
res.end('Not found');
const response = await app.render(req);
await writeWebResponse(app, res, response);
}
} catch (err: unknown) {
if (!res.headersSent) {

2
packages/integrations/node/test/fixtures/node-middleware/src/pages/404.astro vendored
View file

@ -9,5 +9,5 @@
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>404</title>
</head>
<body><h1>404!!!!!!!!!!</h1></body>
<body>Page does not exist</body>
</html>

9
packages/integrations/node/test/fixtures/prerender-404/package.json vendored Normal file
View file

@ -0,0 +1,9 @@
{
"name": "@test/nodejs-prerender-404",
"version": "0.0.0",
"private": true,
"dependencies": {
"astro": "workspace:*",
"@astrojs/node": "workspace:*"
}
}

5
packages/integrations/node/test/fixtures/prerender-404/src/pages/404.astro vendored Normal file
View file

@ -0,0 +1,5 @@
---
export const prerender = true;
---
Page does not exist

12
packages/integrations/node/test/fixtures/prerender-404/src/pages/static.astro vendored Normal file
View file

@ -0,0 +1,12 @@
---
export const prerender = true;
---
<html>
<head>
<title>Static Page</title>
</head>
<body>
<h1>Hello world!</h1>
</body>
</html>

40
packages/integrations/node/test/node-middleware.test.js
View file

@ -3,35 +3,51 @@ import { loadFixture } from './test-utils.js';
import { expect } from 'chai';
import * as cheerio from 'cheerio';
describe('test 404 cant load', () => {
/**
* @typedef {import('../../../astro/test/test-utils').Fixture} Fixture
*/
async function load() {
const mod = await import(`./fixtures/node-middleware/dist/server/entry.mjs?dropcache=${Date.now()}`);
return mod;
}
describe('behavior from middleware', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
let server;
before(async () => {
process.env.ASTRO_NODE_AUTOSTART = 'disabled';
process.env.PRERENDER = false;
fixture = await loadFixture({
root: './fixtures/node-middleware/',
output: 'server',
adapter: nodejs({ mode: 'standalone' }),
});
await fixture.build();
const { startServer } = await load();
let res = startServer();
server = res.server;
});
describe('test 404', async () => {
let devPreview;
before(async () => {
devPreview = await fixture.preview();
});
after(async () => {
await devPreview.stop();
});
after(async () => {
await server.stop();
await fixture.clean();
delete process.env.PRERENDER;
})
describe('404', async () => {
it('when mode is standalone', async () => {
const res = await fixture.fetch('/error-page');
const res = await fetch(`http://${server.host}:${server.port}/error-page`);
expect(res.status).to.equal(404);
const html = await res.text();
const $ = cheerio.load(html);
const h1 = $('h1');
expect(h1.text()).to.equal('404!!!!!!!!!!');
const body = $('body');
expect(body.text()).to.equal('Page does not exist');
});
});
});

189
packages/integrations/node/test/prerender-404.test.js Normal file
View file

@ -0,0 +1,189 @@
import nodejs from '../dist/index.js';
import { loadFixture } from './test-utils.js';
import { expect } from 'chai';
import * as cheerio from 'cheerio';
import { fetch } from 'undici';
/**
* @typedef {import('../../../astro/test/test-utils').Fixture} Fixture
*/
async function load() {
const mod = await import(`./fixtures/prerender-404/dist/server/entry.mjs?dropcache=${Date.now()}`);
return mod;
}
describe('Prerender 404', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
let server;
describe('With base', async () => {
before(async () => {
process.env.ASTRO_NODE_AUTOSTART = 'disabled';
process.env.PRERENDER = true;
fixture = await loadFixture({
base: '/some-base',
root: './fixtures/prerender-404/',
output: 'server',
adapter: nodejs({ mode: 'standalone' }),
});
await fixture.build();
const { startServer } = await load();
let res = startServer();
server = res.server;
});
after(async () => {
await server.stop();
await fixture.clean();
delete process.env.PRERENDER;
});
it('Can render SSR route', async () => {
const res = await fetch(`http://${server.host}:${server.port}/some-base/static`);
const html = await res.text();
const $ = cheerio.load(html);
expect(res.status).to.equal(200);
expect($('h1').text()).to.equal('Hello world!');
});
it('Can handle prerendered 404', async () => {
const res = await fetch(`http://${server.host}:${server.port}/some-base/missing`);
const html = await res.text();
const $ = cheerio.load(html);
expect(res.status).to.equal(404);
expect($('body').text()).to.equal('Page does not exist');
});
});
describe('Without base', async () => {
before(async () => {
process.env.ASTRO_NODE_AUTOSTART = 'disabled';
process.env.PRERENDER = true;
fixture = await loadFixture({
root: './fixtures/prerender-404/',
output: 'server',
adapter: nodejs({ mode: 'standalone' }),
});
await fixture.build();
const { startServer } = await await load();
let res = startServer();
server = res.server;
});
after(async () => {
await server.stop();
await fixture.clean();
delete process.env.PRERENDER;
});
it('Can render SSR route', async () => {
const res = await fetch(`http://${server.host}:${server.port}/static`);
const html = await res.text();
const $ = cheerio.load(html);
expect(res.status).to.equal(200);
expect($('h1').text()).to.equal('Hello world!');
});
it('Can handle prerendered 404', async () => {
const res = await fetch(`http://${server.host}:${server.port}/missing`);
const html = await res.text();
const $ = cheerio.load(html);
expect(res.status).to.equal(404);
expect($('body').text()).to.equal('Page does not exist');
});
});
});
describe('Hybrid 404', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
let server;
describe('With base', async () => {
before(async () => {
process.env.ASTRO_NODE_AUTOSTART = 'disabled';
process.env.PRERENDER = false;
fixture = await loadFixture({
base: '/some-base',
root: './fixtures/prerender-404/',
output: 'hybrid',
adapter: nodejs({ mode: 'standalone' }),
});
await fixture.build();
const { startServer } = await await load();
let res = startServer();
server = res.server;
});
after(async () => {
await server.stop();
await fixture.clean();
delete process.env.PRERENDER;
});
it('Can render SSR route', async () => {
const res = await fetch(`http://${server.host}:${server.port}/some-base/static`);
const html = await res.text();
const $ = cheerio.load(html);
expect(res.status).to.equal(200);
expect($('h1').text()).to.equal('Hello world!');
});
it('Can handle prerendered 404', async () => {
const res = await fetch(`http://${server.host}:${server.port}/some-base/missing`);
const html = await res.text();
const $ = cheerio.load(html);
expect(res.status).to.equal(404);
expect($('body').text()).to.equal('Page does not exist');
});
});
describe('Without base', async () => {
before(async () => {
process.env.ASTRO_NODE_AUTOSTART = 'disabled';
process.env.PRERENDER = false;
fixture = await loadFixture({
root: './fixtures/prerender-404/',
output: 'hybrid',
adapter: nodejs({ mode: 'standalone' }),
});
await fixture.build();
const { startServer } = await await load();
let res = startServer();
server = res.server;
});
after(async () => {
await server.stop();
await fixture.clean();
delete process.env.PRERENDER;
});
it('Can render SSR route', async () => {
const res = await fetch(`http://${server.host}:${server.port}/static`);
const html = await res.text();
const $ = cheerio.load(html);
expect(res.status).to.equal(200);
expect($('h1').text()).to.equal('Hello world!');
});
it('Can handle prerendered 404', async () => {
const res = await fetch(`http://${server.host}:${server.port}/missing`);
const html = await res.text();
const $ = cheerio.load(html);
expect(res.status).to.equal(404);
expect($('body').text()).to.equal('Page does not exist');
});
});
});

20
packages/integrations/vercel/src/edge/entrypoint.ts
View file

@ -13,21 +13,15 @@ export function createExports(manifest: SSRManifest) {
const app = new App(manifest);
const handler = async (request: Request): Promise<Response> => {
if (app.match(request)) {
Reflect.set(request, clientAddressSymbol, request.headers.get('x-forwarded-for'));
const response = await app.render(request);
if (app.setCookieHeaders) {
for (const setCookieHeader of app.setCookieHeaders(response)) {
response.headers.append('Set-Cookie', setCookieHeader);
}
const routeData = app.match(request);
Reflect.set(request, clientAddressSymbol, request.headers.get('x-forwarded-for'));
const response = await app.render(request, routeData);
if (app.setCookieHeaders) {
for (const setCookieHeader of app.setCookieHeaders(response)) {
response.headers.append('Set-Cookie', setCookieHeader);
}
return response;
}
return new Response(null, {
status: 404,
statusText: 'Not found',
});
return response;
};
return { default: handler };

7
packages/integrations/vercel/src/serverless/entrypoint.ts
View file

@ -23,12 +23,7 @@ export const createExports = (manifest: SSRManifest) => {
return res.end(err.reason || 'Invalid request body');
}
let routeData = app.match(request, { matchNotFound: true });
if (!routeData) {
res.statusCode = 404;
return res.end('Not found');
}
let routeData = app.match(request);
let locals = {};
if (request.headers.has(ASTRO_LOCALS_HEADER)) {
let localsAsString = request.headers.get(ASTRO_LOCALS_HEADER);

15
pnpm-lock.yaml
View file

@ -3316,12 +3316,6 @@ importers:
specifier: workspace:*
version: link:../../..
packages/astro/test/fixtures/ssr-prerender-404:
dependencies:
astro:
specifier: workspace:*
version: link:../../..
packages/astro/test/fixtures/ssr-prerender-get-static-paths:
dependencies:
astro:
@ -4656,6 +4650,15 @@ importers:
specifier: workspace:*
version: link:../../../../../astro
packages/integrations/node/test/fixtures/prerender-404:
dependencies:
'@astrojs/node':
specifier: workspace:*
version: link:../../..
astro:
specifier: workspace:*
version: link:../../../../../astro
packages/integrations/node/test/fixtures/url-protocol:
dependencies:
'@astrojs/node':