From b2960d15eae3e272096f5f9332a2c5a61d2f167f Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Fri, 8 Dec 2023 14:36:49 +0100 Subject: [PATCH] fix: make sure promises from fetch handle errors 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 | 5 +++++ .../docs/20-core-concepts/20-load.md | 19 +++++++++++++++++++ packages/kit/src/runtime/server/fetch.js | 16 ++++++++++++++-- .../kit/src/runtime/server/page/load_data.js | 12 +++++++++++- .../basics/src/routes/streaming/+page.svelte | 1 + .../streaming/server-error/+page.server.js | 15 +++++++++++++++ .../streaming/server-error/+page.svelte | 12 ++++++++++++ .../kit/test/apps/basics/test/client.test.js | 14 ++++++++++++++ 8 files changed, 91 insertions(+), 3 deletions(-) create mode 100644 .changeset/soft-colts-tell.md create mode 100644 packages/kit/test/apps/basics/src/routes/streaming/server-error/+page.server.js create mode 100644 packages/kit/test/apps/basics/src/routes/streaming/server-error/+page.svelte diff --git a/.changeset/soft-colts-tell.md b/.changeset/soft-colts-tell.md new file mode 100644 index 000000000000..e3c41ad5bf16 --- /dev/null +++ b/.changeset/soft-colts-tell.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: make sure promises from fetch handle errors diff --git a/documentation/docs/20-core-concepts/20-load.md b/documentation/docs/20-core-concepts/20-load.md index d582e9987904..a1c04b755d41 100644 --- a/documentation/docs/20-core-concepts/20-load.md +++ b/documentation/docs/20-core-concepts/20-load.md @@ -488,6 +488,25 @@ This is useful for creating skeleton loading states, for example:

``` +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. diff --git a/packages/kit/src/runtime/server/fetch.js b/packages/kit/src/runtime/server/fetch.js index 8431e06fc5c8..d97abcdccafd 100644 --- a/packages/kit/src/runtime/server/fetch.js +++ b/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; + }; } /** diff --git a/packages/kit/src/runtime/server/page/load_data.js b/packages/kit/src/runtime/server/page/load_data.js index e24dad5b3bed..fe10b1891f97 100644 --- a/packages/kit/src/runtime/server/page/load_data.js +++ b/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, '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; + }; } /** diff --git a/packages/kit/test/apps/basics/src/routes/streaming/+page.svelte b/packages/kit/test/apps/basics/src/routes/streaming/+page.svelte index afaf91fb15ed..9f3ea1c4a0fe 100644 --- a/packages/kit/test/apps/basics/src/routes/streaming/+page.svelte +++ b/packages/kit/test/apps/basics/src/routes/streaming/+page.svelte @@ -1,2 +1,3 @@ Universal Server +Server Error diff --git a/packages/kit/test/apps/basics/src/routes/streaming/server-error/+page.server.js b/packages/kit/test/apps/basics/src/routes/streaming/server-error/+page.server.js new file mode 100644 index 000000000000..02513bcaadb9 --- /dev/null +++ b/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/') + } + }; +} diff --git a/packages/kit/test/apps/basics/src/routes/streaming/server-error/+page.svelte b/packages/kit/test/apps/basics/src/routes/streaming/server-error/+page.svelte new file mode 100644 index 000000000000..5b07fde4cfa8 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/streaming/server-error/+page.svelte @@ -0,0 +1,12 @@ + + +

{data.eager}

+ +{#await data.lazy.fail} +

loading

+{:catch} +

fail

+{/await} diff --git a/packages/kit/test/apps/basics/test/client.test.js b/packages/kit/test/apps/basics/test/client.test.js index aac4812f51c1..b10006c34436 100644 --- a/packages/kit/test/apps/basics/test/client.test.js +++ b/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'); + }); } });