throwing a Redirect is incorrectly handled as an Error, not a Response.

[aslakhellesoy]

Describe the bug

Calling throw redirect(code, path) throws a Redirect object. (In JavaScript, anything can be thrown, it doesn't have to be an Error)

When SvelteKit catches a thrown object, it tries to deterimne whether the thrown object is a Redirect object (in which case it converts it to a Response). Otherwise it is handled as an error.

SvelteKit uses error instanceof Redirect in 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, the instanceof check will return false, 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

	/**
	* This is a grotesque hack that, in dev, allows us to replace the implementations
	* of these classes that you'd get by importing them from `@sveltejs/kit` with the
	* ones that are imported via Vite and loaded internally, so that instanceof
	* checks work even though SvelteKit imports this module via Vite and consumers
	* import it via Node
	* @param {{
	* ActionFailure: typeof ActionFailure;
	* HttpError: typeof HttpError;
	* Redirect: typeof Redirect;
	* }} implementations
	*/
	export function replace_implementations(implementations) {
	ActionFailure = implementations.ActionFailure;
	HttpError = implementations.HttpError;
	Redirect = implementations.Redirect;
	}

In other words, replace_implementations causes error instanceof Redirect to always return false.
This is what breaks the redirect behaviour.

I'm not sure how best to fix this. Perhaps by replacing error instanceof Redirect with isRedirect(error):

function isRedirect(object) {
    return 'status' in object && 
           'location' in object && 
           typeof object.status === 'number' && 
           typeof object.location === 'string'
}

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.4

Severity

blocking all usage of SvelteKit

Additional Information

I've tried @sveltejs/kit versions between 1.0.10 and 1.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.

[Rich-Harris]

We can't investigate without a repro

[NormandoHall]

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.

[dummdidumm]

Closing as duplicate of #8523
We need to investigate this somehow, it seems there may be some weird instances where this happens from time to time.

[NormandoHall]

@dummdidumm FYI this happens exactly when released v1.1.0 and still as today (1.1.4)

[dummdidumm]

Are you sure it's 1.1.0 and not 1.0.13 already? I suspect #8429 has to do with this.

[NormandoHall]

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

[NormandoHall]

Well, as I mentioned. here I describe the reproduction, and what I have found in .svelte-kit folder.

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 .svelte-kit folder, I backed up. Then start again SK in dev mode, and the error is gone. So, the new folder .svelte-kit is recreated at startup.

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 /types/src/routes/.../$types.d.ts

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 .svelte-kit folder

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 npm update --save every time a new SK release. Maybe this break the generation of types.

I hope this be helpful to address this issue.

[aslakhellesoy]

I've tried @sveltejs/kit versions between 1.0.10 and 1.1.4, and they all seem to have the bug.

I was mistaken. I can confirm 1.0.12 works for me, while 1.0.13 does not.
I think it's more likely that this is caused by a regression in #8429 (causing instanceof not to behave as expected), rather than being a duplicate of #8523.

To confirm my suspicion, I've made the following changes locally (against master):

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:

{ instanceof_redirect: false, looks_like_redirect: true }
{ instanceof_redirect: false, looks_like_redirect: true }
{ instanceof_redirect: false, looks_like_redirect: true }

[dummdidumm]

I closed it as a duplicate because the underlying reason is likely the same - still, thank you both for your in-depth analysis!

[paulovieira]

I confirm that this bug is now fixed in @sveltejs/kit@1.2.9 (thanks to #8690).

Thank you all for the great work and quick fixes.