chore: dedupe the form action request handlers

.changeset/action-error-result.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+chore: share the action error result between form actions and remote forms

packages/kit/src/runtime/app/server/remote/form.js

@@ -11,7 +11,11 @@ import {
 	parse_form_key
 } from '../../../form-utils.js';
 import { get_cache, get_implicit_lookup, run_remote_function } from './shared.js';
-import { ValidationError } from '@sveltejs/kit/internal';
+import { ActionFailure, ValidationError } from '@sveltejs/kit/internal';
+import { DEV } from 'esm-env';
+
+const incorrect_fail_message =
+	'`fail(...)` is for form actions. A remote `form` handler should call `invalid(...)` instead. See https://svelte.dev/docs/kit/remote-functions#form-Programmatic-validation';
 
 /**
  * Creates a form object that can be spread onto a `<form>` element.
@@ -118,9 +122,15 @@ export function form(validate_or_fn, maybe_fn) {
 							() => data,
 							(data) => (!maybe_fn ? fn() : fn(data, issue))
 						);
+
+						if (DEV && output.result instanceof ActionFailure) {
+							throw new Error(incorrect_fail_message);
+						}
 					} catch (e) {
 						if (e instanceof ValidationError) {
 							handle_issues(output, e.issues, form_data, __.id);
+						} else if (DEV && e instanceof ActionFailure) {
+							throw new Error(incorrect_fail_message, { cause: e });
 						} else {
 							throw e;
 						}

packages/kit/src/runtime/server/page/actions.js

@@ -1,6 +1,6 @@
 /** @import { RequestEvent, Actions } from '@sveltejs/kit' */
 /** @import { ActionResult } from '$app/forms' */
-/** @import { SSROptions, SSRNode, ServerNode } from 'types' */
+/** @import { SSROptions, SSRNode, ServerNode, ServerActionResult } from 'types' */
 import { DEV } from 'esm-env';
 import { HttpError, Redirect, ActionFailure, SvelteKitError } from '@sveltejs/kit/internal';
 import { with_request_store, merge_tracing, record_span } from '@sveltejs/kit/internal/server';
@@ -27,98 +27,44 @@ export function is_action_json_request(event) {
  * @param {SSRNode['server'] | undefined} server
  */
 export async function handle_action_json_request(event, state, options, server) {
-	const actions = server?.actions;
-	const location = get_action_location(event.url);
-
-	if (!actions) {
-		const no_actions_error = new SvelteKitError(
-			405,
-			'Method Not Allowed',
-			`POST method not allowed. No form actions exist for ${DEV ? `the page at ${event.route.id}` : 'this page'}`
-		);
+	const result = await handle_action_request(event, state, server);
+	return action_result_json(event, state, options, result);
+}
 
-		const error = await handle_error_and_jsonify(event, state, options, no_actions_error);
+/**
+ * @param {RequestEvent} event
+ * @param {import('types').RequestState} state
+ * @param {SSROptions} options
+ * @param {ServerActionResult} result
+ * @returns {Promise<Response>}
+ */
+async function action_result_json(event, state, options, result) {
+	if (result.type === 'redirect') {
+		return action_json(result);
+	}
 
-		return action_json(
-			{
-				type: 'error',
-				error,
-				location
-			},
-			{
-				status: error.status,
-				headers: {
-					// https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405
-					// "The server must generate an Allow header field in a 405 status code response"
-					allow: 'GET'
-				}
-			}
-		);
+	if (result.type === 'error') {
+		const error = await handle_error_and_jsonify(event, state, options, result.error);
+		return action_json({ ...result, error }, { status: error.status });
 	}
 
-	check_named_default_separate(actions);
+	if (result.type === 'success' && !result.data) {
+		return action_json({ ...result, status: 204, data: undefined });
+	}
 
 	try {
-		const data = await call_action(event, state, actions);
-
-		if (DEV) {
-			validate_action_return(data);
-		}
-
-		if (data instanceof ActionFailure) {
-			return action_json(
-				{
-					type: 'failure',
-					status: data.status,
-					location,
-					// @ts-expect-error we assign a string to what is supposed to be an object. That's ok
-					// because we don't use the object outside, and this way we have better code navigation
-					// through knowing where the related interface is used.
-					data: try_serialize(data.data, stringify, /** @type {string} */ (event.route.id))
-				},
-				{
-					status: data.status
-				}
-			);
-		} else if (data) {
-			return action_json({
-				type: 'success',
-				status: 200,
-				location,
-				// @ts-expect-error see comment above
-				data: try_serialize(data, stringify, /** @type {string} */ (event.route.id))
-			});
-		} else {
-			return action_json({
-				type: 'success',
-				status: 204,
-				location
-			});
-		}
-	} catch (e) {
-		const err = normalize_error(e);
-
-		if (err instanceof Redirect) {
-			return action_json_redirect(err);
-		}
-
-		const transformed = await handle_error_and_jsonify(
-			event,
-			state,
-			options,
-			check_incorrect_fail_use(err)
-		);
-
 		return action_json(
 			{
-				type: 'error',
-				error: transformed,
-				location
+				...result,
+				// @ts-expect-error we assign a string to what is supposed to be an object. That's ok
+				// because we don't use the object outside, and this way we have better code navigation
+				// through knowing where the related interface is used.
+				data: try_serialize(result.data, stringify, /** @type {string} */ (event.route.id))
 			},
-			{
-				status: transformed.status
-			}
+			{ status: result.status }
 		);
+	} catch (e) {
+		return action_result_json(event, state, options, action_error_result(e, result.location));
 	}
 }
 
@@ -139,12 +85,48 @@ export function get_action_location(url) {
 }
 
 /**
- * @param {HttpError | Error} error
+ * @param {RequestEvent} event
+ * @param {string} location
+ * @returns {Extract<ServerActionResult, { type: 'error' }>}
  */
-export function check_incorrect_fail_use(error) {
-	return error instanceof ActionFailure
-		? new Error('Cannot "throw fail()". Use "return fail()"')
-		: error;
+export function method_not_allowed_result(event, location) {
+	event.setHeaders({
+		// https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405
+		// "The server must generate an Allow header field in a 405 status code response"
+		allow: 'GET'
+	});
+	return {
+		type: 'error',
+		location,
+		error: new SvelteKitError(
+			405,
+			'Method Not Allowed',
+			`POST method not allowed. No form actions exist for ${DEV ? `the page at ${event.route.id}` : 'this page'}`
+		)
+	};
+}
+
+/**
+ * @param {unknown} e
+ * @param {string} location
+ * @returns {Extract<ServerActionResult, { type: 'redirect' | 'error' }>}
+ */
+export function action_error_result(e, location) {
+	const err = normalize_error(e);
+
+	if (err instanceof Redirect) {
+		return {
+			type: 'redirect',
+			status: err.status,
+			location: err.location
+		};
+	}
+
+	return {
+		type: 'error',
+		location,
+		error: err
+	};
 }
 
 /**
@@ -177,29 +159,15 @@ export function is_action_request(event) {
  * @param {RequestEvent} event
  * @param {import('types').RequestState} state
  * @param {SSRNode['server'] | undefined} server
- * @returns {Promise<ActionResult>}
+ * @returns {Promise<ServerActionResult>}
  */
 export async function handle_action_request(event, state, server) {
 	const actions = server?.actions;
 	const location = get_action_location(event.url);
 
 	if (!actions) {
 		// TODO should this be a different error altogether?
-		event.setHeaders({
-			// https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405
-			// "The server must generate an Allow header field in a 405 status code response"
-			allow: 'GET'
-		});
-		return {
-			type: 'error',
-			location,
-			// We're lying a bit with the types here; this will be transformed into a proper App.Error object later
-			error: new SvelteKitError(
-				405,
-				'Method Not Allowed',
-				`POST method not allowed. No form actions exist for ${DEV ? `the page at ${event.route.id}` : 'this page'}`
-			)
-		};
+		return method_not_allowed_result(event, location);
 	}
 
 	check_named_default_separate(actions);
@@ -228,22 +196,10 @@ export async function handle_action_request(event, state, server) {
 			};
 		}
 	} catch (e) {
-		const err = normalize_error(e);
-
-		if (err instanceof Redirect) {
-			return {
-				type: 'redirect',
-				status: err.status,
-				location: err.location
-			};
-		}
-
-		return {
-			type: 'error',
-			location,
-			// @ts-expect-error We're lying a bit with the types here; this will be transformed into a proper App.Error object later
-			error: check_incorrect_fail_use(err)
-		};
+		return action_error_result(
+			e instanceof ActionFailure ? new Error('Cannot "throw fail()". Use "return fail()"') : e,
+			location
+		);
 	}
 }
 

packages/kit/src/runtime/server/page/index.js

@@ -1,5 +1,4 @@
 /** @import { RequestEvent, SSRManifest } from '@sveltejs/kit' */
-/** @import { ActionResult } from '$app/forms' */
 /** @import { PageNodeIndexes, RequestState, RequiredResolveOptions, ServerDataNode, SSRNode, SSROptions } from 'types' */
 import { text } from '@sveltejs/kit';
 import { Redirect } from '@sveltejs/kit/internal';
@@ -57,7 +56,7 @@ export async function render_page(event, state, page, options, manifest, nodes,
 
 		let status = 200;
 
-		/** @type {ActionResult | undefined} */
+		/** @type {import('types').ServerActionResult | undefined} */
 		let action_result = undefined;
 
 		if (is_action_request(event)) {

packages/kit/src/runtime/server/page/render.js

@@ -43,7 +43,7 @@ import { has_custom_transporters, uneval } from '#app/internal/transport';
  *   event: import('@sveltejs/kit').RequestEvent;
  *   state: import('types').RequestState;
  *   resolve_opts: import('types').RequiredResolveOptions;
- *   action_result?: import('$app/forms').ActionResult;
+ *   action_result?: import('types').ServerActionResult;
  *   data_serializer: import('./types.js').ServerDataSerializer;
  *   error_components?: Array<import('svelte').Component | undefined>
  * }} opts

packages/kit/src/runtime/server/remote-functions.js

@@ -1,7 +1,6 @@
 /** @import { RequestEvent, SSRManifest } from '@sveltejs/kit' */
 /** @import { RemoteForm } from '$app/server' */
-/** @import { ActionResult } from '$app/forms' */
-/** @import { RemoteFormInternals, RemoteFunctionData, RemoteFunctionResponse, RemoteInternals, RequestState, SSROptions } from 'types' */
+/** @import { RemoteFormInternals, RemoteFunctionData, RemoteFunctionResponse, RemoteInternals, RequestState, ServerActionResult, SSROptions } from 'types' */
 
 import { error } from '@sveltejs/kit';
 import { Redirect, SvelteKitError } from '@sveltejs/kit/internal';
@@ -11,9 +10,11 @@ import { is_form_content_type } from '../../utils/http.js';
 import { create_remote_key, parse_remote_arg, split_remote_key } from '../shared.js';
 import { stringify } from '#app/internal/transport';
 import { handle_error_and_jsonify } from './errors.js';
-import { normalize_error } from '../../utils/error.js';
-import { check_incorrect_fail_use, get_action_location } from './page/actions.js';
-import { DEV } from 'esm-env';
+import {
+	action_error_result,
+	get_action_location,
+	method_not_allowed_result
+} from './page/actions.js';
 import { deserialize_binary_form } from '../form-utils.js';
 import { with_version_header } from './utils.js';
 
@@ -531,7 +532,7 @@ export async function handle_remote_form_post(event, state, manifest, id) {
  * @param {RequestState} state
  * @param {SSRManifest} manifest
  * @param {string} id
- * @returns {Promise<ActionResult>}
+ * @returns {Promise<ServerActionResult>}
  */
 async function handle_remote_form_post_internal(event, state, manifest, id) {
 	const location = get_action_location(event.url);
@@ -547,21 +548,7 @@ async function handle_remote_form_post_internal(event, state, manifest, id) {
 	);
 
 	if (!form) {
-		event.setHeaders({
-			// https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405
-			// "The server must generate an Allow header field in a 405 status code response"
-			allow: 'GET'
-		});
-		return {
-			type: 'error',
-			location,
-			// We're lying a bit with the types here; this will be transformed into a proper App.Error object later
-			error: new SvelteKitError(
-				405,
-				'Method Not Allowed',
-				`POST method not allowed. No form actions exist for ${DEV ? `the page at ${event.route.id}` : 'this page'}`
-			)
-		};
+		return method_not_allowed_result(event, location);
 	}
 
 	if (action_id) {
@@ -591,22 +578,7 @@ async function handle_remote_form_post_internal(event, state, manifest, id) {
 			location
 		};
 	} catch (e) {
-		const err = normalize_error(e);
-
-		if (err instanceof Redirect) {
-			return {
-				type: 'redirect',
-				status: err.status,
-				location: err.location
-			};
-		}
-
-		return {
-			type: 'error',
-			location,
-			// @ts-expect-error We're lying a bit with the types here; this will be transformed into a proper App.Error object later
-			error: check_incorrect_fail_use(err)
-		};
+		return action_error_result(e, location);
 	}
 }