Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Actions #898

Closed
bholmesdev opened this issue Apr 17, 2024 · 41 comments
Closed

Actions #898

bholmesdev opened this issue Apr 17, 2024 · 41 comments

Comments

@bholmesdev
Copy link
Contributor

bholmesdev commented Apr 17, 2024

Body

Summary

Let's make handling form submissions easy and type-safe both server-side and client-side.

Goals

  • Form actions can accept JSON or FormData payloads.
  • You no longer need boilerplate to safely parse the request body based on the Content-Type.
  • You no longer need boilerplate to retrieve form data values from the request body. The action should be able to enumerate expected input names, and the handler should be able to retrieve these values without a type cast. In other words, no more bangs ! or as string casts as in the example formData.get('expected')! as string.
  • You can call an action using a standard HTML form element with the action property.
  • You can use client JavaScript to call an action using scripts or islands. When doing so, data returned by the action is type-safe without type casting. Note: This should consider form handling in popular component frameworks, like the useActionState() and useFormStatus() hooks in React 19.
  • You can declare actions as an endpoint to call from prerendered / static pages using the hybrid output.

Non-goals

  • A solution to client-side validation. Validation is a major piece to forms with several community libraries to choose from (ex. react-hook-form). Astro may recommend standard HTML attributes for client validation including the required and type properties on an input.
  • A hook to retrieve the return value of an action from Astro frontmatter. Action return values may be accessed from client JavaScript for optimistic updates. Otherwise, we expect actions to persist any state to a database to be retrieved manually from your Astro components.
  • Declaring actions within .astro frontmatter. Frontmatter forms a function closure, which can lead to misuse of variables within an action handler. This challenge is shared by getStaticPaths() and it would be best to avoid repeating this pattern in future APIs.

Background & Motivation

Form submissions are a core building block of the web that Astro has yet to form an opinion on (pun intended).

So far, Astro has been rewarded for waiting on the platform and the Astro community to mature before designing a primitive. By waiting on view transitions, we found a SPA-like routing solution grounded in native APIs. By waiting on libSQL, we found a data storage solution for content sites and apps alike. Now, we've waited on other major frameworks to forge new paths with form actions. This includes Remix actions, SvelteKit actions, React server actions, and more.

At the same time, Astro just launched its database primitive: Astro DB. This is propelling the Astro community from static content to more dynamic use cases:

  • A like button that updates a database counter
  • A comments section that inserts a new comment behind an auth gateway
  • A newsletter form that pings a third party mailing service

To meet our community where it's heading, Astro needs a form submission story.

The problem with existing solutions

Astro presents two solutions to handling form submissions with today's primitives. Though capable, these tools are either too primitive or present unacceptable tradeoffs for common use cases.

JSON API routes

JSON API routes allow developers to handle POST requests and return a JSON response to the client. Astro suggests this approach with a documentation recipe, demonstrating how to create an API route and handle the result from a Preact component.

However, REST endpoints are overly primitive for basic use cases. The developer is left to handle parse errors and API contracts themselves, without type safety on either side. To properly handle all error cases, this grows complex for even the simplest of forms:

REST boilerplate example
// src/api/board/[board]/card/[card].ts
import { db, Card, eq, and, desc } from "astro:db";
import type { APIContext } from "astro";

const POST = async ({params, request}: APIContext) => {
  if (!params.card || !params.board) {
    return new Response(
      JSON.stringify({ success: false, error: "Invalid board or card ID" }),
      { status: 400 }
    );
  }

  if (!request.headers.get("content-type")?.includes("application/x-www-form-urlencoded")) {
    return new Response(
      JSON.stringify({ success: false, error: "Must be a form request" }),
      { status: 400 }
    );
  }

  const body = await request.formData();
  const name = body.get("name");
  const description = body.get("description");
  if (typeof name !== "string" || typeof description !== "string") {
    return new Response(
      JSON.stringify({ success: false, error: "Invalid form data" }),
      { status: 400 }
    );
  }

  // Actual app logic starts here!
  const res = await db
    .update(Card)
    .set({ name, description })
    .where(
      and(eq(Card.boardId, params.board), eq(Card.id, params.card))
    );

  return new Response(
    JSON.stringify({ success: res.rowsAffected > 0 }),
    { status: res.rowsAffected > 0 ? 200 : 404 }
  );
}

The client should also guard against malformed response values. This is accomplished through runtime validation with Zod, or a type cast to the response the client expects. Managing this contract in both places leaves room for types to fall out-of-sync. The manual work of defining and casting types is also added complexity that the Astro docs avoid for beginner use cases.

What's more, there is no guidance to progressively enhance this form. By default, a browser will send the form data to the action field specified on the <form> element, and rerender the page with the action response. This default behavior is important to consider when a user submits a form before client JS has finished parsing, a common concern for poor internet connections.

However, we cannot apply our API route as the action. Since our API route returns JSON, the user would be greeted by a stringified JSON blob rather than the refreshed contents of the page. The developer would need to duplicate this API handler into the page frontmatter to return HTML with the refreshed content. This is added complexity that our docs understandably don't discuss.

View transition forms

View transitions for forms allow developers to handle a submission from Astro frontmatter and re-render the page with a SPA-like refresh.

---
const formData = await Astro.request.formData();
const likes = handleSubmission(formData);
---

<form method="POST">
  <button>Likes {likes}</button>
</form>

This avoids common pitfalls with MPA form submissions, including the "Confirm resubmission?" dialog a user may receive attempting to reload the page. This solution also progressively enhances based on the default form action handler.

However, handling submissions from the page's frontmatter is prohibitive for static sites that cannot afford to server-render every route. It also triggers unnecessary work when client-side update is contained. For example, clicking "Likes" in this example will re-render the blog post and remount all client components without the transition:persist decorator.

Last, the user is left to figure out common needs like optimistic UI updates and loading states. The user can attach event listeners for the view transition lifecycle, though we lack documentation on how to do so from popular client frameworks like React.

@github-project-automation github-project-automation bot moved this to Stage 2: Accepted Proposals, No RFC in Public Roadmap Apr 17, 2024
@Princesseuh
Copy link
Member

Moved this from the Stage 1, posted this right before it moved to Stage 2:

I am not super familiar with the subject at hand (I think in the past 10 years, I've only written a <form> once, and there definitely wasn't any sort of typed actions), but I quite like the named actions, seems pretty neat to use.

One thing I'll just mention is that I saw people float the idea of having two files with the same names but a different extension (like page.astro, page.ts) but I'd approach this with caution because TypeScript might not like it. Much like you cannot have a .ts and .tsx with the same name in the same folder, the same may (I'm not 100% sure) apply to Astro files.

@bholmesdev
Copy link
Contributor Author

bholmesdev commented Apr 17, 2024

@Princesseuh I agree with this! During goal setting, we also found a more fundamental issue following SvelteKit's spec for look-behind files: all pages with a page.ts defined would be server rendered.

A common example: a blog with a like button. Each post in your blog would include the like button, and submitting a "click" would trigger a form action to update your database counter. Using the SvelteKit convention, you would duplicate your [...slug].astro with a [...slug].ts to contain your action definitions:

// src/pages/[...slug].ts

export const actions = {
  like: defineAction(...),
}

However, this now means [...slug] is dynamic and will not be prerendered. This means your getStaticPaths() call must be refactored to a server entrypoint with a cache() header to get the same effect. We could also find a workaround in Astro to prerender GET request only, though this may cause confusion for the developer when using the hybrid output; do I set export const prerender from the [...slug].ts? From the [...slug].astro? Both?

You might think "well, form actions do require server handling. Maybe that refactor is justified." This is fair! However, there are alternative APIs we can explore to keep refactors to a minimum for the developer.

One solution would be to keep action declarations to standalone endpoint files, like a src/pages/like-api.ts in our previous example. A call to this separate endpoint could return a JSON result when called from a client script, or redirect back to the referring page when called from an HTML form. This offers similar benefits to a look-behind file while keeping action handling separate from content rendering.

@bholmesdev
Copy link
Contributor Author

Where should actions live?

I want to outline a few places where actions may be defined:

  1. In the .astro component itself. We explored this early on since it felt the most streamlined. However, we quickly saw the limitations of export-ing from Astro frontmatter. Frontmatter creates a function closure, which can lead to misuse of variables within an action handler. This challenge is shared by getStaticPaths() and it would be best to avoid repeating this pattern in future APIs.
  2. In a TypeScript file of the same name as your page. This follows SvelteKit's convention of "look-behind files" and addresses our frontmatter closure problem. However, using this convention would lock your .astro route into dynamic rendering, which forces refactors onto the developer. See this comment for some more complete thoughts.
  3. In a separate endpoint file using an actions export. This allows similar colocation with routes to (2) while keeping dynamic handlers separate from static content. This solution may let you switch from REST exports like POST to a single actions export containing one or several handlers:
src/pages/blog/api.ts

export const actions = {
  like: defineAction(...),
}

To call these actions from a form or from client code, we would need some convention to infer actions available on a route. We may do this with code generation by route name (magicFetch('/blog/api', 'like')), direct imports with server action bundling, or by using object syntax as described by Arsh's typed-api.

  1. In a global src/actions directory. This ditches pages/ colocation with routes for a new directory name. Content Collections and Astro DB offer some parallels with src/content/ and db/, and tRPC uses a centralized file for action declaration as well. I expect this pattern to feel predictable given the prior art. This would also simplify how actions are called compared to (3). Since there's one place for action declarations, we can expose all actions from a virtual module like astro:actions. However, there are concerns with scalability and colocation to discuss.

Right now, I actually lean towards (4). I admittedly wanted to like (3) in this list, since I value colocation with my pages. However, I quickly found myself centralizing actions to a src/pages/api.ts or some equivalent when I was free to define multiple actions, which wasn't much different from a src/actions/index.ts.

I also realized colocation isn't self-explanatory. We want colocation... with what? After working on Astro Studio, I realized we didn't use colocation with presentational UI in pages/; we organized our API by domain models in our database. Using tRPC, we built nested routers for User, Project, Workspace, and a few other concepts. We fell into a structure like this:

// src/procedures/trpc.ts
app({
  user: userRouter, // from ./users.ts
  github: githubRouter, // from ./github.ts
  project: projectRouter, // from ./projects/.ts
});

// in your app
trpc.user.auth()
trpc.github.createTemplate()
trpc.project.create()

We could create a similar organization with src/actions/ using nested objects for each domain:

actions/
  user.ts
  github.ts
  project.ts
  index.ts
import { userActions } from './user';
import { githubActions } from './github';
import { projectActions } from './project';

export const actions = {
  user: userActions,
  github: githubActions,
  project: projectActions,
}
// in your app
actions.user.auth()
actions.github.createTemplate()
actions.project.create()

This pattern mirrors REST-ful APIs as well, which I hope feels intuitive for existing Astro users. I glossed over specifics on calling these actions from a form or a client script, but I'll detail soon!

@pilcrowonpaper
Copy link

pilcrowonpaper commented Apr 18, 2024

You can send either a JSON object or form data to a form action

I would not recommend this. POST requests with form data are considered "simple requests" and ignored by browsers' same origin policy. Even if Astro implements a CSRF protection by default, some devs are going to disable it to enable cross-origin and cross-site requests and not implement CSRF protection since they assume their actions only accept JSON (which is subjected to SOP).

@bholmesdev
Copy link
Contributor Author

Thanks for sharing that nuance @pilcrowonpaper. I know Astro recently added built-in CSRF protection across requests, but I see what you mean about the payloads having different behavior.

It sounds like we shouldn't implicitly let you pass form data or json to the same action. How would you feel if the action explicitly defined the payload it is willing to accept? Assuming a Zod setup for validation, it may look like the following:

const like = defineAction({
  input: formData(), // accepts form data
  handler: () => {}
});

const comment = defineAction({
  input: z.object(), // accepts json
  handler: () => {}
});

@robertvanhoesel
Copy link

TL;DR: Make forData opt-in by exporting additional utilities or mapping formAction(defineAction()) as a way to parse a schema to a formData compatible format.

How would you feel if the action explicitly defined the payload it is willing to accept? Assuming a Zod setup for validation, it may look like the following:

@bholmesdev I just recently spend some effort using @conform-to/zod's parseWithZod() to allow me to create and validate form schema's in FrontMatter, and I would say it would be amazing if we can define expected server action's payload using Zod while still using a formSubmission. It's definitely not possible to map deeply nested Zod schemas to form input, but you can get really far. @conform-to has support for most field and Zod Types.

Safely parsing form data, and maybe validating it against a schema sounds like a good goal to me.

Here's how I currently use forms in Astro:

// admin/discounts/create.astro
---
import { createForm } from 'lib/forms.ts'
const discountSchema = z.object({
  description: z.string().optional().default(""),
  limit: z.number().int().min(1).default(1),
  code: z.string().optional(),
})

const { data, fields, isValid, errors } = await createForm(discountSchema, Astro.request)
// createForm builds fields with required attributes from schema, checks for `request.method=='POST'
// and parses with Zod to data if valid, or errors otherwise

if(isValid) {
  try { await storeOnServer(data) } catch {}
}
---
<form method="post">
   <label>Code</label>
   <input {...fields.code} /> /* type="text" name="code" */
   <small>{errors.code?.message}</small>

    <label>Limit</label>
   <input {...fields.limit} /> /* type="number" name="limit" min="1" */
      <small>{errors.limit?.message}</small>

   <label>Description</label>
   <textarea {...fields.description} /> /* name="description" */
   <small>{errors.description?.message}</small>

   <button type="submit">Create</button>
</form>

Exported action

The below is very much my interopretation/stab at an API I personally would prefer using, and not so much what I think makes sense from a technical perspective.

I think locating actions inside an actions folder is fine, especially if the resulting endpoints don't necessarily match the names or folder structure. That would allow collocation multiple related actions togethers

// src/actions/cart.ts

export const sizes = ["S", "M", "L", "XL"];
const cartItem = z.object({
	   sku: z.string(),
	   size: z.enum(sizes),
	   amount: z.number()
    })

export addToCart = createAction({
	schema: cartItem,
	handler: (item: z.infer<typeof cartItem>) => addToCart(item)
})

export updateCartItem = createAction({
 	schema: cartItem,
	handler: (item: z.infer<typeof cartItem>) => updateCart(item)
})

// etc

createAction() could export some useful utilities like:

  • url: codegenned endpoint of the action
  • schema: Zod schema
  • form.attrs: form attributes including codegenned url of the action, form method, id
  • form.inputs: record of key → input attributes to build, and additional helpers
  • submit: async function that takes the data as JSON object (zod schema), sends it to the server, and returns the result

Usage in forms

On the consumer side, I would use it like the following, perhaps:

// pages/store/[product].astro
import { addToCart } from '../../actions/cart.ts'

const product = Astro.props
const { form, inputs } = addToCart
---
<form {...form.attrs}> /* action="/_actions/cart/addToCart" method="post" */
   <label>Amount</label>
   <input {...form.inputs.amount.attrs } />
   <select {...form.inputs.size.attrs }>
    {form.inputs.size.options.map(size => <option value={size}>{size}</option>)}
    </select>
   <input {...form.inputs.sku.attrs} type="hidden" value={product.sku} />
   <button type="submit">Add to cart</button>
</form>

If I would like to use the action as a full page form submission instead, maybe an other API could be something like:
await formAction(action, Astro.request)

Which creates the above attributes and a form handler on this specific route. It would submit to the action/call its handler, with validated formData and additionally output errors/success state.

// pages/store/[product].astro
import { addToCart } from '../../actions/cart.ts'
import { formAction } from 'astro:actions'

const product = Astro.props
const { form, inputs, success, errors } = await formAction(addToCart, Astro.request)
---
<form {...form.attrs}> /* action="" method="post" */
	{success && <p>Added to cart</p>}
   <label>Amount</label>
   <input {...form.inputs.amount.attrs } />
   {errors.amount && <small>{error.amount.message}</small>}

   <select {...form.inputs.size.attrs }>
    {form.inputs.size.options.map(size => <option value={size}>{size}</option>)}
    </select>
    {errors.size && <small>{error.size.message}</small>}
    
   <input {...form.inputs.sku.attrs} type="hidden" value={product.sku} />
   <button type="submit">Add to cart</button>
</form>

Usage inside Framework Components

You won't touch the exported form property at all and instead directly use the schema, url etc

// components/addToCartButton.tsx
import { addToCart, sizes } from '../../actions/cart.ts'

export const AddToCartButton = (props) => {
   const [amount, setAmount] = createSignal<z.infer<typeof addToCart.schema>['amount']>(1)
   const [size, setSize] = createSignal<z.infer<typeof addToCart.schema>['size']>(sizes[0])
   const [success, setSuccess] = createSignal(false)
   const [errors, setErrors] = createSignal(null)

   const submit = () => {
      try {
         await someHttpLib.post(addToCart.url, { amount: amount(), size: size(), sku: props.sku })
         setSuccess(true)
      } catch (e) {
         setErrors(e)
      }
   }

   return (<div>
      <NumberField value={amount} onChange={setAmount} />
      <Dropdown options={size} selected={size} onSelect={setSize} />
      <Button onClick={submit} />
   </div>)
}

Could even take it up a notch and let submission and handling of the server action result be an exported function

// components/addToCartButton.tsx
import { addToCart, sizes } from '../../actions/cart.ts'

export const AddToCartButton = (props) => {
   const [amount, setAmount] = createSignal<z.infer<typeof addToCart.schema>['amount']>(1)
   const [size, setSize] = createSignal<z.infer<typeof addToCart.schema>['size']>(sizes[0])
   const [success, setSuccess] = createSignal(false)
   const [errors, setErrors] = createSignal(null)
   
   const submit = () => {
      const { success, errors } = await addToCart.submit({ amount: amount(), size: size(), sku: props.sku })
      setSuccess(success)
      setErrors(errors)
   }

   return (<div>
      <NumberField value={amount} onChange={setAmount} />
      <Dropdown options={size} selected={size} onSelect={setSize} />
      <Button onClick={submit} />
   </div>)
}

@bholmesdev
Copy link
Contributor Author

Thanks for that @robertvanhoesel! Good to see thorough overviews like this. I think we're pretty aligned on how we want form actions to feel. Pulling out a few pieces here:

  • We want to validate form data fields using a Zod (or Zod-like) interface. This ideally feels similar to parsing JSON.
  • Actions should be callable from Astro frontmatter. So far, I've just been considering client components and the form action attribute directly. I see the utility and I think it's doable.
  • Action errors (validator or otherwise) should be surfaced in a standard way. I'm a fan of that first example using a try / catch, since validation errors should be caught by HTML attribute validator or client validation. Keeps the base case simple (just call the action and get your data!) but it's still flexible to catch server errors. I could imagine an isValidationError<typeof action>(e) type guard to get type inference on validation errors.
  • Ideally, we could use your validator to generate matching form input attributes. This piece is a bit tougher, since we want to avoid bundling your handler if you access input attributes on the client. I agree the schema and handler should be part of the same createAction call. Perhaps this could be an Astro component feature exclusively using a directive. Do you have thoughts on this?

@robertvanhoesel
Copy link

I think if actions export enough data/schemas/utilities the very concrete implementation I showed above could be more of a library implementing the actions API rather than something that should be packed by default. Could still be part of the core offering but ultimately it starts becoming prescriptive in how to implement actions.

In the end this is a pure DOM spec implementation to have some client side validation based on an action schema. It makes sense to live as import { createForm } from 'astro-actions-dom' or import { Form } from 'astro:actions

Perhaps this could be an Astro component feature exclusively using a directive. Do you have thoughts on this?

I have not considered a directive. Would I assume you mean something like:

<input action:attributes={action.schema.myProperty} />

I'm not sure if I'm a fan, but perhaps that's because directives are sparsely used in Astro at the moment.

I have considered building it into a component, but my main argument against it is that it would prevent you from using UI Framework components which enhance standard HTML Inputs with validation states or UX. I want to be able tp spread the validation attributes and field name onto a framework component, which has my preference over using some custom <Field> component. Even for pure Dom it feels better to have an non-abstracted <input {....attrs} over a Field where I don't know what to expect will be rendered.

Is the below syntax possible in Astro (maybe it already is!), a

component that can take a function as contents/default slot where the first param is the generated inputs/validation?

---
import { Form } from 'astro:actions'
import { postComment } from '../actions/comments'
---

<Form action={postComment}>{(inputs) => (
   <div>
	  <input {...inputs.name.attrs} />
	  <textarea {inputs.content.attrs} />
    </div>
 )}
</Form>

Beyond this DOM topic:

  1. What would be the expectation that an action? It will return resulting data as JSON or throw an exception? Will the action API format the Response object? If not,

  2. As a framework component, how would I call it and then subsequently handle the error? Is there some contract or 'ClientSafeError' type of error?

  3. I'm not sure if it makes sense that the vanilla implementation of actions take formData vs JSON.

    You can call an action using a standard HTML form element with the action property.

    Parsing formData into a richer object schema is a messy process. I'm not aware of frameworks that prefer sending formData over pure JSON.

  4. Is there any opportunity here to combine Astro ViewTransitions and Partial Page routes?


Partial Page Action / Multiple Forms

My biggest frustration currently with native forms and handling forms in Astro is whenever you start having more than one form/action on a page. I currently make sure components that include a form, and can be on the same page as other form components, will check for some unique <button type="submit" name="scope" value="sometinyform"> combination to be present before handling the formData.

I even have a helper in my createForm for creating that scope on the submit and then also validating the scope value is set whenever a POST request on that route is triggered.

const { fields, submitButton } = await createForm(schema, Astro.request, { scope: 'someIdentifier' })
---
<form>
  ...
  <button {...submitButton} >
 </form>

I was initially excited for (Form) Actions in Astro because I hoped it would offer a fairly native solution to this without the need of client side UI components.

So, that makes me wonder, can we adopt a baseline HTMX feature set for this. Combining Partial Route Components (so they can be routed to) with ViewTransitions.

// pages/partials/like.astro
---
import { likeAction } from '../../actions/like.ts'

export const partial = true;

interface Props {
   count: number
   slug: string
 }

let count = Astro.props.count

if(Astro.request.method==="POST") {
 try {
    count = await likeAction(Astro.request.formData()) // count++
    } catch (e) { .... }
 }
---
<form transition:replace action="partials/like" method="post">
   {count} likes
   <button type="submit"> Like</button>
   <input type="hidden" name="slug" value={Astro.props.slug} />
</form>

So we could do something like the below on routes that have ViewTransitions enabled.

// pages/blogs/[slug].astro
---
import Like from '../../partials/like.astro'

const blog = Astro.props

---
<h1>{blog.title}</h1>

<LikeCount slug={blog.slug} count={blog.likes} />

If not the above, I'm really puzzled as to why to support formData out of the box in actions. In that case astro:actions would be more utilities for registering API routes that return API data, which is a fine feature.

I'm a fan of Astro because I can sprinkle in frameworks and libraries where needed. It would be super amazing if we could have basic self replacing forms out of the box.

@bholmesdev
Copy link
Contributor Author

Appreciate the input @robertvanhoesel. I'm hearing a few pieces in your feedback:

  • We can leave certain features to the community to avoid an overly-prescriptive API. For example, generating input attributes.
  • If we support FormData as an input object alongside JSON, we need a strong reason for it. We've been thinking about forms that can be handled via redirects, like a logout or a checkout form. It would also allow users to add fallback behavior for client forms when a form is submitted before JS mounts (aka progressive enhancement). I'll detail a few examples to see how we feel about this.
  • It would be great if you could selectively re-render parts of the page depending on an action result. This avoids client frameworks and allows a native form without parsing. I would consider this piece as large enough for a separate proposal. We just started a stage 1 proposal on selective pre-rendering / "server islands", which would allow dynamically updating just a Like counter as you described.

@hfournier
Copy link

@bholmesdev your last bullet sounds a bit htmx-ish.

@NuroDev
Copy link

NuroDev commented Apr 25, 2024

If we support FormData as an input object alongside JSON, we need a strong reason for it.

Just to spit ball & throw it out there: I think the majority of the time a JSON payload is all anyone will ever need. The one / main use cases I can think of for when FormData may be needed is for for if your action has a payload that cannot be serialised.

The main example being a user account settings page where the user can upload an avatar / image. The options in this scenario in my mind would be:

  • Base64 encode the image on client & add it to a JSON payload.
    • This will work but is not ideal.
    • Requires client-side cpu work
  • Upload the avatar separately using client-side JS.
    • This takes away from the "this form has all the data" paradigm.
    • Requires either splitting up forms or adding isolated use-case client JS.
  • The form action can support FormData, per user opt-in as suggested previously.

The other main scenario I could possibly wonder when FormData may be needed instead of a JSON payload would be for Dates / BigInts, but that could potentially be worked around by implementing Superjson support.

@bholmesdev
Copy link
Contributor Author

Alright, we've been iterating on form actions with a lot through feedback from core, maintainers, and the general community. It seems the thoughts that resonated most were:

  • Add first-party input validation - One of our goals is to remove the boilerplate for creating endpoints. We know see parsing the input is a key part of this.
  • Avoid bundler magic - There should not be a risk of leaking server code onto the client.
  • Let us access errors, server and client-side - There should be some standard way to catch and handle action errors.
  • if we tackle progressive enhancement, it should feel invisible - Astro devs shouldn't need separate code paths to consider a zero-JS fallback.

I'm excited to share a demo video for a proof-of-concept. We're already reconsidering some ideas noted here, namely removing the need for an enhance flag. Give it a watch! Thanks y'all.

https://www.loom.com/share/81a46fa487b64ee98cb4954680f3646e?sid=717eb237-4dd8-41ce-8612-1274b471c2ca

@bholmesdev
Copy link
Contributor Author

bholmesdev commented Apr 25, 2024

Good feedback @NuroDev! I agree JSON is the best default, and we'll likely lead with that in our documentation examples. Still, the form action property supported in React 19 does show the value of form data payloads. You can control state updates from the client, but get zero-JS progressive enhancement for free. It's a tighter requirement to store all request information in form inputs instead of JS. But for forms fit the mold, it's a great practice to support.

That's also a good callout on file uploads. It sounds like the tRPC team has encoded file blobs to JSON strings with reasonable success, but it's probably not the approach for everyone. At the very least, it would be nice to access the raw request body from a context variable if you need to go deep parsing inputs.

@pilcrowonpaper
Copy link

Wait, how do you implement progressive enhancement with JSON?

Re: input validation - I don't really see the benefit of integrating it into the core of form actions. Why can't it be a wrapper over defineAction() like defineActionWithValidation()?

@NuroDev
Copy link

NuroDev commented Apr 27, 2024

I don't really see the benefit of integrating it into the core of form actions.

As a counter point to this: Astro already offers input validation for stuff like content collections out of the box with defineCollection, so would it not make sense to continue the trend?

Though that did pose me the question: If someone doesn't want any kind of validation, or wishes to use a different validation library (Valibot, Typebox, etc) would the 1st parameter of the handler just return the un-validated request body I assume?

@bholmesdev bholmesdev changed the title Form Actions Actions May 1, 2024
@bholmesdev bholmesdev mentioned this issue May 2, 2024
@bholmesdev
Copy link
Contributor Author

Alright, thanks again for the quality input everyone. We've finally hit a point that we can draft an RFC. It includes complete docs of all expected features, and describes any drawbacks or alternatives we've discussed here. Give it a read when you get the chance. Open to feedback as always.

#912

@mayank99
Copy link

mayank99 commented May 3, 2024

I took a brief look at #912 and it feels a tad overcomplicated and restrictive. I don't want to use zod or anything, I just want to call a function that executes on the server. (This is not to meant to be dismissive users who want zod, but I feel that it shouldn't be required)

The ideal API would allow defining functions in the same astro file as the form:

---
const doStuff = async (formData) => {
  // do stuff with formData
}
---

<form method="POST" action={doStuff}>…</form>

I see that this is listed as a non-goal, which is a bit unfortunate. What is the main problem with this? (i.e. what is meant by "misuse of variables"?) Today we can already handle form submissions in frontmatter, so this kinda feels natural to me? (at least when using SSR)

As a compromise, it would be nice if actions were regular functions and allowed to be exported from anywhere (not just in src/actions/):

// in src/wherever.js
export const doStuff = async (formData) => {
  // do stuff with formData
}

and imported anywhere, probably with import attributes:

---
import { doStuff } from "../wherever.js" with { type: "action" };
---

<form method="POST" action={doStuff}>…</form>

The RFC says the main argument against this syntax is that users might forget to set the import attribute. (I feel like users deserve more credit than that, but anyway…)

Maybe there should be runtime restriction around where the action is invoked from. e.g. <form action> works out-of-the-box in .astro, but anything else could perhaps require using a helper (and throw otherwise)?

Example
export const doStuff = async () => {};
import { doStuff } from "./doStuff" with { type: "action" };
import { invokeAction } from "astro:actions";

const handleFormSubmit = async (e) => {
  e.preventDefault();

  // throws if not used in .astro file
  // doStuff();
  
  // must wrap in invokeAction
  const { stuff } = await invokeAction(doStuff)();
}

<form onSubmit={handleFormSubmit}</form>

Edit: Is it possible to implement it such that it throws if imported without the attribute? I feel like it should be possible using a helper at the place where the action is defined (rather than at the call-site).

import { defineAction } from "astro:actions";
export const doStuff = defineAction(async () => {});
import { doStuff } from "./doStuff";
await doStuff(); // ❌ throws
import { doStuff } from "./doStuff" with { type: "action" };
await doStuff(); // 👍 all good

@bholmesdev
Copy link
Contributor Author

bholmesdev commented May 3, 2024

Hey @mayank99! Appreciate the detailed input here. It sounds like you see a bit too much overhead with defineAction() compared to a straightforward function. A primary motivation for Zod was to simplify parsing formData properties without type casting or validation error tracking. Just curious, how do you normally parse FormData objects in your projects?

I'll also admit defineAction() are a superset of action handlers, this happens to work in our early prototype:

export const server = {
  comment: async (formData: FormData) => {...}
}

Once we have an experimental release, it may be worth trying this alongside defineAction() to see what sticks.

It also seems like you value colocation of actions within your Astro components. We agree, this seems intuitive. However, we had a few qualms with actions inside Astro frontmatter beyond what was listed in the non-goal:

  • Function closures could be an issue. Users may expect variables outside of their action to be accessible within an action. They may also expect to mutate outside variables based on the result of an action. Each come with bundler concerns that are difficult to catch and raise error messages for.
  • It's unclear how to use the result of an action. For instance, say we wanted to show an error banner if doStuff fails. How could we do that?

Your import attributes idea would solve this issue. defineAction() could raise exceptions somehow to catch mis-use. There were just a few issues we were unsure about:

  • Should import attributes be used for transformations like this? It's a stage 3 proposal with differing opinions on how they should be held. They are also not (yet) implemented in Vite. Betting on attributes would be a slight gamble.
  • How do you typically organize your backend handlers in your codebase? With your suggestion, we introduce the need for a defineAction() wrapper and an import attribute at each call-site. This is a bit more syntax than importing directly from a virtual module like astro:actions. Perhaps that complexity is worth it, just want to hear your perspective.

@mayank99
Copy link

mayank99 commented May 3, 2024

@bholmesdev I appreciate you taking the time to dive deeper into my feedback! I enjoy this kind of nuanced discussion 💜

Just curious, how do you normally parse FormData objects in your projects?

Normally I extract the fields like this:

const { postId, title, body } = Object.fromEntries(formData);

and validate them in varying ways:

Manually
if (typeof postId !== 'string') throw "Expected postId to be a string";
// …
Using tiny-invariant
invariant(typeof postId === 'string');
invariant(typeof title === 'string');
invariant(typeof body === 'string');

The neat thing about this is that zod can be very easily added to this step.

const { postId, title, body } = PostSchema.parse(Object.fromEntries(formData));

Users may expect variables outside of their action to be accessible within an action. They may also expect to mutate outside variables based on the result of an action.

Yes, that's part of why I want the colocation in the frontmatter in the first place 😅

I think in RSC, this works sorta as expected. You can define server actions in the same file as a server component, and even use variables from the surrounding scope.

It's unclear how to use the result of an action. For instance, say we wanted to show an error banner if doStuff fails. How could we do that?

The way I normally do this is either directly setting the error variable in frontmatter, or if I'm redirecting to a different page, then I use an ephemeral cookie (I believe this is called "session flashing" in some backend frameworks).

In any case, I think the same approach could be used as the current RFC (i.e. Astro.getActionResult) if something is returned from the action? I don't see how/why it would need to be too different for frontmatter actions.


Should import attributes be used for transformations like this?

I think yes? On the web, there are two cases supported in different browsers: type: "css" and type: "json". In both cases, they take the raw file, and transform it into something that makes more sense in the JS file where it is imported (e.g. a CSSStyleSheet instance and a regular JS object respectively).

It's a stage 3 proposal with differing opinions on how they should be held. They are also not (yet) implemented in Vite. Betting on attributes would be a slight gamble.

This is a great point. It might be worth talking to the maintainers of Node.js and Vite to see how they feel about it.


How do you typically organize your backend handlers in your codebase?

I think people should be free to organize it however they want, rather than being restricted to using the src/actions folder. For instance, I typically like to colocate the actions in the same folder with the component using it.

With your suggestion, we introduce the need for a defineAction() wrapper and an import attribute at each call-site.

My impression was that defineAction() was already required in the current RFC.

The import attribute is indeed more verbose. I wonder if typescript support for import attributes could make it so that auto-import adds the import attribute too. 🤔

This is a bit more syntax than importing directly from a virtual module like astro:actions. Perhaps that complexity is worth it, just want to hear your perspective.

Virtual modules add an extra level of overhead imo, since there is an intermediate layer between where the action is defined vs used. I've already had some intermittent issues with the generated astro:db modules.

With import attributes, we'd be able to get rid of the intermediate layer and import the action directly.

@okikio
Copy link
Member

okikio commented May 5, 2024

Should import attributes be used for transformations like this?

I think yes? On the web, there are two cases supported in different browsers: type: "css" and type: "json". In both cases, they take the raw file, and transform it into something that makes more sense in the JS file where it is imported (e.g. a CSSStyleSheet instance and a regular JS object respectively).

It's a stage 3 proposal with differing opinions on how they should be held. They are also not (yet) implemented in Vite. Betting on attributes would be a slight gamble.

This is a great point. It might be worth talking to the maintainers of Node.js and Vite to see how they feel about it.

@mayank99 Building on your comment on import attributes, you're right that they're not quite standardized yet, another possible, less elegant, but ready solution might be using import query params, e.g. ...?action at least until Vite.js and Node.js are ready for import attributes, IMO predefined folders apply a number of restrictions that aren't really necessary for what actions normally do

Note: I do like the actions/ folder as a convention but not necessarily a rule, instead I think using import query params ...?action would be a more appropriate solution at least until import attributes are standardized.

E.g.

// src/components/Like.tsx
import { actions } from "~/actions/index.ts?action";
import { useState } from "preact/hooks";

export function Like({ postId }: { postId: string }) {
  const [likes, setLikes] = useState(0);
  return (
    <button
      onClick={async () => {
        const newLikes = await actions.like({ postId });
        setLikes(newLikes);
      }}
    >
      {likes} likes
    </button>
  );
}

@bholmesdev
Copy link
Contributor Author

bholmesdev commented May 7, 2024

Thanks for the input @mayank99 and @okikio! Let me speak to a few bits of feedback I'm hearing:

First, it sounds like there's desire to handle validation yourself without an input wrapper. Taking Mayank's example, something like:

handler: (formData) => {
  const { postId, title, body } = PostSchema.parse(Object.fromEntries(formData));
}

I spent some time prototyping this, since I did like the simplified onboarding experience. However, there's a few problems with the approach:

  • What if you want to respect number or checkbox inputs? Object.fromEntries() is no longer sufficient, and you'll likely call .get() with parsing to the appropriate type. Either this or Zod's coerce property, which would be a little awkward for booleans.
  • What about validation errors? One option may be to use safeParse and return the error object from the action. However, this wouldn't set the HTTP status to an error, which is one added piece to remember (and something we'd need to allow configurability for). Astro could also catch thrown Zod errors from Astro's internals to return an exception to the user. However, this loses the type inferencing we can offer you today with an input attribute. By chaining the .safe() property from an action call, you get a type-safe fields property based on your validator:
const result = await actions.myAction.safe(formData);
if (isInputError(result.error)) {
  result.error.fields.postId // autocompletion!
}

In the end, it felt like a first-party input validator offered enough utility to keep. I've also updated the handler's default input type to be a FormData object. This makes validation optional. So if you want to DIY validation, it's still fairly straightforward:

action: defineAction({
  accept: 'form',
  handler: (formData) => {
    formData.get('property')
   }
})

Now, about the astro:actions module...

First, I'm pretty confident we can avoid an intermittent issues with virtual module types using this model (see astro:db). Types are inferred directly from the server object, so you do not need astro dev to be running for types to stay up-to-date. This should hopefully prevent the need for server reloads as types update.

It also seems like colocation with individual components is valuable to y'all. I understand that. Still, I want to offer some thoughts:

First, frontmatter closures are dangerous for reasons not discussed in the above examples. I can't forget a discussion with the Clerk CEO about an authentication issue their users were seeing. They expected the following to gate their action correctly:

function Page() {
  const user = auth();
  function login() {
    "use server";
    if (user.session === 'expected') { /* do something */ }
  }
  return (<></>);
}

Not only did this not check the user object from the action, but it encoded the user as a hidden form input to "resume" that state on the server. This is because components do not rerun when an action is called; only the action does. Unless Astro wants to adopt HTML diffing over client-side calls, we would have the same problem.

Putting closures aside, there is a world using import attributes from a separate file. But there are a few troubles that make this difficult:

  • import attributes are unsupported in Vite or the current stable release of Node.
  • the syntax is somewhat verbose and unfamiliar to the average JS dev. I saw mixed reactions showing the concept to others. Sometimes that's a sign of innovation, other times a sign of friction. I'm still mixed on this.
  • it's unclear how the action would map to an endpoint in your project. By using a centralized actions file, we can create easy-to-debug paths for each action. actions.blog.like() maps to /_actions/blog.like, for instance. This grows complex and harder to debug if we generate urls from, say, file hashes.

I'll also address Okikio's suggestion for a ?flag using Vite bundling. This is supported today, though it's tough to preserve types using the model. When implementing in userland, @bluwy needed to implement a TypeScript plugin to properly regex the flag away. It's tough to justify the maintenance burden, especially if this API is a compromise.

In the end, an actions file is simpler and well-proven from a technical standpoint. We just can't justify a gamble on import assertions. What's more, starting with the simpler path keeps this door open; start with an actions file, move to actions anywhere ™️ if there's enough demand.

Hope this makes sense! Our goal was to entertain every possible direction before landing on a release, so this is all appreciated.

@cbasje
Copy link

cbasje commented May 14, 2024

First of all, I love that you are adding this feature! I use SvelteKit in my job and the Superforms library has been a gamechanger so I am glad that Astro is adding something similar. Fun fact about the above conversation: the Superforms library just had a major rewrite to allow for more custom validation but that is besides my point haha.

I was playing around with the feature on a project that I want to use this. On this project I need to set a config.base because it shares a domain with other projects. However, when I try to use an Action, it gives me an error Action not found because it still tries to call Actions at /_actions/... instead of /base/_actions/.... I don't see a way of adapting the request anywhere and if it is possible without middleware? I am also not sure if this is a good place to report it but I wanted to do it somewhere.

Anyway, thanks for all this great work!

@bholmesdev
Copy link
Contributor Author

Hey @cbasje! Glad you're enjoying the feature. You're right that the base config option should be respected from client code. I can take a look at that.

@ciw1973
Copy link

ciw1973 commented May 14, 2024

Wait, how do you implement progressive enhancement with JSON?

You can't, that's why actions being able to handle FormData is so important.

Accessibility requirements mean that many large organisations and government departments mandate that their public facing (and they strongly recommend that their internal) apps and sites are fully functional without JS.

Try using an SPA with screen reader software, and you'll quickly see why this isn't just about catering for people who've decided for whatever reason to disable JS in their browser.

@third774
Copy link

@matthewp matthewp moved this from Stage 2: Accepted Proposals, No RFC to Stage 3: Accepted Proposals, Has RFC in Public Roadmap May 21, 2024
@robertvanhoesel
Copy link

@bholmesdev Finally got to play around with Actions today.

Excellent job on the implementation and API, error handling and progressive enhancement (I like how getActionResult works a lot). The proxy object definition is really useful as I can mimic different parts of my app and ensure certain middleware is run on matching action routes.

One question I have: I can export an action from a route pages/myRoute.astro like below, and everything works (as I expected). I know this is a discouraged pattern because people may not be aware of the context around the function not being available in the action. But it is a very nice way to collocate my actions. Is the current thinking still the same, or can I safely introduce the below pattern to others?

export const myAction = () => defineAction({ ... }

@bholmesdev
Copy link
Contributor Author

Glad you like how actions are working @robertvanhoesel! And uh... I'm very surprised this works actually. Can you give a more complete example? 😅

@florian-lefebvre
Copy link
Member

Stage 3 RFC available in #912

@robertvanhoesel
Copy link

Glad you like how actions are working @robertvanhoesel! And uh... I'm very surprised this works actually. Can you give a more complete example? 😅

@bholmesdev See below, maybe it works because the action is imported in the <script> element vs the frontmatter? Would you say it's better to avoid this pattern?

pages/users.astro

---
import Button from "@components/ui/Button.astro";
import { UserPermissions, formatRole, userHasPermission } from "@services/users";
import { ActionError, defineAction, z } from "astro:actions";
import { Trash, UserPlus, Users } from "lucide-static";

const { api, session } = Astro.locals;
const users = await api.listUsers();
const canManageTeam = await userHasPermission(Astro, UserPermissions.manage.team);

export const removeUserAction = () =>
  defineAction({
    input: z.object({
      orgId: z.number(),
      userId: z.number(),
    }),
    handler: async ({ orgId, userId }, ctx) => {
      if (userId === ctx.locals.session.user?.id)
        throw new ActionError({ message: "You cannot remove yourself from the organization", code: "BAD_REQUEST" });
      if (!(await userHasPermission(ctx, UserPermissions.manage.team)))
        throw new ActionError({ message: "You do not have permission to perform this action", code: "UNAUTHORIZED" });
      // .... api call
      return true;
    },
  });
---
<table>
  {
    users.map(async ({ user, role }) => (
      <tr>
        <td>{user.name}</td>
       
        {canManageTeam && (
          <td>
            {user.id != session.user?.id && (
              <Button
                size="sm"
                isDanger
                data-remove-user={user.id}
                data-org={game.organization_id}
                icon={Trash}
              />
            )}
          </td>
        )}
      </tr>
    ))
  }
</table>

<script>
  import { actions } from "astro:actions";
  document.addEventListener("astro:page-load", () => {
    document.querySelectorAll<HTMLButtonElement>("[data-remove-user]").forEach((button) => {
      button.addEventListener("click", async () => {
        const remove = window.confirm("Are you sure you want to remove this user?");
        if (!remove) return;
        const result = await actions.manage.removeUserfromOrg.safe({
          userId: +button.dataset.removeUser!,
          orgId: +button.dataset.org!,
        });
        if (result.data) button.closest("tr, .table-row")?.remove();
        else window.alert(result.error?.message);
      });
    });
  });
</script>

actions/index.ts

import { removeUserAction } from "src/pages/manage/[gameSlug]/config/users/index.astro";

export const server = {
  // every actiong listed under 'manage' will be prefixed with '/_actions/manage.' and ensures the middleware matches the route and authenticates the user
  manage: {
    removeUserfromOrg: removeUserAction()
  }
};

@bholmesdev
Copy link
Contributor Author

@robertvanhoesel Ah ha, I didn't realize you were importing the action back into your index.ts file. Yes, I'd expect that to work. Exporting from Astro components can cause bundling issues in productions builds from my experience, so tread carefully. I'll admit that pattern isn't one we're unit testing.

I'll take that as a request for colocation.

@arihantverma
Copy link

@bholmesdev if input has a zod union schema, handler doesn't convert FormData in JavaScript object, but instead passes through the FormData as is. Is this intended?

Repro: https://github.com/arihantverma/astro-actions-zod-union-repro

@bholmesdev
Copy link
Contributor Author

@arihantverma ah ha, I don't think we have handling for union types in our inference code. It's more complex to traverse a Zod union to match FormData fields correctly, so we haven't implemented yet. Can you share your use case?

@wiredprairie
Copy link

Regarding non-JSX/React development (with Svelte 5RC in this case)...

I'd like to be able to expose a property on the Svelte component that represents the return types for the action.

type ActionTypeHere = /* ? */
let {
	form
}: {
	form: ActionTypeHere
} = $props()

While this code works:

type ActionTypeHere = Awaited<ReturnType<typeof actions.subscribe>>

I wonder if there's a less verbose way of doing the same thing that has been provided or could be provided?

@arihantverma
Copy link

@bholmesdev

ah ha, I don't think we have handling for union types in our inference code. It's more complex to traverse a Zod union to match FormData fields correctly, so we haven't implemented yet. Can you share your use case?

Ah okay, got it. Amm… so I was playing around with this use case:

  • a user selects multiple images through <input type="file" />
  • adds alt and optionally caption for each of those images through a custom list + form ( for each image ) component
  • clicks an upload button, which sends all the images along with their meta data information in Form Data, one image [action call] at a time.

If the image is big, I break it in chunks. If the image is big and there are > 1 number of chunks, I don't want to send the caption and alt data again. So for a big image, first action call sends first image chunk, caption and alt, the subsequent action calls send only remaining chunks without alt and caption. So I was trying out a union for the types

// first chunk
type FirstChunk = {
   type: "first-chunk"
   image: File,
   alt: string
   caption?: string
}

type RestChunk = {
   type: "rest-chunk"
  image: File
}

type ActionFormData = FirstChunk | RestChunk

Since zod union didn't convert FormData in JavaScript object, I ended up writing two actions and handling the common handling in a reusable function easily.

Unless I'm missing something, is this a valid use case?

@bholmesdev
Copy link
Contributor Author

Great point @wiredprairie. I bet we could expose a utility type for this, like: Infer<typeof subscribe>

@bholmesdev
Copy link
Contributor Author

Got it @arihantverma. That use case definitely makes sense! I agree creating two actions would be the safest workaround.

To explain a bit how our internals work: we crawl through your Zod schema to decide how the incoming formData should be parsed. By assuming this schema is always an object, we can safely preprocess your input before passing to Zod. If we introduce a union type, it's much harder to do this preprocessing; we don't know which side of the union we should take!

That said, I bet we could support Zod's discriminatedUnion property. This switches which side of a union to take based on a value. In your case, you have a discriminated union based on the type parameter. Marking that as lower priority since a workaround exists, but would z.discriminatedUnion solve your problem?

@arihantverma
Copy link

@bholmesdev oh yes discriminatedUnion property would solve it. Thank you!

@firxworx
Copy link

firxworx commented Jun 24, 2024

I have been playing with actions a bit and I appreciate that it can replace the need to have a separate Express/Fastify/Hono/etc API with Astro.

I get that in general server actions have been conceived as being mutation focused...

However in terms of code organization, isn't it nice to have all your CRUD actions (i.e. including "read") maintained in one place as would be the case of a typical/classic backend from Express to Fastify to Hono?

Is "read"/"get" a use-case that has been considered in any detail? The typing doesn't seem as nice if one is using actions to fetch data and the interface both with and without .safe() feels a bit clunky with react-query.

I would love to view astro actions as the potential replacement for a separate backend API.

@firxworx
Copy link

firxworx commented Jun 24, 2024

In terms of influence / inspiration for some of the RPC like features I would like to share the lesser known ts-rest which is an RPC-like setup over JSON-REST (using JSON/REST makes API's compatible everywhere not some stack-specific protocol which is important for business and may use-cases).

See https://ts-rest.com/docs/intro or a real example from my repo https://github.com/firxworx/ts-rest-workspace.

There are a few comparisons to TRPC in the various discussions on actions but I'm not sure this is always the best source of inspiration because of how completely "proprietary" the protocol can be at times.

One part that I'd like to tickle y'all brains with is that 'contract' concept (between client and server) is entrenched as a central concept and how its used to "guarantee" (thanks to zod validations) how requests + responses are typed. It also provides really nice client DX with an RPC-like client including one compatible with react-query.

The availability of clients, the option for custom clients, plus well-documented ability to go without anything and use curl or postman or write your own fetch calls from scratch provides a lot of options for developers.

I view actions in a similar sort of light where its like a contract definition of "I get this input and will only respond this or that output" and its really cool how it even includes the possible error responses and statuses in that contract.

@firxworx
Copy link

A quick piece of feedback: I don't think astro:actions should also export yet another z zod.

I found it confusing and interpreted that as "this is a special z that only applies to actions" and when I asked on Discord I was told "no its just like the others". I was also told I'm not the first to have the same question.

I get it if Astro exports its own z so devs can be sure the versions of zod they're using line up, but exporting it from a bunch of places is messy and confusing imho. In all cases the devs are using Astro (and likely IDE with auto imports etc.) so they end up faced with a wall of z's and if you're not an Astro maintainer you have no idea what that means.

@firxworx
Copy link

@bholmesdev sorry not sure where to put these, tagging so at least someone who knows sees them.

New piece of feedback after another evening playing with actions:

The isInputError(..) type guard input types are too restrictive and limits use to a specific happy path.

Proposed change/fix: isInputError(..) (or another new type guard that is exported alongside it) should accept input type unknown because try/catch blocks return unknown in JS/TS as a limitation of the language.

Example Use-Case

An example use-case which we already see in the wild is using react-query e.g. useMutation() hook calling a back-end action from a client-side component e.g. actions.products.update.

By not choosing actions.products.update.safe we want react-query to receive and handle an error per its query client configuration (as this can include logic for logging, telemetry, retries, etc.).

In the useMutation() options (type UseMutationOptions<...>) one can specify an onError() callback which is useful for handling in an app and providing user feedback e.g. a toast/snack message, and for updating form fields with any errors from back-end validation.

The onError callback receives an error of type Error which is logical for JS/TS in general but is now incompatible with the types accepted by isInputError() which currently only accepts ActionError.

Discussion

This is one specific use-case example.

It is reasonable to assume that a dev may use a try/catch block with actions and as a result all errors received by the catch block will necessarily be of type unknown.

Therefore any widely applicable / generally useful type guard should accept input type unknown to then narrow the type.

If you want to keep the typing of error is ActionInputError<T> the guard can still be generic but set a default so it can be used in both type known and unknown case.

Minor Point

The name of the isInputError() type guard is misleading and I do not think it is helpful to new developers learning astro:actions.

For maintainability and devX type guards should be consistently named for what they check e.g. isRecord(..) rather than something arbitrary vs. the type being guarded.

Therefore the type guard should be named isActionInputError(..) because it enforces error is ActionInputError<T>.

Perhaps more minor I would also suggest ditching the ErrorInferenceObject type in the code which is Record<string, any> (perhaps better Record<string, unknown>) and use that type directly instead of an alias that implies its something more substantial or meaningful than it is.

@florian-lefebvre florian-lefebvre moved this from Stage 3: Accepted Proposals, Has RFC to Implemented in Public Roadmap Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Implemented
Development

No branches or pull requests