From 0aeb63a05bcb1f8966fa8fd8175481f655c29030 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 27 Jun 2018 18:35:41 -0400 Subject: [PATCH 1/4] simplify rendering of error pages --- src/middleware.ts | 99 +++++++++++------------------------------------ 1 file changed, 23 insertions(+), 76 deletions(-) diff --git a/src/middleware.ts b/src/middleware.ts index 05bb025..49720c0 100644 --- a/src/middleware.ts +++ b/src/middleware.ts @@ -162,8 +162,13 @@ 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) { - req.params = route.params(route.pattern.exec(req.path)); + const not_found_route = routes.find((route: RouteObject) => route.error === '4xx'); + const error_route = routes.find((route: RouteObject) => route.error === '5xx'); + + 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 +179,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(not_found_route, req, res, 404, 'Not found'); + } + return; } @@ -186,7 +198,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[`_${route.error}`]) // TODO this is gross .filter(file => !file.match(/\.map$/)) .map(file => `<${req.baseUrl}/client/${file}>;rel="preload";as="script"`) .join(', '); @@ -197,7 +209,7 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter: const props = { params: req.params, query: req.query, path: req.path }; 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 +217,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 +257,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 +267,8 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter: return; } - if (error) { - handle_error(req, res, error.statusCode, error.message); + if (preload_error) { + handle_route(error_route, req, res, preload_error.statusCode, preload_error.message); return; } @@ -370,84 +382,19 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter: } } } catch (error) { - handle_error(req, res, 500, error); + 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%', ``) - .replace('%sapper.scripts%', ``) - .replace('%sapper.html%', html) - .replace('%sapper.head%', `${head}`) - .replace('%sapper.styles%', (css && css.code ? `` : '')); - - 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(not_found_route, req, res, 404, 'Not found'); }; } From e87247493f1b865aaffe143a3df82f9836421563 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 28 Jun 2018 11:38:21 -0400 Subject: [PATCH 2/4] replace 4xx.html and 5xx.html with _error.html --- src/core/create_manifests.ts | 8 ++++---- src/core/create_routes.ts | 8 +++----- src/middleware.ts | 23 +++++++++++++++++------ src/runtime/index.ts | 13 +++---------- src/runtime/interfaces.ts | 2 +- test/app/routes/5xx.html | 6 ------ test/app/routes/{4xx.html => _error.html} | 2 +- test/app/routes/blog/[slug].html | 2 +- test/common/test.js | 8 ++++---- test/unit/create_routes.test.js | 12 ++++-------- 10 files changed, 38 insertions(+), 46 deletions(-) delete mode 100644 test/app/routes/5xx.html rename test/app/routes/{4xx.html => _error.html} (80%) diff --git a/src/core/create_manifests.ts b/src/core/create_manifests.ts index 1135a8c..58f3075 100644 --- a/src/core/create_manifests.ts +++ b/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') { - return `{ error: '${route.id.slice(1)}', load: () => import(/* webpackChunkName: "${route.id}" */ '${file}') }`; + if (route.id === '_error') { + 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') { - return `{ error: '${route.id.slice(1)}', handlers: [${handlers}] }`; + if (route.id === '_error') { + return `{ error: true, handlers: [${handlers}] }`; } const params = route.params.length === 0 diff --git a/src/core/create_routes.ts b/src/core/create_routes.ts index a6239ff..4515076 100644 --- a/src/core/create_routes.ts +++ b/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 (b.parts[0] === '4xx' || b.parts[0] === '5xx') return 1; + if (a.parts[0] === '_error') return -1; + if (b.parts[0] === '_error') return 1; const max = Math.max(a.parts.length, b.parts.length); diff --git a/src/middleware.ts b/src/middleware.ts index 49720c0..0ca1a99 100644 --- a/src/middleware.ts +++ b/src/middleware.ts @@ -162,8 +162,7 @@ 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')); - const not_found_route = routes.find((route: RouteObject) => route.error === '4xx'); - const error_route = routes.find((route: RouteObject) => route.error === '5xx'); + const error_route = routes.find((route: RouteObject) => route.error); function handle_route(route: RouteObject, req: Req, res: ServerResponse, status = 200, error: Error | string = null) { req.params = error @@ -184,7 +183,7 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter: res.statusCode = status; res.end(error instanceof Error ? error.message : error); } else { - handle_route(not_found_route, req, res, 404, 'Not found'); + handle_route(error_route, req, res, 404, 'Not found'); } return; @@ -198,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] || chunks[`_${route.error}`]) // TODO this is gross + .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(', '); @@ -208,6 +207,11 @@ 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 preload_error: { statusCode: number, message: Error | string }; @@ -306,6 +310,7 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter: .replace('%sapper.head%', `${head}`) .replace('%sapper.styles%', (css && css.code ? `` : '')); + res.statusCode = status; res.end(page); if (process.send) { @@ -382,7 +387,13 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter: } } } catch (error) { - handle_route(error_route, req, res, 500, error || 'Internal server 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'); + } } } @@ -394,7 +405,7 @@ function get_route_handler(App: Component, routes: RouteObject[], store_getter: if (!route.error && route.pattern.test(req.path)) return handle_route(route, req, res); } - handle_route(not_found_route, req, res, 404, 'Not found'); + handle_route(error_route, req, res, 404, 'Not found'); }; } diff --git a/src/runtime/index.ts b/src/runtime/index.ts index e890bcf..5185385 100644 --- a/src/runtime/index.ts +++ b/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 - ? errors['4xx'] - : errors['5xx']; - - return route.load().then(({ default: Page }: { default: ComponentConstructor }) => { + return error_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 }; @@ -262,10 +258,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 = { - '4xx': opts.routes.find(r => r.error === '4xx'), - '5xx': opts.routes.find(r => r.error === '5xx') - }; + error_route = opts.routes.find(r => r.error); if (opts && opts.store) { store = opts.store(manifest.store); diff --git a/src/runtime/interfaces.ts b/src/runtime/interfaces.ts index d141039..00f814c 100644 --- a/src/runtime/interfaces.ts +++ b/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; ignore?: boolean; }; diff --git a/test/app/routes/5xx.html b/test/app/routes/5xx.html deleted file mode 100644 index 1425e2a..0000000 --- a/test/app/routes/5xx.html +++ /dev/null @@ -1,6 +0,0 @@ - - Internal server error - - -

Internal server error

-

{error.message}

\ No newline at end of file diff --git a/test/app/routes/4xx.html b/test/app/routes/_error.html similarity index 80% rename from test/app/routes/4xx.html rename to test/app/routes/_error.html index c63ca84..9974a17 100644 --- a/test/app/routes/4xx.html +++ b/test/app/routes/_error.html @@ -2,5 +2,5 @@ {status} -

Not found

+

{status}

{error.message}

\ No newline at end of file diff --git a/test/app/routes/blog/[slug].html b/test/app/routes/blog/[slug].html index 7fdc8b4..7836fc7 100644 --- a/test/app/routes/blog/[slug].html +++ b/test/app/routes/blog/[slug].html @@ -19,7 +19,7 @@ return this.error(500, 'something went wrong'); } - return fetch(`blog/${slug}.json`).then(r => { + return fetch(`blog/${slug}.json`).then(async r => { if (r.status === 200) { return r.json().then(post => ({ post })); this.error(r.status, '') diff --git a/test/common/test.js b/test/common/test.js index 3c637eb..84fdd17 100644 --- a/test/common/test.js +++ b/test/common/test.js @@ -441,7 +441,7 @@ function run({ mode, basepath = '' }) { }) .then(() => nightmare.page.title()) .then(title => { - assert.equal(title, 'Not found') + assert.equal(title, '404') }); }); @@ -456,7 +456,7 @@ function run({ mode, basepath = '' }) { }) .then(() => nightmare.page.title()) .then(title => { - assert.equal(title, 'Not found'); + assert.equal(title, '404'); }); }); @@ -468,7 +468,7 @@ function run({ mode, basepath = '' }) { }) .then(() => nightmare.page.title()) .then(title => { - assert.equal(title, 'Internal server error') + assert.equal(title, '500') }); }); @@ -483,7 +483,7 @@ function run({ mode, basepath = '' }) { }) .then(() => nightmare.page.title()) .then(title => { - assert.equal(title, 'Internal server error'); + assert.equal(title, '500'); }); }); diff --git a/test/unit/create_routes.test.js b/test/unit/create_routes.test.js index e4e33f6..f299464 100644 --- a/test/unit/create_routes.test.js +++ b/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', - '5xx.html', + '_error.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', - '5xx.html', + '_error.html', 'index.html', 'guide/index.html', 'blog/index.html', @@ -99,8 +97,7 @@ describe('create_routes', () => { routes = create_routes({ files: [ - '4xx.html', - '5xx.html', + '_error.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', - '5xx.html', + '_error.html', 'index.html', 'guide/index.html', 'blog/index.html', From 5c4e4d5d36e2a2de128efc27e3eb01bf3ebcf31b Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 28 Jun 2018 11:45:38 -0400 Subject: [PATCH 3/4] only navigate if route is found - fixes #279 --- src/runtime/index.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/runtime/index.ts b/src/runtime/index.ts index 5185385..b2308f6 100644 --- a/src/runtime/index.ts +++ b/src/runtime/index.ts @@ -208,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; @@ -286,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); }); } From 2205b8aec53aff53ccdfdca4e3d31e431ad8cb75 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 28 Jun 2018 12:03:27 -0400 Subject: [PATCH 4/4] oops --- test/app/routes/blog/[slug].html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/app/routes/blog/[slug].html b/test/app/routes/blog/[slug].html index 7836fc7..7fdc8b4 100644 --- a/test/app/routes/blog/[slug].html +++ b/test/app/routes/blog/[slug].html @@ -19,7 +19,7 @@ return this.error(500, 'something went wrong'); } - return fetch(`blog/${slug}.json`).then(async r => { + return fetch(`blog/${slug}.json`).then(r => { if (r.status === 200) { return r.json().then(post => ({ post })); this.error(r.status, '')