Skip to content

fix: make sure promises from fetch handle errors #11228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/soft-colts-tell.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix: make sure promises from fetch handle errors
19 changes: 19 additions & 0 deletions documentation/docs/20-core-concepts/20-load.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
16 changes: 14 additions & 2 deletions packages/kit/src/runtime/server/fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
Expand All @@ -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) => {
Expand Down Expand Up @@ -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;
};
}

/**
Expand Down
12 changes: 11 additions & 1 deletion packages/kit/src/runtime/server/page/load_data.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -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;
};
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
<a href="/streaming/universal">Universal</a>
<a href="/streaming/server">Server</a>
<a href="/streaming/server-error">Server Error</a>
Original file line number Diff line number Diff line change
@@ -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/')
}
};
}
Original file line number Diff line number Diff line change
@@ -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}
14 changes: 14 additions & 0 deletions packages/kit/test/apps/basics/test/client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
Expand Down Expand Up @@ -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');
});
}
});

Expand Down