Skip to content

Commit

Permalink
[breaking] render raw response when unexpected error occurs in endpoi…
Browse files Browse the repository at this point in the history 
…nt (#6434)

* [breaking] render raw response when unexpected error occurs in endpoint

..instead of failing completely. Also catch possible fetch error (which would occur when network fails).
Closes #4801

There are still some code paths which would bubble up to the handler, but these are of the kind "you use a wrong SvelteKit option"

* take into account json, call handle error

* simplify

Co-authored-by: Rich Harris <hello@rich-harris.dev>
  • Loading branch information
@dummdidumm @Rich-Harris
dummdidumm and Rich-Harris authored Aug 31, 2022
1 parent b34539a commit eb24cf5
Show file tree
Hide file tree
Showing 7 changed files with 178 additions and 168 deletions.
5 changes: 5 additions & 0 deletions .changeset/few-ghosts-attend.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

[breaking] catch and render raw response when unexpected error occurs in endpoint
17 changes: 7 additions & 10 deletions packages/kit/src/runtime/server/endpoint.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { HttpError, Redirect } from '../control.js';
import { Redirect } from '../control.js';
import { check_method_names, method_not_allowed } from './utils.js';

/**
Expand Down Expand Up @@ -39,9 +39,8 @@ export async function render_endpoint(event, mod, state) {
);

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

Expand All @@ -52,15 +51,13 @@ export async function render_endpoint(event, mod, state) {

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

throw error;
}
}
261 changes: 118 additions & 143 deletions packages/kit/src/runtime/server/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@ import { render_page } from './page/index.js';
import { render_response } from './page/render.js';
import { respond_with_error } from './page/respond_with_error.js';
import { coalesce_to_error } from '../../utils/error.js';
import { serialize_error, GENERIC_ERROR, static_error_page } from './utils.js';
import { GENERIC_ERROR, handle_fatal_error } from './utils.js';
import { decode_params, disable_search, normalize_path } from '../../utils/url.js';
import { exec } from '../../utils/routing.js';
import { negotiate } from '../../utils/http.js';
import { render_data } from './data/index.js';
import { DATA_SUFFIX } from '../../constants.js';

Expand Down Expand Up @@ -190,175 +189,151 @@ export async function respond(request, options, state) {
transformPageChunk: default_transform
};

try {
const response = await options.hooks.handle({
event,
resolve: async (event, opts) => {
if (opts) {
// TODO remove for 1.0
if ('transformPage' in opts) {
throw new Error(
'transformPage has been replaced by transformPageChunk — see https://github.com/sveltejs/kit/pull/5657 for more information'
);
}

if ('ssr' in opts) {
throw new Error(
'ssr has been removed, set it in the appropriate +layout.js instead. See the PR for more information: https://github.com/sveltejs/kit/pull/6197'
);
}

resolve_opts = {
transformPageChunk: opts.transformPageChunk || default_transform
};
/**
*
* @param {import('types').RequestEvent} event
* @param {import('types').ResolveOptions} [opts]
*/
async function resolve(event, opts) {
try {
if (opts) {
// TODO remove for 1.0
if ('transformPage' in opts) {
throw new Error(
'transformPage has been replaced by transformPageChunk — see https://github.com/sveltejs/kit/pull/5657 for more information'
);
}

if (state.prerendering?.fallback) {
return await render_response({
event,
options,
state,
page_config: { ssr: false, csr: true },
status: 200,
error: null,
branch: [],
fetched: [],
validation_errors: undefined,
cookies: [],
resolve_opts
});
if ('ssr' in opts) {
throw new Error(
'ssr has been removed, set it in the appropriate +layout.js instead. See the PR for more information: https://github.com/sveltejs/kit/pull/6197'
);
}

if (route) {
/** @type {Response} */
let response;

if (is_data_request) {
response = await render_data(event, route, options, state);
} else if (route.page) {
response = await render_page(event, route, route.page, options, state, resolve_opts);
} else if (route.endpoint) {
response = await render_endpoint(event, await route.endpoint(), state);
} else {
// a route will always have a page or an endpoint, but TypeScript
// doesn't know that
throw new Error('This should never happen');
}
resolve_opts = {
transformPageChunk: opts.transformPageChunk || default_transform
};
}

if (!is_data_request) {
// we only want to set cookies on __data.js requests, we don't
// want to cache stuff erroneously etc
for (const key in headers) {
const value = headers[key];
response.headers.set(key, /** @type {string} */ (value));
}
}
if (state.prerendering?.fallback) {
return await render_response({
event,
options,
state,
page_config: { ssr: false, csr: true },
status: 200,
error: null,
branch: [],
fetched: [],
validation_errors: undefined,
cookies: [],
resolve_opts
});
}

if (route) {
/** @type {Response} */
let response;

for (const cookie of cookies) {
response.headers.append('set-cookie', cookie);
if (is_data_request) {
response = await render_data(event, route, options, state);
} else if (route.page) {
response = await render_page(event, route, route.page, options, state, resolve_opts);
} else if (route.endpoint) {
response = await render_endpoint(event, await route.endpoint(), state);
} else {
// a route will always have a page or an endpoint, but TypeScript
// doesn't know that
throw new Error('This should never happen');
}

if (!is_data_request) {
// we only want to set cookies on __data.js requests, we don't
// want to cache stuff erroneously etc
for (const key in headers) {
const value = headers[key];
response.headers.set(key, /** @type {string} */ (value));
}
}

// respond with 304 if etag matches
if (response.status === 200 && response.headers.has('etag')) {
let if_none_match_value = request.headers.get('if-none-match');
for (const cookie of cookies) {
response.headers.append('set-cookie', cookie);
}

// ignore W/ prefix https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match#directives
if (if_none_match_value?.startsWith('W/"')) {
if_none_match_value = if_none_match_value.substring(2);
}
// respond with 304 if etag matches
if (response.status === 200 && response.headers.has('etag')) {
let if_none_match_value = request.headers.get('if-none-match');

const etag = /** @type {string} */ (response.headers.get('etag'));
// ignore W/ prefix https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match#directives
if (if_none_match_value?.startsWith('W/"')) {
if_none_match_value = if_none_match_value.substring(2);
}

if (if_none_match_value === etag) {
const headers = new Headers({ etag });
const etag = /** @type {string} */ (response.headers.get('etag'));

// https://datatracker.ietf.org/doc/html/rfc7232#section-4.1
for (const key of ['cache-control', 'content-location', 'date', 'expires', 'vary']) {
const value = response.headers.get(key);
if (value) headers.set(key, value);
}
if (if_none_match_value === etag) {
const headers = new Headers({ etag });

return new Response(undefined, {
status: 304,
headers
});
// https://datatracker.ietf.org/doc/html/rfc7232#section-4.1
for (const key of ['cache-control', 'content-location', 'date', 'expires', 'vary']) {
const value = response.headers.get(key);
if (value) headers.set(key, value);
}
}

return response;
return new Response(undefined, {
status: 304,
headers
});
}
}

if (state.initiator === GENERIC_ERROR) {
return new Response('Internal Server Error', {
status: 500
});
}
return response;
}

// if this request came direct from the user, rather than
// via a `fetch` in a `load`, render a 404 page
if (!state.initiator) {
return await respond_with_error({
event,
options,
state,
status: 404,
error: new Error(`Not found: ${event.url.pathname}`),
resolve_opts
});
}
if (state.initiator === GENERIC_ERROR) {
return new Response('Internal Server Error', {
status: 500
});
}

if (state.prerendering) {
return new Response('not found', { status: 404 });
}
// if this request came direct from the user, rather than
// via a `fetch` in a `load`, render a 404 page
if (!state.initiator) {
return await respond_with_error({
event,
options,
state,
status: 404,
error: new Error(`Not found: ${event.url.pathname}`),
resolve_opts
});
}

// we can't load the endpoint from our own manifest,
// so we need to make an actual HTTP request
return await fetch(request);
},
if (state.prerendering) {
return new Response('not found', { status: 404 });
}

// we can't load the endpoint from our own manifest,
// so we need to make an actual HTTP request
return await fetch(request);
} catch (e) {
const error = coalesce_to_error(e);
return handle_fatal_error(event, options, error);
}
}

try {
return await options.hooks.handle({
event,
resolve,
// TODO remove for 1.0
// @ts-expect-error
get request() {
throw new Error('request in handle has been replaced with event' + details);
}
});

// TODO for 1.0, change the error message to point to docs rather than PR
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);

options.handle_error(error, event);

const type = negotiate(event.request.headers.get('accept') || 'text/html', [
'text/html',
'application/json'
]);

if (is_data_request || type === 'application/json') {
return new Response(serialize_error(error, options.get_stack), {
status: 500,
headers: { 'content-type': 'application/json; charset=utf-8' }
});
}

// TODO is this necessary? should we just return a plain 500 at this point?
try {
return await respond_with_error({
event,
options,
state,
status: 500,
error,
resolve_opts
});
} catch (/** @type {unknown} */ e) {
const error = coalesce_to_error(e);
return static_error_page(options, 500, error.message);
}
return handle_fatal_error(event, options, error);
}
}
2 changes: 1 addition & 1 deletion packages/kit/src/runtime/server/page/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ export async function render_page(event, route, page, options, state, resolve_op
});
} catch (error) {
// if we end up here, it means the data loaded successfull
// but the page failed to render
// but the page failed to render, or that a prerendering error occurred
options.handle_error(/** @type {Error} */ (error), event);

return await respond_with_error({
Expand Down
32 changes: 32 additions & 0 deletions packages/kit/src/runtime/server/utils.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { devalue } from 'devalue';
import { DATA_SUFFIX } from '../../constants.js';
import { negotiate } from '../../utils/http.js';
import { HttpError } from '../control.js';

/** @param {any} body */
Expand Down Expand Up @@ -175,3 +177,33 @@ export function static_error_page(options, status, message) {
status
});
}

/**
* @param {import('types').RequestEvent} event
* @param {import('types').SSROptions} options
* @param {Error} error
*/
export function handle_fatal_error(event, options, error) {
let status = 500;

if (error instanceof HttpError) {
status = error.status;
} else {
options.handle_error(error, event);
}

// ideally we'd use sec-fetch-dest instead, but Safari — quelle surprise — doesn't support it
const type = negotiate(event.request.headers.get('accept') || 'text/html', [
'text/html',
'application/json'
]);

if (event.url.pathname.endsWith(DATA_SUFFIX) || type === 'application/json') {
return new Response(serialize_error(error, options.get_stack), {
status,
headers: { 'content-type': 'application/json; charset=utf-8' }
});
}

return static_error_page(options, status, error.message);
}
Loading

0 comments on commit eb24cf5

Please sign in to comment.