Skip to content
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/honest-parrots-compare.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

Handle `throw error/redirect` in `+server.js`
34 changes: 24 additions & 10 deletions packages/kit/src/runtime/server/endpoint.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { HttpError, Redirect } from '../../index/private.js';
import { check_method_names, method_not_allowed } from './utils.js';

/**
Expand Down Expand Up @@ -29,16 +30,29 @@ export async function render_endpoint(event, route) {
return method_not_allowed(mod, method);
}

const response = await handler(
/** @type {import('types').RequestEvent<Record<string, any>>} */ (event)
);

if (!(response instanceof Response)) {
return new Response(
`Invalid response from route ${event.url.pathname}: handler should return a Response object`,
{ status: 500 }
try {
const response = await handler(
/** @type {import('types').RequestEvent<Record<string, any>>} */ (event)
);
}

return response;
if (!(response instanceof Response)) {
return new Response(
`Invalid response from route ${event.url.pathname}: handler should return a Response object`,
{ status: 500 }
);
}

return response;
} catch (error) {
if (error instanceof HttpError) {
return new Response(error.message, { status: error.status });
} else if (error instanceof Redirect) {
return new Response(undefined, {
status: error.status,
headers: { Location: error.location }
});
} else {
throw error;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be converted into a Response, too? If it isn't this means unexpected errors get a regular "page" treatment and they are rendered within possible +error.svelte pages which seems wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yeah

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though actually... i wonder if that should happen at the bottom of respond. We already have a TODO comment there asking this exact question.

that way we get all the options.handle_error and sanitization logic centralised

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gah, ok that's not possible because we want to render an error page if user code errors in handle (at least, that's the decision we made at some point in the past).

i do feel like there's some opportunities for simplification around error handling generally. might noodle in a separate PR. will merge this in the meantime though, no point in delaying it

}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { error } from '@sveltejs/kit';

export function GET() {
throw error(401, 'You shall not pass');
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { redirect } from '@sveltejs/kit';

export function GET() {
throw redirect(302, '/');
}
20 changes: 20 additions & 0 deletions packages/kit/test/apps/basics/test/server.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,26 @@ test.describe('Errors', () => {
'This is your custom error page saying: "Cannot read properties of undefined (reading \'toUpperCase\')"'
);
});

test('throw error(..) in endpoint', async ({ page, read_errors }) => {
const res = await page.goto('/errors/endpoint-throw-error');

const error = read_errors('/errors/endpoint-throw-error');
expect(error).toBe(undefined);

expect(await res?.text()).toBe('You shall not pass');
expect(res?.status()).toBe(401);
});

test('throw redirect(..) in endpoint', async ({ page, read_errors }) => {
const res = await page.goto('/errors/endpoint-throw-redirect');
expect(res?.status()).toBe(200); // redirects are opaque to the browser

const error = read_errors('/errors/endpoint-throw-redirect');
expect(error).toBe(undefined);

expect(await page.textContent('h1')).toBe('the answer is 42');
});
});

test.describe('Load', () => {
Expand Down