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

Refactor 404 and 500 approach #7754

Merged
merged 22 commits into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from all 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: 5 additions & 0 deletions .changeset/big-suns-wave.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Refactor `404` and `500` route handling for consistency and improved prerendering support
5 changes: 5 additions & 0 deletions .changeset/heavy-cooks-laugh.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/node': patch
---

Improve `404` behavior in middleware mode
6 changes: 6 additions & 0 deletions .changeset/rich-toys-jog.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@astrojs/netlify': patch
'@astrojs/vercel': patch
---

Improve `404` behavior for `serverless` and `edge`
146 changes: 80 additions & 66 deletions packages/astro/src/core/app/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand Down Expand Up @@ -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',
});
}
}

Reflect.set(request, clientLocalsSymbol, locals ?? {});

// Use the 404 status code for 404.astro components
if (routeData.route === '/404') {
defaultStatus = 404;
if (!routeData) {
return this.#renderError(request, { routeData, status: 404 });
}

let mod = await this.#getModuleForRoute(routeData);
Reflect.set(request, clientLocalsSymbol, locals ?? {});
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);
Expand All @@ -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 {
Expand All @@ -238,7 +196,6 @@ export class App {
status: 200,
headers,
});

attachToResponse(newResponse, response.cookies);
return newResponse;
}
Expand Down Expand Up @@ -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
Comment on lines +268 to +269
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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
* If it 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird; why do we return newRespnose.status if we check the original staus? Can you add a comment?

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;
Expand Down
8 changes: 0 additions & 8 deletions packages/astro/test/fixtures/ssr-prerender-404/package.json

This file was deleted.

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
---
Astro.response.status = 404;
Astro.response.statusText = 'Oops';
Astro.response.headers.set('One-Two', 'three');
---
<html>
<head>
Expand Down
30 changes: 0 additions & 30 deletions packages/astro/test/ssr-prerender-404.test.js

This file was deleted.

8 changes: 8 additions & 0 deletions packages/astro/test/ssr-response.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
28 changes: 11 additions & 17 deletions packages/integrations/netlify/src/netlify-edge-functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down
10 changes: 1 addition & 9 deletions packages/integrations/netlify/src/netlify-functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};
Expand Down
11 changes: 6 additions & 5 deletions packages/integrations/node/src/nodeMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,17 @@ 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,
next?: (err?: unknown) => void,
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);
Expand All @@ -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) {
Expand Down
Loading
Loading