throwing a Redirect is incorrectly handled as an Error, not a Response. #8617
Comments
|
We can't investigate without a repro |
|
I am not sure if it is related, but today (SK1.1.4) I need to remove .svelte-kit folder to "fix" this error when redirecting. #8539 Rich, it is very hard for me made a repo, because the code is so big and this error not always happens. Seams always happens in dev mode, no production mode. |
|
Closing as duplicate of #8523 |
|
@dummdidumm FYI this happens exactly when released v1.1.0 and still as today (1.1.4) |
|
Are you sure it's 1.1.0 and not 1.0.13 already? I suspect #8429 has to do with this. |
|
I am not sure. I remember 1.0.12 runs ok. Then upgrade to 1.1.0. But wait, I have found something right now! I will back in few minutes |
|
Well, as I mentioned. here I describe the reproduction, and what I have found in Today I am running 1.1.4 ok. Then upgrade to 1.2.0, and fails again (thrown on every redirect from server as #8539). Well, before delete again the As an old cracker I was, I opened these two folders (the backed up and the new) and compare with a diff tool. I opened file by file. This is the result: This picture shows that all files and folder are the same content, except for server-internals.js because holds a new version.
So the difference comes here, into
And these are the lines different. Failed .svelte-kit folder (backed up) export type ActionData = Expand<Kit.AwaitedActions<typeof import('./+page.server.js').actions>> | null;
export type PageServerData = Expand<OptionalUnion<EnsureDefined<Kit.AwaitedProperties<Awaited<ReturnType<typeof import('./+page.server.js').load>>>>>>;The new regenerated export type ActionData = Expand<Kit.AwaitedActions<typeof import('../../../../../../../src/routes/accounts/edit/[id]/+page.server.js').actions>> | null;
export type PageServerData = Expand<OptionalUnion<EnsureDefined<Kit.AwaitedProperties<Awaited<ReturnType<typeof import('../../../../../../../src/routes/accounts/edit/[id]/+page.server.js').load>>>>>>;See the difference? The path. The regenerated folder created the files with the complete path. I think this happens after every upgrade of SK. I am using I hope this be helpful to address this issue. |
I was mistaken. I can confirm 1.0.12 works for me, while 1.0.13 does not. To confirm my suspicion, I've made the following changes locally (against diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js
index 5d61d730..f606f895 100644
--- a/packages/kit/src/runtime/client/client.js
+++ b/packages/kit/src/runtime/client/client.js
@@ -26,7 +26,7 @@ import { parse } from './parse.js';
import Root from '__GENERATED__/root.svelte';
import { nodes, server_loads, dictionary, matchers, hooks } from '__GENERATED__/client-manifest.js';
-import { HttpError, Redirect } from '../control.js';
+import { HttpError, is_redirect } from '../control.js';
import { stores } from './singletons.js';
import { unwrap_promises } from '../../utils/promises.js';
import * as devalue from 'devalue';
@@ -806,7 +806,7 @@ export function create_client({ target, base }) {
try {
branch.push(await branch_promises[i]);
} catch (err) {
- if (err instanceof Redirect) {
+ if (is_redirect(err)) {
return {
type: 'redirect',
location: err.location
@@ -1625,7 +1625,7 @@ export function create_client({ target, base }) {
route: routes.find(({ id }) => id === route.id) ?? null
});
} catch (error) {
- if (error instanceof Redirect) {
+ if (is_redirect(error)) {
// this is a real edge case — `load` would need to return
// a redirect but only in the browser
await native_navigation(new URL(error.location, location.href));
diff --git a/packages/kit/src/runtime/control.js b/packages/kit/src/runtime/control.js
index 87e0dcc2..fcd73135 100644
--- a/packages/kit/src/runtime/control.js
+++ b/packages/kit/src/runtime/control.js
@@ -61,3 +61,23 @@ export function replace_implementations(implementations) {
HttpError = implementations.HttpError;
Redirect = implementations.Redirect;
}
+
+/**
+ * @param {any} object
+ * @returns {object is Redirect}
+ */
+export function is_redirect(object) {
+ const instanceof_redirect = object instanceof Redirect;
+ const looks_like_redirect =
+ 'status' in object &&
+ 'location' in object &&
+ typeof object.status === 'number' &&
+ typeof object.location === 'string';
+ if (instanceof_redirect !== looks_like_redirect) {
+ console.error({
+ instanceof_redirect,
+ looks_like_redirect
+ });
+ }
+ return instanceof_redirect || looks_like_redirect;
+}
diff --git a/packages/kit/src/runtime/server/data/index.js b/packages/kit/src/runtime/server/data/index.js
index 8c0faa4f..e5aa40e2 100644
--- a/packages/kit/src/runtime/server/data/index.js
+++ b/packages/kit/src/runtime/server/data/index.js
@@ -1,4 +1,4 @@
-import { HttpError, Redirect } from '../../control.js';
+import { HttpError, Redirect, is_redirect } from '../../control.js';
import { normalize_error } from '../../../utils/error.js';
import { once } from '../../../utils/functions.js';
import { load_server_data } from '../page/load_data.js';
@@ -98,7 +98,7 @@ export async function render_data(
const nodes = await Promise.all(
promises.map((p, i) =>
p.catch(async (error) => {
- if (error instanceof Redirect) {
+ if (is_redirect(error)) {
throw error;
}
@@ -126,7 +126,7 @@ export async function render_data(
} catch (e) {
const error = normalize_error(e);
- if (error instanceof Redirect) {
+ if (is_redirect(error)) {
return redirect_json_response(error);
} else {
// TODO make it clearer that this was an unexpected error
diff --git a/packages/kit/src/runtime/server/endpoint.js b/packages/kit/src/runtime/server/endpoint.js
index fcce53a9..73fa3482 100644
--- a/packages/kit/src/runtime/server/endpoint.js
+++ b/packages/kit/src/runtime/server/endpoint.js
@@ -1,5 +1,5 @@
import { negotiate } from '../../utils/http.js';
-import { Redirect } from '../control.js';
+import { is_redirect } from '../control.js';
import { method_not_allowed } from './utils.js';
/**
@@ -55,7 +55,7 @@ export async function render_endpoint(event, mod, state) {
return response;
} catch (e) {
- if (e instanceof Redirect) {
+ if (is_redirect(e)) {
return new Response(undefined, {
status: e.status,
headers: { location: e.location }
diff --git a/packages/kit/src/runtime/server/page/actions.js b/packages/kit/src/runtime/server/page/actions.js
index afb3ee67..3c878775 100644
--- a/packages/kit/src/runtime/server/page/actions.js
+++ b/packages/kit/src/runtime/server/page/actions.js
@@ -2,7 +2,7 @@ import * as devalue from 'devalue';
import { error, json } from '../../../exports/index.js';
import { normalize_error } from '../../../utils/error.js';
import { is_form_content_type, negotiate } from '../../../utils/http.js';
-import { HttpError, Redirect, ActionFailure } from '../../control.js';
+import { HttpError, ActionFailure, is_redirect } from '../../control.js';
import { handle_error_and_jsonify } from '../utils.js';
/** @param {import('types').RequestEvent} event */
@@ -70,7 +70,7 @@ export async function handle_action_json_request(event, options, server) {
} catch (e) {
const error = normalize_error(e);
- if (error instanceof Redirect) {
+ if (is_redirect(error)) {
return action_json({
type: 'redirect',
status: error.status,
@@ -154,7 +154,7 @@ export async function handle_action_request(event, server) {
} catch (e) {
const error = normalize_error(e);
- if (error instanceof Redirect) {
+ if (is_redirect(error)) {
return {
type: 'redirect',
status: error.status,
diff --git a/packages/kit/src/runtime/server/page/index.js b/packages/kit/src/runtime/server/page/index.js
index cec1e42c..21cc5d0a 100644
--- a/packages/kit/src/runtime/server/page/index.js
+++ b/packages/kit/src/runtime/server/page/index.js
@@ -2,7 +2,7 @@ import { text } from '../../../exports/index.js';
import { compact } from '../../../utils/array.js';
import { normalize_error } from '../../../utils/error.js';
import { add_data_suffix } from '../../../utils/url.js';
-import { HttpError, Redirect } from '../../control.js';
+import { HttpError, is_redirect } from '../../control.js';
import {
get_option,
redirect_response,
@@ -232,7 +232,7 @@ export async function render_page(event, route, page, options, manifest, state,
} catch (e) {
const err = normalize_error(e);
- if (err instanceof Redirect) {
+ if (is_redirect(err)) {
if (state.prerendering && should_prerender_data) {
const body = JSON.stringify({
type: 'redirect',
diff --git a/packages/kit/src/runtime/server/page/respond_with_error.js b/packages/kit/src/runtime/server/page/respond_with_error.js
index 61082761..5fb2d68c 100644
--- a/packages/kit/src/runtime/server/page/respond_with_error.js
+++ b/packages/kit/src/runtime/server/page/respond_with_error.js
@@ -7,7 +7,7 @@ import {
redirect_response,
GENERIC_ERROR
} from '../utils.js';
-import { HttpError, Redirect } from '../../control.js';
+import { HttpError, is_redirect } from '../../control.js';
/**
* @typedef {import('./types.js').Loaded} Loaded
@@ -97,7 +97,7 @@ export async function respond_with_error({
} catch (e) {
// Edge case: If route is a 404 and the user redirects to somewhere from the root layout,
// we end up here.
- if (e instanceof Redirect) {
+ if (is_redirect(e)) {
return redirect_response(e.status, e.location);
}
diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js
index 07b7eab1..09b2db93 100644
--- a/packages/kit/src/runtime/server/respond.js
+++ b/packages/kit/src/runtime/server/respond.js
@@ -17,7 +17,7 @@ import { exec } from '../../utils/routing.js';
import { INVALIDATED_PARAM, redirect_json_response, render_data } from './data/index.js';
import { add_cookies_to_headers, get_cookies } from './cookie.js';
import { create_fetch } from './fetch.js';
-import { Redirect } from '../control.js';
+import { Redirect, is_redirect } from '../control.js';
import {
validate_common_exports,
validate_page_server_exports,
@@ -292,7 +292,7 @@ export async function respond(request, options, manifest, state) {
return response;
} catch (e) {
- if (e instanceof Redirect) {
+ if (is_redirect(e)) {
if (is_data_request) {
return redirect_json_response(e);
} else {
When I run my playwright tests, the server prints out a bunch of these: |
|
I closed it as a duplicate because the underlying reason is likely the same - still, thank you both for your in-depth analysis! |
|
I confirm that this bug is now fixed in @sveltejs/[email protected] (thanks to #8690). Thank you all for the great work and quick fixes. |
Describe the bug
Calling
throw redirect(code, path)throws aRedirectobject. (In JavaScript, anything can be thrown, it doesn't have to be anError)When SvelteKit catches a thrown object, it tries to deterimne whether the thrown object is a
Redirectobject (in which case it converts it to aResponse). Otherwise it is handled as an error.SvelteKit uses
error instanceof Redirectin a few places to make this check.This works fine if there is only one implementation of
Redirect, but there might actually be more than one. In that case, theinstanceofcheck will returnfalse, treating a redirect as an error.So why are there more than two implementations of
Redirect? I searched for "grotesque" to find out 😄 .kit/packages/kit/src/runtime/control.js
Lines 47 to 63 in c9d2711
In other words,
replace_implementationscauseserror instanceof Redirectto always returnfalse.This is what breaks the
redirectbehaviour.I'm not sure how best to fix this. Perhaps by replacing
error instanceof RedirectwithisRedirect(error):If you think this is a good solution I'd be happy to submit a PR.
Reproduction
I'm sorry, I haven't prepared one. But many people seem to have a problem with this on Discord.
Logs
No response
System Info
System: OS: macOS 13.0.1 CPU: (8) x64 Intel(R) Core(TM) i7-7920HQ CPU @ 3.10GHz Memory: 1.49 GB / 16.00 GB Shell: 5.8.1 - /bin/zsh Binaries: Node: 16.18.1 - ~/.asdf/installs/nodejs/16.18.1/bin/node npm: 8.19.2 - ~/.asdf/plugins/nodejs/shims/npm Browsers: Chrome: 109.0.5414.87 Firefox: 108.0.2 Safari: 16.1 npmPackages: @sveltejs/adapter-cloudflare: 1.0.4 => 1.0.4 @sveltejs/kit: 1.0.13 => 1.0.13 svelte: 3.55.1 => 3.55.1 vite: 4.0.4 => 4.0.4Severity
blocking all usage of SvelteKit
Additional Information
I've tried
@sveltejs/kitversions between1.0.10and1.1.4, and they all seem to have the bug.Because the bug has only surfaced in the past 24h or so (I think), it is probably triggered by a dependency that has moved underneath. Perhaps
vite.The text was updated successfully, but these errors were encountered: