From 1c3265a8c9c0b1b1bd597f756b63463146bacc3a Mon Sep 17 00:00:00 2001
From: Ben Holmes <hey@bholmes.dev>
Date: Tue, 30 Jul 2024 11:42:52 -0400
Subject: [PATCH] Actions: Make `.safe()` the default return value (#11571)

* feat: new orThrow types

* fix: parens on return type

* feat: switch implementation to orThrow()

* feat(e2e): update PostComment

* fix: remove callSafely from middleware

* fix: toString() for actions

* fix(e2e): more orThrow updates

* feat: remove progressive enhancement from orThrow

* fix: remove _astroActionSafe handler from react

* feat(e2e): update test to use safe calling

* chore: console log

* chore: unused import

* fix: add rewriting: true to test fixture

* fix: correctly throw for server-only actions

* chore: changeset

* fix: update type tests

* fix(test): remove .safe() chain

* docs: use "patch" with BREAKING CHANGE notice

* docs: clarify react integration in changeset
---
 .changeset/light-chairs-happen.md             | 29 ++++++++++
 .../actions-blog/src/components/Like.tsx      |  2 +-
 .../src/components/PostComment.tsx            |  2 +-
 .../actions-react-19/src/actions/index.ts     |  7 +--
 .../actions-react-19/src/components/Like.tsx  |  4 +-
 packages/astro/src/@types/astro.ts            |  2 +-
 .../astro/src/actions/runtime/middleware.ts   | 10 ++--
 packages/astro/src/actions/runtime/route.ts   |  3 +-
 packages/astro/src/actions/runtime/utils.ts   |  5 +-
 .../src/actions/runtime/virtual/server.ts     | 37 +++++++------
 packages/astro/templates/actions.mjs          | 54 +++++++++----------
 .../test/fixtures/actions/astro.config.mjs    |  1 +
 .../actions/src/pages/subscribe.astro         |  6 +--
 .../astro/test/types/action-return-type.ts    | 11 +++-
 packages/astro/test/types/is-input-error.ts   |  2 +-
 packages/integrations/react/server.js         | 13 +----
 16 files changed, 105 insertions(+), 83 deletions(-)
 create mode 100644 .changeset/light-chairs-happen.md

diff --git a/.changeset/light-chairs-happen.md b/.changeset/light-chairs-happen.md
new file mode 100644
index 000000000000..486ecd327e6b
--- /dev/null
+++ b/.changeset/light-chairs-happen.md
@@ -0,0 +1,29 @@
+---
+'@astrojs/react': patch
+'astro': patch
+---
+
+**BREAKING CHANGE to the experimental Actions API only.** Install the latest `@astrojs/react` integration as well if you're using React 19 features.
+
+Make `.safe()` the default return value for actions. This means `{ data, error }` will be returned when calling an action directly. If you prefer to get the data while allowing errors to throw, chain the `.orThrow()` modifier.
+
+```ts
+import { actions } from 'astro:actions';
+
+// Before
+const { data, error } = await actions.like.safe();
+// After
+const { data, error } = await actions.like();
+
+// Before
+const newLikes = await actions.like();
+// After
+const newLikes = await actions.like.orThrow();
+```
+
+## Migration
+
+To migrate your existing action calls:
+
+- Remove `.safe` from existing _safe_ action calls
+- Add `.orThrow` to existing _unsafe_ action calls
diff --git a/packages/astro/e2e/fixtures/actions-blog/src/components/Like.tsx b/packages/astro/e2e/fixtures/actions-blog/src/components/Like.tsx
index 7d4e6a53d161..9e39d8f9c7cb 100644
--- a/packages/astro/e2e/fixtures/actions-blog/src/components/Like.tsx
+++ b/packages/astro/e2e/fixtures/actions-blog/src/components/Like.tsx
@@ -11,7 +11,7 @@ export function Like({ postId, initial }: { postId: string; initial: number }) {
 			disabled={pending}
 			onClick={async () => {
 				setPending(true);
-				setLikes(await actions.blog.like({ postId }));
+				setLikes(await actions.blog.like.orThrow({ postId }));
 				setPending(false);
 			}}
 			type="submit"
diff --git a/packages/astro/e2e/fixtures/actions-blog/src/components/PostComment.tsx b/packages/astro/e2e/fixtures/actions-blog/src/components/PostComment.tsx
index f73d152e12bf..b6b6bcea1c29 100644
--- a/packages/astro/e2e/fixtures/actions-blog/src/components/PostComment.tsx
+++ b/packages/astro/e2e/fixtures/actions-blog/src/components/PostComment.tsx
@@ -21,7 +21,7 @@ export function PostComment({
 					e.preventDefault();
 					const form = e.target as HTMLFormElement;
 					const formData = new FormData(form);
-					const { data, error } = await actions.blog.comment.safe(formData);
+					const { data, error } = await actions.blog.comment(formData);
 					if (isInputError(error)) {
 						return setBodyError(error.fields.body?.join(' '));
 					} else if (error) {
diff --git a/packages/astro/e2e/fixtures/actions-react-19/src/actions/index.ts b/packages/astro/e2e/fixtures/actions-react-19/src/actions/index.ts
index 39f9dcf9aab8..cd42207729cf 100644
--- a/packages/astro/e2e/fixtures/actions-react-19/src/actions/index.ts
+++ b/packages/astro/e2e/fixtures/actions-react-19/src/actions/index.ts
@@ -1,5 +1,5 @@
 import { db, Likes, eq, sql } from 'astro:db';
-import { defineAction, getApiContext, z } from 'astro:actions';
+import { defineAction, z, type SafeResult } from 'astro:actions';
 import { experimental_getActionState } from '@astrojs/react/actions';
 
 export const server = {
@@ -28,12 +28,13 @@ export const server = {
 			handler: async ({ postId }, ctx) => {
 				await new Promise((r) => setTimeout(r, 200));
 
-				const state = await experimental_getActionState<number>(ctx);
+				const state = await experimental_getActionState<SafeResult<any, number>>(ctx);
+				const previousLikes = state.data ?? 0;
 
 				const { likes } = await db
 					.update(Likes)
 					.set({
-						likes: state + 1,
+						likes: previousLikes + 1,
 					})
 					.where(eq(Likes.postId, postId))
 					.returning()
diff --git a/packages/astro/e2e/fixtures/actions-react-19/src/components/Like.tsx b/packages/astro/e2e/fixtures/actions-react-19/src/components/Like.tsx
index 0d2dde00916e..652ea935ab1c 100644
--- a/packages/astro/e2e/fixtures/actions-react-19/src/components/Like.tsx
+++ b/packages/astro/e2e/fixtures/actions-react-19/src/components/Like.tsx
@@ -16,13 +16,13 @@ export function Like({ postId, label, likes }: { postId: string; label: string;
 export function LikeWithActionState({ postId, label, likes: initial }: { postId: string; label: string; likes: number }) {
 	const [likes, action] = useActionState(
 		experimental_withState(actions.blog.likeWithActionState),
-		10,
+		{ data: initial },
 	);
 
 	return (
 		<form action={action}>
 			<input type="hidden" name="postId" value={postId} />
-			<Button likes={likes} label={label} />
+			<Button likes={likes.data} label={label} />
 		</form>
 	);
 }
diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts
index 20eb91f63805..f0b286e8515d 100644
--- a/packages/astro/src/@types/astro.ts
+++ b/packages/astro/src/@types/astro.ts
@@ -2845,7 +2845,7 @@ interface AstroSharedContext<
 		TAction extends ActionClient<unknown, TAccept, TInputSchema>,
 	>(
 		action: TAction
-	) => Awaited<ReturnType<TAction['safe']>> | undefined;
+	) => Awaited<ReturnType<TAction>> | undefined;
 	/**
 	 * Route parameters for this request if this is a dynamic route.
 	 */
diff --git a/packages/astro/src/actions/runtime/middleware.ts b/packages/astro/src/actions/runtime/middleware.ts
index 9662f5872b0c..f916691b8a05 100644
--- a/packages/astro/src/actions/runtime/middleware.ts
+++ b/packages/astro/src/actions/runtime/middleware.ts
@@ -8,7 +8,7 @@ import { AstroError } from '../../core/errors/errors.js';
 import { defineMiddleware } from '../../core/middleware/index.js';
 import { ApiContextStorage } from './store.js';
 import { formContentTypes, getAction, hasContentType } from './utils.js';
-import { callSafely, getActionQueryString } from './virtual/shared.js';
+import { getActionQueryString } from './virtual/shared.js';
 
 export type Locals = {
 	_actionsInternal: {
@@ -72,9 +72,7 @@ async function handlePost({
 	if (contentType && hasContentType(contentType, formContentTypes)) {
 		formData = await request.clone().formData();
 	}
-	const actionResult = await ApiContextStorage.run(context, () =>
-		callSafely(() => action(formData))
-	);
+	const actionResult = await ApiContextStorage.run(context, () => action(formData));
 
 	return handleResult({ context, next, actionName, actionResult });
 }
@@ -137,9 +135,7 @@ async function handlePostLegacy({ context, next }: { context: APIContext; next:
 		});
 	}
 
-	const actionResult = await ApiContextStorage.run(context, () =>
-		callSafely(() => action(formData))
-	);
+	const actionResult = await ApiContextStorage.run(context, () => action(formData));
 	return handleResult({ context, next, actionName, actionResult });
 }
 
diff --git a/packages/astro/src/actions/runtime/route.ts b/packages/astro/src/actions/runtime/route.ts
index 463a4ac6eee0..a1e711b186d6 100644
--- a/packages/astro/src/actions/runtime/route.ts
+++ b/packages/astro/src/actions/runtime/route.ts
@@ -1,7 +1,6 @@
 import type { APIRoute } from '../../@types/astro.js';
 import { ApiContextStorage } from './store.js';
 import { formContentTypes, getAction, hasContentType } from './utils.js';
-import { callSafely } from './virtual/shared.js';
 
 export const POST: APIRoute = async (context) => {
 	const { request, url } = context;
@@ -23,7 +22,7 @@ export const POST: APIRoute = async (context) => {
 		// https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/415
 		return new Response(null, { status: 415 });
 	}
-	const result = await ApiContextStorage.run(context, () => callSafely(() => action(args)));
+	const result = await ApiContextStorage.run(context, () => action(args));
 	if (result.error) {
 		return new Response(
 			JSON.stringify({
diff --git a/packages/astro/src/actions/runtime/utils.ts b/packages/astro/src/actions/runtime/utils.ts
index 91f2859d45e9..45f6cdc61f37 100644
--- a/packages/astro/src/actions/runtime/utils.ts
+++ b/packages/astro/src/actions/runtime/utils.ts
@@ -1,3 +1,6 @@
+import type { ZodType } from 'zod';
+import type { ActionAccept, ActionClient } from './virtual/server.js';
+
 export const formContentTypes = ['application/x-www-form-urlencoded', 'multipart/form-data'];
 
 export function hasContentType(contentType: string, expected: string[]) {
@@ -17,7 +20,7 @@ export type MaybePromise<T> = T | Promise<T>;
  */
 export async function getAction(
 	path: string
-): Promise<((param: unknown) => MaybePromise<unknown>) | undefined> {
+): Promise<ActionClient<unknown, ActionAccept, ZodType> | undefined> {
 	const pathKeys = path.replace('/_actions/', '').split('.');
 	// @ts-expect-error virtual module
 	let { server: actionLookup } = await import('astro:internal-actions');
diff --git a/packages/astro/src/actions/runtime/virtual/server.ts b/packages/astro/src/actions/runtime/virtual/server.ts
index 93b3f54e1137..fb6a36c9256a 100644
--- a/packages/astro/src/actions/runtime/virtual/server.ts
+++ b/packages/astro/src/actions/runtime/virtual/server.ts
@@ -28,21 +28,21 @@ export type ActionClient<
 > = TInputSchema extends z.ZodType
 	? ((
 			input: TAccept extends 'form' ? FormData : z.input<TInputSchema>
-		) => Promise<Awaited<TOutput>>) & {
+		) => Promise<
+			SafeResult<
+				z.input<TInputSchema> extends ErrorInferenceObject
+					? z.input<TInputSchema>
+					: ErrorInferenceObject,
+				Awaited<TOutput>
+			>
+		>) & {
 			queryString: string;
-			safe: (
+			orThrow: (
 				input: TAccept extends 'form' ? FormData : z.input<TInputSchema>
-			) => Promise<
-				SafeResult<
-					z.input<TInputSchema> extends ErrorInferenceObject
-						? z.input<TInputSchema>
-						: ErrorInferenceObject,
-					Awaited<TOutput>
-				>
-			>;
+			) => Promise<Awaited<TOutput>>;
 		}
-	: ((input?: any) => Promise<Awaited<TOutput>>) & {
-			safe: (input?: any) => Promise<SafeResult<never, Awaited<TOutput>>>;
+	: (input?: any) => Promise<SafeResult<never, Awaited<TOutput>>> & {
+			orThrow: (input?: any) => Promise<Awaited<TOutput>>;
 		};
 
 export function defineAction<
@@ -66,12 +66,15 @@ export function defineAction<
 			? getFormServerHandler(handler, inputSchema)
 			: getJsonServerHandler(handler, inputSchema);
 
-	Object.assign(serverHandler, {
-		safe: async (unparsedInput: unknown) => {
-			return callSafely(() => serverHandler(unparsedInput));
-		},
+	const safeServerHandler = async (unparsedInput: unknown) => {
+		return callSafely(() => serverHandler(unparsedInput));
+	};
+
+	Object.assign(safeServerHandler, {
+		orThrow: serverHandler,
 	});
-	return serverHandler as ActionClient<TOutput, TAccept, TInputSchema> & string;
+
+	return safeServerHandler as ActionClient<TOutput, TAccept, TInputSchema> & string;
 }
 
 function getFormServerHandler<TOutput, TInputSchema extends ActionInputSchema<'form'>>(
diff --git a/packages/astro/templates/actions.mjs b/packages/astro/templates/actions.mjs
index 1101c136181b..fbd89d75dbf4 100644
--- a/packages/astro/templates/actions.mjs
+++ b/packages/astro/templates/actions.mjs
@@ -7,36 +7,30 @@ function toActionProxy(actionCallback = {}, aggregatedPath = '') {
 				return target[objKey];
 			}
 			const path = aggregatedPath + objKey.toString();
-			const action = (param) => actionHandler(param, path);
+			const action = (param) => callSafely(() => handleActionOrThrow(param, path));
 
-			action.toString = () => getActionQueryString(path);
-			action.queryString = action.toString();
-			action.safe = (input) => {
-				return callSafely(() => action(input));
-			};
-			action.safe.toString = () => action.toString();
+			Object.assign(action, {
+				queryString: getActionQueryString(path),
+				toString: () => action.queryString,
+				// Progressive enhancement info for React.
+				$$FORM_ACTION: function () {
+					return {
+						method: 'POST',
+						// `name` creates a hidden input.
+						// It's unused by Astro, but we can't turn this off.
+						// At least use a name that won't conflict with a user's formData.
+						name: '_astroAction',
+						action: action.toString(),
+					};
+				},
+				// Note: `orThrow` does not have progressive enhancement info.
+				// If you want to throw exceptions,
+				//  you must handle those exceptions with client JS.
+				orThrow: (param) => {
+					return handleActionOrThrow(param, path);
+				},
+			});
 
-			// Add progressive enhancement info for React.
-			action.$$FORM_ACTION = function () {
-				return {
-					method: 'POST',
-					// `name` creates a hidden input.
-					// It's unused by Astro, but we can't turn this off.
-					// At least use a name that won't conflict with a user's formData.
-					name: '_astroAction',
-					action: action.toString(),
-				};
-			};
-			action.safe.$$FORM_ACTION = function () {
-				const data = new FormData();
-				data.set('_astroActionSafe', 'true');
-				return {
-					method: 'POST',
-					name: '_astroAction',
-					action: action.toString(),
-					data,
-				};
-			};
 			// recurse to construct queries for nested object paths
 			// ex. actions.user.admins.auth()
 			return toActionProxy(action, path + '.');
@@ -49,14 +43,14 @@ function toActionProxy(actionCallback = {}, aggregatedPath = '') {
  * @param {string} path Built path to call action by path name.
  * Usage: `actions.[name](param)`.
  */
-async function actionHandler(param, path) {
+async function handleActionOrThrow(param, path) {
 	// When running server-side, import the action and call it.
 	if (import.meta.env.SSR) {
 		const { getAction } = await import('astro/actions/runtime/utils.js');
 		const action = await getAction(path);
 		if (!action) throw new Error(`Action not found: ${path}`);
 
-		return action(param);
+		return action.orThrow(param);
 	}
 
 	// When running client-side, make a fetch request to the action path.
diff --git a/packages/astro/test/fixtures/actions/astro.config.mjs b/packages/astro/test/fixtures/actions/astro.config.mjs
index fc6477578bdf..812c9ac1590d 100644
--- a/packages/astro/test/fixtures/actions/astro.config.mjs
+++ b/packages/astro/test/fixtures/actions/astro.config.mjs
@@ -4,6 +4,7 @@ import { defineConfig } from 'astro/config';
 export default defineConfig({
 	output: 'server',
 	experimental: {
+		rewriting: true,
 		actions: true,
 	},
 });
diff --git a/packages/astro/test/fixtures/actions/src/pages/subscribe.astro b/packages/astro/test/fixtures/actions/src/pages/subscribe.astro
index e0b51e087eb4..a5cadb179741 100644
--- a/packages/astro/test/fixtures/actions/src/pages/subscribe.astro
+++ b/packages/astro/test/fixtures/actions/src/pages/subscribe.astro
@@ -1,11 +1,11 @@
 ---
 import { actions } from 'astro:actions';
 
-const { url, channel } = await actions.subscribeFromServer({
+const res = await actions.subscribeFromServer.orThrow({
 	channel: 'bholmesdev',
 });
 ---
 
-<p data-url>{url}</p>
-<p data-channel>{channel}</p>
+<p data-url>{res.url}</p>
+<p data-channel>{res.channel}</p>
 
diff --git a/packages/astro/test/types/action-return-type.ts b/packages/astro/test/types/action-return-type.ts
index 4ca66eed25d4..4922da553037 100644
--- a/packages/astro/test/types/action-return-type.ts
+++ b/packages/astro/test/types/action-return-type.ts
@@ -1,6 +1,10 @@
 import { describe, it } from 'node:test';
 import { expectTypeOf } from 'expect-type';
-import { type ActionReturnType, defineAction } from '../../dist/actions/runtime/virtual/server.js';
+import {
+	type SafeResult,
+	type ActionReturnType,
+	defineAction,
+} from '../../dist/actions/runtime/virtual/server.js';
 import { z } from '../../zod.mjs';
 
 describe('ActionReturnType', () => {
@@ -13,6 +17,9 @@ describe('ActionReturnType', () => {
 				return { name };
 			},
 		});
-		expectTypeOf<ActionReturnType<typeof action>>().toEqualTypeOf<{ name: string }>();
+		expectTypeOf<ActionReturnType<typeof action>>().toEqualTypeOf<
+			SafeResult<any, { name: string }>
+		>();
+		expectTypeOf<ActionReturnType<typeof action.orThrow>>().toEqualTypeOf<{ name: string }>();
 	});
 });
diff --git a/packages/astro/test/types/is-input-error.ts b/packages/astro/test/types/is-input-error.ts
index 2ab65e3d9e9f..64eaa1b12165 100644
--- a/packages/astro/test/types/is-input-error.ts
+++ b/packages/astro/test/types/is-input-error.ts
@@ -10,7 +10,7 @@ const exampleAction = defineAction({
 	handler: () => {},
 });
 
-const result = await exampleAction.safe({ name: 'Alice' });
+const result = await exampleAction({ name: 'Alice' });
 
 describe('isInputError', () => {
 	it('isInputError narrows unknown error types', async () => {
diff --git a/packages/integrations/react/server.js b/packages/integrations/react/server.js
index 6624a5610f00..16bf6785e28a 100644
--- a/packages/integrations/react/server.js
+++ b/packages/integrations/react/server.js
@@ -1,5 +1,4 @@
 import opts from 'astro:react:opts';
-import { AstroError } from 'astro/errors';
 import React from 'react';
 import ReactDOM from 'react-dom/server';
 import { incrementId } from './context.js';
@@ -151,17 +150,7 @@ async function getFormState({ result }) {
 
 	if (!actionKey || !actionName) return undefined;
 
-	const isUsingSafe = formData.has('_astroActionSafe');
-	if (!isUsingSafe && actionResult.error) {
-		throw new AstroError(
-			`Unhandled error calling action ${actionName.replace(/^\/_actions\//, '')}:\n[${
-				actionResult.error.code
-			}] ${actionResult.error.message}`,
-			'use `.safe()` to handle from your React component.'
-		);
-	}
-
-	return [isUsingSafe ? actionResult : actionResult.data, actionKey, actionName];
+	return [actionResult, actionKey, actionName];
 }
 
 async function renderToPipeableStreamAsync(vnode, options) {