handle non-200 status codes from root __layout.svelte (#4665)

* failing test for #4659 * prevent infinite loop when loading non-existent route in __layout * update test * tighten up * changeset * Update .changeset/purple-rats-wink.md * wut
sveltejs · Jul 14, 2022 · 67e2342 · 67e2342
1 parent 9ad8b6b
commit 67e2342
Show file tree

Hide file tree

Showing 12 changed files with 56 additions and 9 deletions.
diff --git a/.changeset/purple-rats-wink.md b/.changeset/purple-rats-wink.md
@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+Render generic error page if `__layout` returns error while rendering full error page
diff --git a/packages/kit/src/runtime/load.js b/packages/kit/src/runtime/load.js
@@ -26,7 +26,10 @@ export function normalize(loaded) {
 		const status = loaded.status;
 
 		if (!loaded.error && has_error_status) {
-			return { status: status || 500, error: new Error() };
+			return {
+				status: status || 500,
+				error: new Error(`${status}`)
+			};
 		}
 
 		const error = typeof loaded.error === 'string' ? new Error(loaded.error) : loaded.error;

diff --git a/packages/kit/src/runtime/server/index.js b/packages/kit/src/runtime/server/index.js
@@ -3,7 +3,7 @@ import { render_page } from './page/index.js';
 import { render_response } from './page/render.js';
 import { respond_with_error } from './page/respond_with_error.js';
 import { coalesce_to_error } from '../../utils/error.js';
-import { decode_params, serialize_error } from './utils.js';
+import { decode_params, serialize_error, GENERIC_ERROR } from './utils.js';
 import { normalize_path } from '../../utils/url.js';
 import { exec } from '../../utils/routing.js';
 import { negotiate } from '../../utils/http.js';
@@ -274,6 +274,12 @@ export async function respond(request, options, state) {
 					}
 				}
 
+				if (state.initiator === GENERIC_ERROR) {
+					return new Response('Internal Server Error', {
+						status: 500
+					});
+				}
+
 				// if this request came direct from the user, rather than
 				// via a `fetch` in a `load`, render a 404 page
 				if (!state.initiator) {
@@ -328,6 +334,7 @@ export async function respond(request, options, state) {
 			});
 		}
 
+		// TODO is this necessary? should we just return a plain 500 at this point?
 		try {
 			const $session = await options.hooks.getSession(event);
 			return await respond_with_error({

diff --git a/packages/kit/src/runtime/server/page/load_node.js b/packages/kit/src/runtime/server/page/load_node.js
@@ -13,7 +13,7 @@ import { domain_matches, path_matches } from './cookie.js';
  *   event: import('types').RequestEvent;
  *   options: import('types').SSROptions;
  *   state: import('types').SSRState;
- *   route: import('types').SSRPage | null;
+ *   route: import('types').SSRPage | import('types').SSRErrorPage;
  *   node: import('types').SSRNode;
  *   $session: any;
  *   stuff: Record<string, any>;

diff --git a/packages/kit/src/runtime/server/page/respond_with_error.js b/packages/kit/src/runtime/server/page/respond_with_error.js
@@ -1,6 +1,7 @@
 import { render_response } from './render.js';
 import { load_node } from './load_node.js';
 import { coalesce_to_error } from '../../../utils/error.js';
+import { GENERIC_ERROR } from '../utils.js';
 
 /**
  * @typedef {import('./types.js').Loaded} Loaded
@@ -41,7 +42,7 @@ export async function respond_with_error({
 					event,
 					options,
 					state,
-					route: null,
+					route: GENERIC_ERROR,
 					node: default_layout,
 					$session,
 					stuff: {},
@@ -50,12 +51,16 @@ export async function respond_with_error({
 				})
 			);
 
+			if (layout_loaded.loaded.error) {
+				throw layout_loaded.loaded.error;
+			}
+
 			const error_loaded = /** @type {Loaded} */ (
 				await load_node({
 					event,
 					options,
 					state,
-					route: null,
+					route: GENERIC_ERROR,
 					node: default_error,
 					$session,
 					stuff: layout_loaded ? layout_loaded.stuff : {},

diff --git a/packages/kit/src/runtime/server/utils.js b/packages/kit/src/runtime/server/utils.js
@@ -96,3 +96,8 @@ function clone_error(error, get_stack) {
 
 	return object;
 }
+
+/** @type {import('types').SSRErrorPage} */
+export const GENERIC_ERROR = {
+	id: '__error'
+};
diff --git a/packages/kit/test/apps/basics/src/routes/__layout.svelte b/packages/kit/test/apps/basics/src/routes/__layout.svelte
@@ -1,6 +1,14 @@
 <script context="module">
 	/** @type {import('@sveltejs/kit').Load} */
-	export async function load() {
+	export async function load({ fetch, url }) {
+		if (url.pathname.startsWith('/errors/error-in-layout')) {
+			const res = await fetch('/errors/error-in-layout/non-existent');
+
+			return {
+				status: res.status
+			};
+		}
+
 		return {
 			props: {
 				foo: {

diff --git a/packages/kit/test/apps/basics/src/routes/errors/error-in-layout/index.svelte b/packages/kit/test/apps/basics/src/routes/errors/error-in-layout/index.svelte
@@ -0,0 +1 @@
+<h1>hello</h1>
diff --git a/packages/kit/test/apps/basics/test/client.test.js b/packages/kit/test/apps/basics/test/client.test.js
@@ -317,7 +317,7 @@ test.describe('Errors', () => {
 		await page.goto('/errors/load-status-without-error-client');
 
 		expect(await page.textContent('footer')).toBe('Custom layout');
-		expect(await page.textContent('#message')).toBe('This is your custom error page saying: ""');
+		expect(await page.textContent('#message')).toBe('This is your custom error page saying: "401"');
 		expect(await page.innerHTML('h1')).toBe('401');
 	});
 });

diff --git a/packages/kit/test/apps/basics/test/server.test.js b/packages/kit/test/apps/basics/test/server.test.js
@@ -356,6 +356,15 @@ test.describe('Errors', () => {
 	});
 });
 
+test.describe('Load', () => {
+	test('fetching a non-existent resource in root layout fails without hanging', async ({
+		request
+	}) => {
+		const response = await request.get('/errors/error-in-layout');
+		expect(await response.text()).toContain('Error: 500');
+	});
+});
+
 test.describe('Routing', () => {
 	test('event.params are available in handle', async ({ request }) => {
 		const response = await request.get('/routing/params-in-handle/banana');

diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js
@@ -505,7 +505,7 @@ test.describe('Errors', () => {
 		const response = await page.goto('/errors/load-status-without-error-server');
 
 		expect(await page.textContent('footer')).toBe('Custom layout');
-		expect(await page.textContent('#message')).toBe('This is your custom error page saying: ""');
+		expect(await page.textContent('#message')).toBe('This is your custom error page saying: "401"');
 		expect(/** @type {Response} */ (response).status()).toBe(401);
 	});
 

diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts
@@ -290,6 +290,10 @@ export interface SSRPage {
 	b: Array<number | undefined>;
 }
 
+export interface SSRErrorPage {
+	id: '__error';
+}
+
 export interface SSRPagePart {
 	id: string;
 	load: SSRComponentLoader;
@@ -300,7 +304,7 @@ export type SSRRoute = SSREndpoint | SSRPage;
 export interface SSRState {
 	fallback?: string;
 	getClientAddress: () => string;
-	initiator?: SSRPage | null;
+	initiator?: SSRPage | SSRErrorPage;
 	platform?: any;
 	prerendering?: PrerenderOptions;
 }