fix: make sure promises from fetch handle errors (#11228)

Ensures that people using `fetch` directly in their load functions don't run into uncaught promise errors with streaming.
Also adds some docs for this case.
related to #9785

.changeset/soft-colts-tell.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+fix: make sure promises from fetch handle errors

documentation/docs/20-core-concepts/20-load.md

@@ -488,6 +488,25 @@ This is useful for creating skeleton loading states, for example:
 </p>
 ```
 
+When streaming data, be careful to handle promise rejections correctly. More specifically, the server could crash with an "unhandled promise rejection" error if a lazy-loaded promise fails before rendering starts (at which point it's caught) and isn't handling the error in some way. When using SvelteKit's `fetch` directly in the `load` function, SvelteKit will handle this case for you. For other promises, it is enough to attach a noop-`catch` to the promise to mark it as handled.
+
+```js
+/// file: src/routes/+page.server.js
+/** @type {import('./$types').PageServerLoad} */
+export function load({ fetch }) {
+	const ok_manual = Promise.reject();
+	ok_manual.catch(() => {});
+
+	return {
+		streamed: {
+			ok_manual,
+			ok_fetch: fetch('/fetch/that/could/fail'),
+			dangerous_unhandled: Promise.reject()
+		}
+	};
+}
+```
+
 > On platforms that do not support streaming, such as AWS Lambda, responses will be buffered. This means the page will only render once all promises resolve. If you are using a proxy (e.g. NGINX), make sure it does not buffer responses from the proxied server.
 
 > Streaming data will only work when JavaScript is enabled. You should avoid returning nested promises from a universal `load` function if the page is server rendered, as these are _not_ streamed — instead, the promise is recreated when the function reruns in the browser.

packages/kit/src/runtime/server/fetch.js

@@ -14,7 +14,10 @@ import * as paths from '__sveltekit/paths';
  * @returns {typeof fetch}
  */
 export function create_fetch({ event, options, manifest, state, get_cookie_header, set_internal }) {
-	return async (info, init) => {
+	/**
+	 * @type {typeof fetch}
+	 */
+	const server_fetch = async (info, init) => {
 		const original_request = normalize_fetch_input(info, init, event.url);
 
 		// some runtimes (e.g. Cloudflare) error if you access `request.mode`,
@@ -23,7 +26,7 @@ export function create_fetch({ event, options, manifest, state, get_cookie_heade
 		let credentials =
 			(info instanceof Request ? info.credentials : init?.credentials) ?? 'same-origin';
 
-		return await options.hooks.handleFetch({
+		return options.hooks.handleFetch({
 			event,
 			request: original_request,
 			fetch: async (info, init) => {
@@ -144,6 +147,15 @@ export function create_fetch({ event, options, manifest, state, get_cookie_heade
 			}
 		});
 	};
+
+	// Don't make this function `async`! Otherwise, the user has to `catch` promises they use for streaming responses or else
+	// it will be an unhandled rejection. Instead, we add a `.catch(() => {})` ourselves below to this from happening.
+	return (input, init) => {
+		// See docs in fetch.js for why we need to do this
+		const response = server_fetch(input, init);
+		response.catch(() => {});
+		return response;
+	};
 }
 
 /**

packages/kit/src/runtime/server/page/load_data.js

@@ -195,13 +195,14 @@ export async function load_data({
  * @param {import('./types.js').Fetched[]} fetched
  * @param {boolean} csr
  * @param {Pick<Required<import('@sveltejs/kit').ResolveOptions>, 'filterSerializedResponseHeaders'>} resolve_opts
+ * @returns {typeof fetch}
  */
 export function create_universal_fetch(event, state, fetched, csr, resolve_opts) {
 	/**
 	 * @param {URL | RequestInfo} input
 	 * @param {RequestInit} [init]
 	 */
-	return async (input, init) => {
+	const universal_fetch = async (input, init) => {
 		const cloned_body = input instanceof Request && input.body ? input.clone().body : null;
 
 		const cloned_headers =
@@ -329,6 +330,15 @@ export function create_universal_fetch(event, state, fetched, csr, resolve_opts)
 
 		return proxy;
 	};
+
+	// Don't make this function `async`! Otherwise, the user has to `catch` promises they use for streaming responses or else
+	// it will be an unhandled rejection. Instead, we add a `.catch(() => {})` ourselves below to this from happening.
+	return (input, init) => {
+		// See docs in fetch.js for why we need to do this
+		const response = universal_fetch(input, init);
+		response.catch(() => {});
+		return response;
+	};
 }
 
 /**

packages/kit/test/apps/basics/src/routes/streaming/+page.svelte

@@ -1,2 +1,3 @@
 <a href="/streaming/universal">Universal</a>
 <a href="/streaming/server">Server</a>
+<a href="/streaming/server-error">Server Error</a>

packages/kit/test/apps/basics/src/routes/streaming/server-error/+page.server.js

@@ -0,0 +1,15 @@
+// Tests the case where a lazy promise is rejected before the rendering started
+export async function load({ fetch }) {
+	const eager = new Promise((resolve) => {
+		setTimeout(() => {
+			resolve('eager');
+		}, 100);
+	});
+
+	return {
+		eager: await eager,
+		lazy: {
+			fail: fetch('http://localhost:1337/')
+		}
+	};
+}

packages/kit/test/apps/basics/src/routes/streaming/server-error/+page.svelte

@@ -0,0 +1,12 @@
+<script>
+	/** @type {import('./$types').PageData} */
+	export let data;
+</script>
+
+<p class="eager">{data.eager}</p>
+
+{#await data.lazy.fail}
+	<p class="loadingfail">loading</p>
+{:catch}
+	<p class="fail">fail</p>
+{/await}

packages/kit/test/apps/basics/test/client.test.js

@@ -761,6 +761,14 @@ test.describe('Streaming', () => {
 		expect(page.locator('p.loadingfail')).toBeHidden();
 	});
 
+	test('Catches fetch errors from server load functions (client nav)', async ({ page }) => {
+		await page.goto('/streaming');
+		page.click('[href="/streaming/server-error"]');
+
+		await expect(page.locator('p.eager')).toHaveText('eager');
+		expect(page.locator('p.fail')).toBeVisible();
+	});
+
 	// TODO `vite preview` buffers responses, causing these tests to fail
 	if (process.env.DEV) {
 		test('Works for universal load functions (direct hit)', async ({ page }) => {
@@ -802,6 +810,12 @@ test.describe('Streaming', () => {
 			expect(page.locator('p.loadingsuccess')).toBeHidden();
 			expect(page.locator('p.loadingfail')).toBeHidden();
 		});
+
+		test('Catches fetch errors from server load functions (direct hit)', async ({ page }) => {
+			page.goto('/streaming/server-error');
+			await expect(page.locator('p.eager')).toHaveText('eager');
+			await expect(page.locator('p.fail')).toHaveText('fail');
+		});
 	}
 });