Merge pull request #300 from sveltejs/gh-293

[WIP] simplify rendering of error pages

src/core/create_manifests.ts

@@ -52,8 +52,8 @@ function generate_client(routes: Route[], path_to_routes: string, dev_port?: num
 
 					const file = posixify(`${path_to_routes}/${page.file}`);
 
-					if (route.id === '_4xx' || route.id === '_5xx') {
+					if (route.id === '_error') {
-						return `{ error: '${route.id.slice(1)}', load: () => import(/* webpackChunkName: "${route.id}" */ '${file}') }`;
+						return `{ error: true, load: () => import(/* webpackChunkName: "${route.id}" */ '${file}') }`;
 					}
 
 					const params = route.params.length === 0
@@ -107,8 +107,8 @@ function generate_server(routes: Route[], path_to_routes: string) {
 							`{ type: '${type}', module: ${route.id}${index} }`)
 						.join(', ');
 
-					if (route.id === '_4xx' || route.id === '_5xx') {
+					if (route.id === '_error') {
-						return `{ error: '${route.id.slice(1)}', handlers: [${handlers}] }`;
+						return `{ error: true, handlers: [${handlers}] }`;
 					}
 
 					const params = route.params.length === 0

src/core/create_routes.ts

@@ -5,10 +5,8 @@ import { Route } from '../interfaces';
 
 export default function create_routes({ files } = { files: glob.sync('**/*.*', { cwd: locations.routes(), dot: true, nodir: true }) }) {
 	const routes: Route[] = files
-		.filter((file: string) => !/(^|\/|\\)_/.test(file))
+		.filter((file: string) => !/(^|\/|\\)(_(?!error\.html)|\.(?!well-known))/.test(file))
 		.map((file: string) => {
-			if (/(^|\/|\\)(_|\.(?!well-known))/.test(file)) return;
-
 			if (/]\[/.test(file)) {
 				throw new Error(`Invalid route ${file} — parameters must be separated`);
 			}
@@ -30,8 +28,8 @@ export default function create_routes({ files } = { files: glob.sync('**/*.*', {
 			return !found;
 		})
 		.sort((a, b) => {
-			if (a.parts[0] === '4xx' || a.parts[0] === '5xx') return -1;
+			if (a.parts[0] === '_error') return -1;
-			if (b.parts[0] === '4xx' || b.parts[0] === '5xx') return 1;
+			if (b.parts[0] === '_error') return 1;
 
 			const max = Math.max(a.parts.length, b.parts.length);
 

src/middleware.ts

@@ -162,8 +162,12 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter:
 		? () => fs.readFileSync(`${locations.app()}/template.html`, 'utf-8')
 		: (str => () => str)(fs.readFileSync(`${locations.dest()}/template.html`, 'utf-8'));
 
-	function handle_route(route: RouteObject, req: Req, res: ServerResponse) {
+	const error_route = routes.find((route: RouteObject) => route.error);
-		req.params = route.params(route.pattern.exec(req.path));
+
+	function handle_route(route: RouteObject, req: Req, res: ServerResponse, status = 200, error: Error | string = null) {
+		req.params = error
+			? {}
+			: route.params(route.pattern.exec(req.path));
 
 		const handlers = route.handlers[Symbol.iterator]();
 
@@ -174,7 +178,14 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter:
 				const { value: handler, done } = handlers.next();
 
 				if (done) {
-					handle_error(req, res, 404, 'Not found');
+					if (route.error) {
+						// there was an error rendering the error page!
+						res.statusCode = status;
+						res.end(error instanceof Error ? error.message : error);
+					} else {
+						handle_route(error_route, req, res, 404, 'Not found');
+					}
+
 					return;
 				}
 
@@ -186,7 +197,7 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter:
 					// preload main.js and current route
 					// TODO detect other stuff we can preload? images, CSS, fonts?
 					const link = []
-						.concat(chunks.main, chunks[route.id])
+						.concat(chunks.main, chunks[route.id] || chunks._error) // TODO this is gross
 						.filter(file => !file.match(/\.map$/))
 						.map(file => `<${req.baseUrl}/client/${file}>;rel="preload";as="script"`)
 						.join(', ');
@@ -196,8 +207,13 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter:
 					const store = store_getter ? store_getter(req) : null;
 					const props = { params: req.params, query: req.query, path: req.path };
 
+					if (route.error) {
+						props.error = error instanceof Error ? error : { message: error };
+						props.status = status;
+					}
+
 					let redirect: { statusCode: number, location: string };
-					let error: { statusCode: number, message: Error | string };
+					let preload_error: { statusCode: number, message: Error | string };
 
 					Promise.resolve(
 						mod.preload ? mod.preload.call({
@@ -205,7 +221,7 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter:
 								redirect = { statusCode, location };
 							},
 							error: (statusCode: number, message: Error | string) => {
-								error = { statusCode, message };
+								preload_error = { statusCode, message };
 							},
 							fetch: (url: string, opts?: any) => {
 								const parsed = new URL(url, `http://127.0.0.1:${process.env.PORT}${req.baseUrl ? req.baseUrl + '/'  :''}`);
@@ -245,7 +261,7 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter:
 						store
 						}, req) : {}
 					).catch(err => {
-						error = { statusCode: 500, message: err };
+						preload_error = { statusCode: 500, message: err };
 					}).then(preloaded => {
 						if (redirect) {
 							res.statusCode = redirect.statusCode;
@@ -255,8 +271,8 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter:
 							return;
 						}
 
-						if (error) {
+						if (preload_error) {
-							handle_error(req, res, error.statusCode, error.message);
+							handle_route(error_route, req, res, preload_error.statusCode, preload_error.message);
 							return;
 						}
 
@@ -294,6 +310,7 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter:
 							.replace('%sapper.head%', `<noscript id='sapper-head-start'></noscript>${head}<noscript id='sapper-head-end'></noscript>`)
 							.replace('%sapper.styles%', (css && css.code ? `<style>${css.code}</style>` : ''));
 
+						res.statusCode = status;
 						res.end(page);
 
 						if (process.send) {
@@ -370,84 +387,25 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter:
 					}
 				}
 			} catch (error) {
-				handle_error(req, res, 500, error);
+				if (route.error) {
+					// there was an error rendering the error page!
+					res.statusCode = status;
+					res.end(error instanceof Error ? error.message : error);
+				} else {
+					handle_route(error_route, req, res, 500, error || 'Internal server error');
+				}
 			}
 		}
 
 		next();
 	}
 
-	const not_found_route = routes.find((route: RouteObject) => route.error === '4xx');
-	const error_route = routes.find((route: RouteObject) => route.error === '5xx');
-
-	function handle_error(req: Req, res: ServerResponse, statusCode: number, message: Error | string) {
-		res.statusCode = statusCode;
-		res.setHeader('Content-Type', 'text/html');
-
-		const error = message instanceof Error ? message : new Error(message);
-
-		const not_found = statusCode >= 400 && statusCode < 500;
-
-		const route = not_found
-			? not_found_route
-			: error_route;
-
-		function render_page({ head, css, html }) {
-			const page = template()
-				.replace('%sapper.base%', `<base href="${req.baseUrl}/">`)
-				.replace('%sapper.scripts%', `<script>__SAPPER__={baseUrl: "${req.baseUrl}"}</script><script src='${req.baseUrl}/client/${get_chunks().main}'></script>`)
-				.replace('%sapper.html%', html)
-				.replace('%sapper.head%', `<noscript id='sapper-head-start'></noscript>${head}<noscript id='sapper-head-end'></noscript>`)
-				.replace('%sapper.styles%', (css && css.code ? `<style>${css.code}</style>` : ''));
-
-			res.end(page);
-		}
-
-		function handle_notfound() {
-			const title: string = not_found
-				? 'Not found'
-				: `Internal server error: ${error.message}`;
-
-			render_page({ head: '', css: null, html: title });
-		}
-
-		if (route) {
-			const handlers = route.handlers[Symbol.iterator]();
-
-			function next() {
-				const { value: handler, done } = handlers.next();
-
-				if (done) {
-					handle_notfound();
-				} else if (handler.type === 'page') {
-					render_page(handler.module.render({
-						status: statusCode,
-						error
-					}, {
-						store: store_getter && store_getter(req)
-					}));
-				} else {
-					const handle_method = mod[method_export];
-					if (handle_method) {
-						handle_method(req, res, next);
-					} else {
-						next();
-					}
-				}
-			}
-
-			next();
-		} else {
-			handle_notfound();
-		}
-	}
-
 	return function find_route(req: Req, res: ServerResponse) {
 		for (const route of routes) {
 			if (!route.error && route.pattern.test(req.path)) return handle_route(route, req, res);
 		}
 
-		handle_error(req, res, 404, 'Not found');
+		handle_route(error_route, req, res, 404, 'Not found');
 	};
 }
 

src/runtime/index.ts

@@ -8,7 +8,7 @@ export let component: Component;
 let target: Node;
 let store: Store;
 let routes: Route[];
-let errors: { '4xx': Route, '5xx': Route };
+let error_route: Route;
 
 const history = typeof window !== 'undefined' ? window.history : {
 	pushState: (state: any, title: string, href: string) => {},
@@ -117,11 +117,7 @@ function prepare_route(Page: ComponentConstructor, props: RouteData) {
 		error = { statusCode: 500, message: err };
 	}).then(preloaded => {
 		if (error) {
-			const route = error.statusCode >= 400 && error.statusCode < 500
+			return error_route.load().then(({ default: Page }: { default: ComponentConstructor }) => {
-				? errors['4xx']
-				: errors['5xx'];
-
-			return route.load().then(({ default: Page }: { default: ComponentConstructor }) => {
 				const err = error.message instanceof Error ? error.message : new Error(error.message);
 				Object.assign(props, { status: error.statusCode, error: err });
 				return { Page, props, redirect: null };
@@ -212,7 +208,11 @@ function handle_popstate(event: PopStateEvent) {
 	if (event.state) {
 		const url = new URL(window.location.href);
 		const target = select_route(url);
-		navigate(target, event.state.id);
+		if (target) {
+			navigate(target, event.state.id);
+		} else {
+			window.location.href = window.location.href;
+		}
 	} else {
 		// hashchange
 		cid = ++uid;
@@ -262,10 +262,7 @@ export function init(opts: { App: ComponentConstructor, target: Node, routes: Ro
 	App = opts.App;
 	target = opts.target;
 	routes = opts.routes.filter(r => !r.error);
-	errors = {
+	error_route = opts.routes.find(r => r.error);
-		'4xx': opts.routes.find(r => r.error === '4xx'),
-		'5xx': opts.routes.find(r => r.error === '5xx')
-	};
 
 	if (opts && opts.store) {
 		store = opts.store(manifest.store);
@@ -293,7 +290,7 @@ export function init(opts: { App: ComponentConstructor, target: Node, routes: Ro
 		history.replaceState({ id: uid }, '', href);
 
 		const target = select_route(new URL(window.location.href));
-		return navigate(target, uid);
+		if (target) return navigate(target, uid);
 	});
 }
 

src/runtime/interfaces.ts

@@ -17,7 +17,7 @@ export interface Component {
 export type Route = {
 	pattern: RegExp;
 	load: () => Promise<{ default: ComponentConstructor }>;
-	error?: string;
+	error?: boolean;
 	params?: (match: RegExpExecArray) => Record<string, string>;
 	ignore?: boolean;
 };

test/app/routes/5xx.html

@@ -1,6 +0,0 @@
-<svelte:head>
-	<title>Internal server error</title>
-</svelte:head>
-
-<h1>Internal server error</h1>
-<p>{error.message}</p>

test/app/routes/_error.html

@@ -2,5 +2,5 @@
 	<title>{status}</title>
 </svelte:head>
 
-<h1>Not found</h1>
+<h1>{status}</h1>
 <p>{error.message}</p>

test/common/test.js

@@ -439,7 +439,7 @@ function run({ mode, basepath = '' }) {
 					})
 					.then(() => nightmare.page.title())
 					.then(title => {
-						assert.equal(title, 'Not found')
+						assert.equal(title, '404')
 					});
 			});
 
@@ -454,7 +454,7 @@ function run({ mode, basepath = '' }) {
 					})
 					.then(() => nightmare.page.title())
 					.then(title => {
-						assert.equal(title, 'Not found');
+						assert.equal(title, '404');
 					});
 			});
 
@@ -466,7 +466,7 @@ function run({ mode, basepath = '' }) {
 					})
 					.then(() => nightmare.page.title())
 					.then(title => {
-						assert.equal(title, 'Internal server error')
+						assert.equal(title, '500')
 					});
 			});
 
@@ -481,7 +481,7 @@ function run({ mode, basepath = '' }) {
 					})
 					.then(() => nightmare.page.title())
 					.then(title => {
-						assert.equal(title, 'Internal server error');
+						assert.equal(title, '500');
 					});
 			});
 

test/unit/create_routes.test.js

@@ -71,8 +71,7 @@ describe('create_routes', () => {
 				'blog/[slug].html',
 				'api/gists/[id].js',
 				'api/gists/index.js',
-				'4xx.html',
+				'_error.html',
-				'5xx.html',
 				'blog/index.html',
 				'blog/rss.xml.js',
 				'guide/index.html',
@@ -83,8 +82,7 @@ describe('create_routes', () => {
 		assert.deepEqual(
 			routes.map(r => r.handlers[0].file),
 			[
-				'4xx.html',
+				'_error.html',
-				'5xx.html',
 				'index.html',
 				'guide/index.html',
 				'blog/index.html',
@@ -99,8 +97,7 @@ describe('create_routes', () => {
 
 		routes = create_routes({
 			files: [
-				'4xx.html',
+				'_error.html',
-				'5xx.html',
 				'api/blog/[slug].js',
 				'api/blog/index.js',
 				'api/guide/contents.js',
@@ -119,8 +116,7 @@ describe('create_routes', () => {
 		assert.deepEqual(
 			routes.map(r => r.handlers[0].file),
 			[
-				'4xx.html',
+				'_error.html',
-				'5xx.html',
 				'index.html',
 				'guide/index.html',
 				'blog/index.html',