Thrown exceptions from __layout.svelte load result in plain text error responses

[Conduitry]

Describe the bug

Throwing an exception (or returning a rejecting promise) from a load function should be equivalent to returning { status: 500, error } (apart from the former also resulting in a call to the handleError hook). However, currently, it results in a plain text error message response from the server, which should be returned only when the __error.svelte component fails to render.

Reproduction

Starting from the demo app, add this to __layout.svelte:

<script context="module">
	export function load() {
		throw new Error("FOO");
	}
</script>

Navigate to any page.

Logs

No response

System Info

System:
    OS: Linux 5.10 Ubuntu 20.04.3 LTS (Focal Fossa)
    CPU: (8) x64 Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz
    Memory: 6.36 GB / 12.32 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Yarn: 1.22.15
    npm: 8.1.4
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.4
    @sveltejs/kit: next => 1.0.0-next.202
    svelte: ^3.44.0 => 3.44.3

Severity

serious, but I can work around it

Additional Information

This is inconvenient because I'd like to use the handleError hook as the sole place for performing certain kinds of error logging, but I'd also prefer to not return a plain text error response to users. This means that currently my __layout.svelte's load function needs to catch the error and reimplement the handleError handling in a different way (since it has access to load arguments instead of to the request object).

[Conduitry]

The issue seems to be in https://github.com/sveltejs/kit/blob/master/packages/kit/src/runtime/server/page/respond_with_error.js where the first call to load_node for the layout component is returning a rejecting promise. I could make it not do that and (when is_leaf and is_error are both false - i.e., when this is a layout component) instead catch the error thrown by module.load.call and turn it into a { status, error } object but something about that feels a bit unclean. It might be better to make this the responsibility of respond_with_error, but that also feels unclean, and also trickier to do, and requires more duplication of load_node logic.

[Conduitry]

Implementation aside, I'm now having a bit of a question about what the desired behavior even is.

Currently, when the layout load returns { status, error } for a page, this switches us over to rendering an error page, at which point the layout load is called a second time as part of rendering the error page. Is this what we want? Is this avoidable?

The behavior here when { status, error } is returned might not matter a ton because load should probably be idempotent, but if it throws an error, that's no longer idempotent. Do we want to call the handleError hook twice, once for when we encountered the error while rendering a page, and a second time for when we encountered the error while rendering the error page?

[Conduitry]

What I have right now is

diff --git a/packages/kit/src/runtime/server/page/load_node.js b/packages/kit/src/runtime/server/page/load_node.js
index 9cb9b1b8..4c4c6a52 100644
--- a/packages/kit/src/runtime/server/page/load_node.js
+++ b/packages/kit/src/runtime/server/page/load_node.js
@@ -2,6 +2,7 @@ import { normalize } from '../../load.js';
 import { respond } from '../index.js';
 import { escape_json_string_in_html } from '../../../utils/escape.js';
 import { is_root_relative, resolve } from '../../../utils/url.js';
+import { coalesce_to_error } from '../../../utils/error.js';
 
 const s = JSON.stringify;
 
@@ -282,7 +283,15 @@ export async function load_node({
 			/** @type {import('types/page').ErrorLoadInput} */ (load_input).error = error;
 		}
 
-		loaded = await module.load.call(null, load_input);
+		try {
+			loaded = await module.load.call(null, load_input);
+		} catch (err) {
+			if (!is_leaf && !is_error) {
+				loaded = { status: 500, error: coalesce_to_error(err) };
+			} else {
+				throw err;
+			}
+		}
 	} else {
 		loaded = {};
 	}

which appears to break a number of existing tests, and also doesn't call handleError at all when the layout load function throws an error. I'm not sure whether this is telling me that I'm on the entirely wrong track.

Edit: Whoops, fixed typo. There's no failing tests now, at least. I'm still not sure whether this is the right track.

[Conduitry]

I'm feeling a little better about this:

diff --git a/packages/kit/src/runtime/server/page/load_node.js b/packages/kit/src/runtime/server/page/load_node.js
index 9cb9b1b8..02bd9ac9 100644
--- a/packages/kit/src/runtime/server/page/load_node.js
+++ b/packages/kit/src/runtime/server/page/load_node.js
@@ -1,5 +1,6 @@
 import { normalize } from '../../load.js';
 import { respond } from '../index.js';
+import { coalesce_to_error } from '../../../utils/error.js';
 import { escape_json_string_in_html } from '../../../utils/escape.js';
 import { is_root_relative, resolve } from '../../../utils/url.js';
 
@@ -18,6 +19,7 @@ const s = JSON.stringify;
  *   prerender_enabled: boolean;
  *   is_leaf: boolean;
  *   is_error: boolean;
+ *   handle_error: any | undefined;
  *   status?: number;
  *   error?: Error;
  * }} opts
@@ -35,6 +37,7 @@ export async function load_node({
 	prerender_enabled,
 	is_leaf,
 	is_error,
+	handle_error,
 	status,
 	error
 }) {
@@ -282,7 +285,16 @@ export async function load_node({
 			/** @type {import('types/page').ErrorLoadInput} */ (load_input).error = error;
 		}
 
-		loaded = await module.load.call(null, load_input);
+		try {
+			loaded = await module.load.call(null, load_input);
+		} catch (err) {
+			if (handle_error) {
+				handle_error(err);
+				loaded = { status: 500, error: coalesce_to_error(err) };
+			} else {
+				throw err;
+			}
+		}
 	} else {
 		loaded = {};
 	}
diff --git a/packages/kit/src/runtime/server/page/respond_with_error.js b/packages/kit/src/runtime/server/page/respond_with_error.js
index 986a788b..97f82de3 100644
--- a/packages/kit/src/runtime/server/page/respond_with_error.js
+++ b/packages/kit/src/runtime/server/page/respond_with_error.js
@@ -43,7 +43,8 @@ export async function respond_with_error({ request, options, state, $session, st
 			stuff: {},
 			prerender_enabled: is_prerender_enabled(options, default_error, state),
 			is_leaf: false,
-			is_error: false
+			is_error: false,
+			handle_error: err => options.handle_error(err, request)
 		})
 	);

(although it still has some type issues). In this case, load_node is explicitly told when it should handle errors from the underlying load function (instead of throwing that error), and it then also uses this as a function to call to note that an error has been handled. This function is only passed during the call to load_node for the layout in respond_with_error, and it just calls the underlying handle_error function with that error and the appropriate request object.

[nhe23]

I think #3239 fixed this by calling respond_with_error after handler_error is run. At least I'm seeing the __error.svelte page when throwing an exception in a load function (tested with the hn example). However the originally thrown exception with my message is not shown. Instead im seeing this:

[Conduitry]

The behavior here hasn't changed, at least as far as I can tell.

It's also unclear to me how the __layout.svelte load function ought to be avoiding throwing the same error again when it's being called a second time while rendering the __error.svelte route. It can't tell whether it's currently being used to render an error page or not, and thus can't tell whether it should skip making some of the potentially problematic calls that got it into this error state to begin with.

[bluwy]

A bit tangential, but would it make sense if we leave it as is, but for dev mode, we show VIte's error overlay of the thrown error? This could differentiate between expected error (from load) and unexpected error (from throw).

[Conduitry]

🤷 I mention in the "Additional Information" section specifically why I'm looking for this behavior. I currently have to duplicate some handleError hook logging logic in the __layout load.

[georgecrawford]

@Conduitry @bluwy Are you still seeing this with the latest SvelteKit? I think things have been improved. I see the error reported in the handleError() hook, and the __error.svelte page correctly rending it (both with client-side navigation and SSR).

[Conduitry]

I'm still seeing in 1.0.0-next.285 what I was seeing before. If the __layout.svelte load throws an error, SvelteKit attempts to render __error.svelte, which results in __layout.svelte load being called again, which throws an error again, and renders the plain text error page.

[cdcarson]

I'm not sure if this is the same issue, but a similar thing happens when you navigate (with Javascript enabled) to a page where a shadow endpoint throws an error. Rather than the error message thrown by the endpoint I get "Failed to load data".

Few other notes:

• If you do not navigate (i.e. if you are on the page already and refresh) the "right" error is displayed.
• If you turn off javascript and navigate the right error is always displayed.
• If you log the error in __error.svelte load, you always get the right result on the server even you get the wrong result in the client.
• It's not a dev/prod issue. The same behavior happens when you build it.

Here's a minimal repro: https://github.com/cdcarson/svelte-throw-endpoint-error-issue

• Go to http://localhost:3000. Click on the "Route where the endpoint always throws" link. You'll see "Failed to load data."
• Refresh the same page. You'll see "My special endpoint error." This is what I'd like to be able to show to users.

I'm using SvelteKit v1.0.0-next.302.

Let me know if this needs to be a separate issue. Not being able to throw errors consistently is a pretty fundamental flaw/stumbling block. Thanks.

[dummdidumm]

The problem occurs when the root layout throws an error. Other layouts are not rerun, but the root layout is still rerun in the "fall back to something" case. I think the solution to this and #2154 is to not use the user-defined root layout if it exists but rather always have one layout above that which is just a <slot />, or remove the default_layout logic in client.js (which is only used in the fatal error case, where I argue correctness is more important than the possibility for a nicer UI)

[Rich-Harris]

It's now possible to (group) your roots and put the root +error.svelte outside the group...

src/routes/
├ (main)
│ ├ foo/
│ ├ bar/
│ ├ etc/
│ ├ +layout.svelte # can safely throw errors in here
│ └ +page.svelte
├ +error.svelte   
└ +layout.svelte   # optional

...which mostly solves this issue. That said, there are always going to be cases where we need to bail out to something (e.g. the +error.svelte can't render because it references data.undefined.something).

Right now, that's a plain text response. I think we should make that a more appealing HTML page — something very basic, but nicer than what we currently have — and allow people to create a src/error.html page that overrides it, allowing you to at least render a branded page with a 'visit our Twitter page for updates' or whatever.

Unlike +error.svelte, we can guarantee that src/error.html can be rendered without errors.

Changing the plain text error response to HTML would be a breaking change, so I've added the label.