Actions: give better guidance when a Response is returned by an Action (

#11828) * feat: add error for invalid handler data * refactor: remove redirect from ctx object * chore: changeset * chore: fix redirect codes * fix: move redirect out of actionApiContext constructor * refactor(test): reuse redirects const * wip: bump timeouts * wip: more bumps
withastro · Aug 26, 2024 · 20d47aa · 20d47aa
1 parent f1df1b3
commit 20d47aa
Show file tree

Hide file tree

Showing 11 changed files with 72 additions and 18 deletions.
diff --git a/.changeset/thirty-bikes-peel.md b/.changeset/thirty-bikes-peel.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Improves error message when invalid data is returned by an Action.
diff --git a/packages/astro/e2e/actions-blog.test.js b/packages/astro/e2e/actions-blog.test.js
@@ -25,7 +25,9 @@ test.describe('Astro Actions - Blog', () => {
 		const likeButton = page.getByLabel('Like');
 		await expect(likeButton, 'like button starts with 10 likes').toContainText('10');
 		await likeButton.click();
-		await expect(likeButton, 'like button should increment likes').toContainText('11');
+		await expect(likeButton, 'like button should increment likes').toContainText('11', {
+			timeout: 6_000,
+		});
 	});
 
 	test('Like action - server-side', async ({ page, astro }) => {
@@ -36,7 +38,9 @@ test.describe('Astro Actions - Blog', () => {
 
 		await expect(likeCount, 'like button starts with 10 likes').toContainText('10');
 		await likeButton.click();
-		await expect(likeCount, 'like button should increment likes').toContainText('11');
+		await expect(likeCount, 'like button should increment likes').toContainText('11', {
+			timeout: 6_000,
+		});
 	});
 
 	test('Comment action - validation error', async ({ page, astro }) => {
@@ -131,6 +135,6 @@ test.describe('Astro Actions - Blog', () => {
 
 		const logoutButton = page.getByTestId('logout-button');
 		await logoutButton.click();
-		await expect(page).toHaveURL(astro.resolveUrl('/blog/'));
+		await expect(page).toHaveURL(astro.resolveUrl('/blog/'), { timeout: 6_000 });
 	});
 });
diff --git a/packages/astro/playwright.config.js b/packages/astro/playwright.config.js
@@ -13,7 +13,7 @@ export default defineConfig({
 		 * Maximum time expect() should wait for the condition to be met.
 		 * For example in `await expect(locator).toHaveText();`
 		 */
-		timeout: 4 * 1000,
+		timeout: 6 * 1000,
 	},
 	/* Fail the build on CI if you accidentally left test in the source code. */
 	forbidOnly: !!process.env.CI,

diff --git a/packages/astro/playwright.firefox.config.js b/packages/astro/playwright.firefox.config.js
@@ -13,7 +13,7 @@ const config = {
 		 * Maximum time expect() should wait for the condition to be met.
 		 * For example in `await expect(locator).toHaveText();`
 		 */
-		timeout: 4 * 1000,
+		timeout: 6 * 1000,
 	},
 	/* Fail the build on CI if you accidentally left test in the source code. */
 	forbidOnly: !!process.env.CI,

diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts
@@ -53,7 +53,10 @@ import type {
 	TransitionBeforeSwapEvent,
 } from '../transitions/events.js';
 import type { DeepPartial, OmitIndexSignature, Simplify } from '../type-utils.js';
-import type { SUPPORTED_MARKDOWN_FILE_EXTENSIONS } from './../core/constants.js';
+import type {
+	SUPPORTED_MARKDOWN_FILE_EXTENSIONS,
+	REDIRECT_STATUS_CODES,
+} from './../core/constants.js';
 
 export type { AstroIntegrationLogger, ToolbarServerHelpers };
 
@@ -2980,7 +2983,7 @@ export interface AstroAdapter {
 	supportedAstroFeatures: AstroFeatureMap;
 }
 
-export type ValidRedirectStatus = 300 | 301 | 302 | 303 | 304 | 307 | 308;
+export type ValidRedirectStatus = (typeof REDIRECT_STATUS_CODES)[number];
 
 // Shared types between `Astro` global and API context object
 interface AstroSharedContext<

diff --git a/packages/astro/src/actions/runtime/utils.ts b/packages/astro/src/actions/runtime/utils.ts
@@ -10,7 +10,10 @@ export function hasContentType(contentType: string, expected: string[]) {
 	return expected.some((t) => type === t);
 }
 
-export type ActionAPIContext = Omit<APIContext, 'getActionResult' | 'callAction' | 'props'>;
+export type ActionAPIContext = Omit<
+	APIContext,
+	'getActionResult' | 'callAction' | 'props' | 'redirect'
+>;
 export type MaybePromise<T> = T | Promise<T>;
 
 /**

diff --git a/packages/astro/src/actions/runtime/virtual/shared.ts b/packages/astro/src/actions/runtime/virtual/shared.ts
@@ -6,6 +6,9 @@ import type {
 	MaybePromise,
 	ActionAPIContext as _ActionAPIContext,
 } from '../utils.js';
+import { REDIRECT_STATUS_CODES } from '../../../core/constants.js';
+import { ActionsReturnedInvalidDataError } from '../../../core/errors/errors-data.js';
+import { AstroError } from '../../../core/errors/errors.js';
 
 export type ActionAPIContext = _ActionAPIContext;
 export const ACTION_QUERY_PARAMS = _ACTION_QUERY_PARAMS;
@@ -237,14 +240,30 @@ export function serializeActionResult(res: SafeResult<any, any>): SerializedActi
 			status: 204,
 		};
 	}
+	let body;
+	try {
+		body = devalueStringify(res.data, {
+			// Add support for URL objects
+			URL: (value) => value instanceof URL && value.href,
+		});
+	} catch (e) {
+		let hint = ActionsReturnedInvalidDataError.hint;
+		if (res.data instanceof Response) {
+			hint = REDIRECT_STATUS_CODES.includes(res.data.status as any)
+				? 'If you need to redirect when the action succeeds, trigger a redirect where the action is called. See the Actions guide for server and client redirect examples: https://docs.astro.build/en/guides/actions.'
+				: 'If you need to return a Response object, try using a server endpoint instead. See https://docs.astro.build/en/guides/endpoints/#server-endpoints-api-routes';
+		}
+		throw new AstroError({
+			...ActionsReturnedInvalidDataError,
+			message: ActionsReturnedInvalidDataError.message(String(e)),
+			hint,
+		});
+	}
 	return {
 		type: 'data',
 		status: 200,
 		contentType: 'application/json+devalue',
-		body: devalueStringify(res.data, {
-			// Add support for URL objects
-			URL: (value) => value instanceof URL && value.href,
-		}),
+		body,
 	};
 }
 

diff --git a/packages/astro/src/core/constants.ts b/packages/astro/src/core/constants.ts
@@ -45,6 +45,11 @@ export const DEFAULT_404_COMPONENT = 'astro-default-404.astro';
  */
 export const DEFAULT_500_COMPONENT = 'astro-default-500.astro';
 
+/**
+ * A response with one of these status codes will create a redirect response.
+ */
+export const REDIRECT_STATUS_CODES = [301, 302, 303, 307, 308, 300, 304] as const;
+
 /**
  * A response with one of these status codes will be rewritten
  * with the result of rendering the respective error page.

diff --git a/packages/astro/src/core/errors/errors-data.ts b/packages/astro/src/core/errors/errors-data.ts
@@ -1680,6 +1680,21 @@ export const ActionsUsedWithForGetError = {
 	hint: 'Actions are experimental. Visit the RFC for usage instructions: https://github.com/withastro/roadmap/blob/actions/proposals/0046-actions.md',
 } satisfies ErrorData;
 
+/**
+ * @docs
+ * @see
+ * - [Actions RFC](https://github.com/withastro/roadmap/blob/actions/proposals/0046-actions.md)
+ * @description
+ * Action handler returned invalid data. Handlers should return serializable data types, and cannot return a Response object.
+ */
+export const ActionsReturnedInvalidDataError = {
+	name: 'ActionsReturnedInvalidDataError',
+	title: 'Action handler returned invalid data.',
+	message: (error: string) =>
+		`Action handler returned invalid data. Handlers should return serializable data types like objects, arrays, strings, and numbers. Parse error: ${error}`,
+	hint: 'See the devalue library for all supported types: https://github.com/rich-harris/devalue',
+} satisfies ErrorData;
+
 /**
  * @docs
  * @see

diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts
@@ -216,8 +216,12 @@ export class RenderContext {
 
 	createAPIContext(props: APIContext['props'], isPrerendered: boolean): APIContext {
 		const context = this.createActionAPIContext();
+		const redirect = (path: string, status = 302) =>
+			new Response(null, { status, headers: { Location: path } });
+
 		return Object.assign(context, {
 			props,
+			redirect,
 			getActionResult: createGetActionResult(context.locals),
 			callAction: createCallAction(context),
 			// Used internally by Actions middleware.
@@ -255,8 +259,6 @@ export class RenderContext {
 		const renderContext = this;
 		const { cookies, params, pipeline, url } = this;
 		const generator = `Astro v${ASTRO_VERSION}`;
-		const redirect = (path: string, status = 302) =>
-			new Response(null, { status, headers: { Location: path } });
 
 		const rewrite = async (reroutePayload: RewritePayload) => {
 			return await this.#executeRewrite(reroutePayload);
@@ -292,7 +294,6 @@ export class RenderContext {
 			get preferredLocaleList() {
 				return renderContext.computePreferredLocaleList();
 			},
-			redirect,
 			rewrite,
 			request: this.request,
 			site: pipeline.site,

diff --git a/packages/astro/test/actions.test.js b/packages/astro/test/actions.test.js
@@ -3,6 +3,7 @@ import { after, before, describe, it } from 'node:test';
 import * as cheerio from 'cheerio';
 import * as devalue from 'devalue';
 import { serializeActionResult } from '../dist/actions/runtime/virtual/shared.js';
+import { REDIRECT_STATUS_CODES } from '../dist/core/constants.js';
 import testAdapter from './test-adapter.js';
 import { loadFixture } from './test-utils.js';
 
@@ -436,8 +437,6 @@ describe('Astro Actions', () => {
 	});
 });
 
-const validRedirectStatuses = new Set([301, 302, 303, 304, 307, 308]);
-
 /**
  * Follow an expected redirect response.
  *
@@ -448,7 +447,7 @@ const validRedirectStatuses = new Set([301, 302, 303, 304, 307, 308]);
 async function followExpectedRedirect(req, app) {
 	const redirect = await app.render(req, { addCookieHeader: true });
 	assert.ok(
-		validRedirectStatuses.has(redirect.status),
+		REDIRECT_STATUS_CODES.includes(redirect.status),
 		`Expected redirect status, got ${redirect.status}`,
 	);