From 20d47aa85a3a0d7ac3390f749715d92de830cf3e Mon Sep 17 00:00:00 2001 From: Ben Holmes Date: Mon, 26 Aug 2024 17:53:51 -0400 Subject: [PATCH] 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 --- .changeset/thirty-bikes-peel.md | 5 ++++ packages/astro/e2e/actions-blog.test.js | 10 ++++--- packages/astro/playwright.config.js | 2 +- packages/astro/playwright.firefox.config.js | 2 +- packages/astro/src/@types/astro.ts | 7 +++-- packages/astro/src/actions/runtime/utils.ts | 5 +++- .../src/actions/runtime/virtual/shared.ts | 27 ++++++++++++++++--- packages/astro/src/core/constants.ts | 5 ++++ packages/astro/src/core/errors/errors-data.ts | 15 +++++++++++ packages/astro/src/core/render-context.ts | 7 ++--- packages/astro/test/actions.test.js | 5 ++-- 11 files changed, 72 insertions(+), 18 deletions(-) create mode 100644 .changeset/thirty-bikes-peel.md diff --git a/.changeset/thirty-bikes-peel.md b/.changeset/thirty-bikes-peel.md new file mode 100644 index 000000000000..5a5aacc7fd3a --- /dev/null +++ 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 index b1b5c33ecf39..1631f7dfece7 100644 --- 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 index 5aacd6d01881..2209a5c27f7c 100644 --- 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 index 537bb4099e23..77c16ed48c7f 100644 --- 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 index 3420954a90e5..0ebbdef01003 100644 --- 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 index 776171aa2ac5..84208c879d60 100644 --- 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; +export type ActionAPIContext = Omit< + APIContext, + 'getActionResult' | 'callAction' | 'props' | 'redirect' +>; export type MaybePromise = T | Promise; /** diff --git a/packages/astro/src/actions/runtime/virtual/shared.ts b/packages/astro/src/actions/runtime/virtual/shared.ts index 64d56d13a9aa..be9f063b9b2c 100644 --- 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): 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 index 8e9f5ac74e0a..274f86797077 100644 --- 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 index ad64adb4af54..1abd7ba1b101 100644 --- 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 index ca6f0e47aca8..e81e1696495b 100644 --- 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 index 806bfad4b237..3c803972cea8 100644 --- 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}`, );