Skip to content
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.

Sign up for GitHub

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

error if handle returns non-Response #3496

Merged
merged 2 commits into from
Jan 22, 2022
+13 −1
Merged

error if handle returns non-Response #3496

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6660811
error if handle returns non-Response
Rich-Harris Jan 21, 2022
9fa1e7c
change TODO comment
Rich-Harris Jan 22, 2022
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/green-mayflies-shave.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

Error if handle hook returns something other than a Response
9 changes: 8 additions & 1 deletion packages/kit/src/runtime/server/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export async function respond(request, options, state = {}) {
let ssr = true;

try {
return await options.hooks.handle({
const response = await options.hooks.handle({
event,
resolve: async (event, opts) => {
if (opts && 'ssr' in opts) ssr = /** @type {boolean} */ (opts.ssr);
Expand Down Expand Up @@ -195,6 +195,13 @@ export async function respond(request, options, state = {}) {
throw new Error('request in handle has been replaced with event' + details);
}
});

// TODO remove for 1.0
Copy link
Member

Choose a reason for hiding this comment

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

Validation might still be useful after 1.0 to make sure users don't do anything stupid. Personally I would remove the todo

Copy link
Contributor

Choose a reason for hiding this comment

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

Validation might still be useful after 1.0 to make sure users don't do anything stupid. Personally I would remove the todo

I'm obviously not on the Svelte developer team, but I agree with that, the check makes sense before and after 1.0 version

Copy link
Member Author

Choose a reason for hiding this comment

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

Checking might be useful, but pointing to the PR is specifically to help people migrate. After 1.0 they should just be directed to the docs. I changed the comment instead of removing it

if (response && !(response instanceof Response)) {
throw new Error('handle must return a Response object' + details);
}

return response;
} catch (/** @type {unknown} */ e) {
const error = coalesce_to_error(e);

Expand Down