withastro / roadmap

Ideas, suggestions, and formal RFC proposals for the Astro project.
320 stars 30 forks source link

Actions #912

Closed bholmesdev closed 3 months ago

bholmesdev commented 7 months ago

Summary

Astro actions make it easy to define and call backend functions with type-safety.

// src/actions/index.ts
import { defineAction, z } from "astro:actions";

export const server = {
  like: defineAction({
    // accept json
    input: z.object({ postId: z.string() }),
    handler: async ({ postId }, context) => {
      // update likes in db

      return likes;
    },
  }),
};

// src/components/Like.tsx
import { actions } from "astro:actions";
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>
  );
}

Links

rishi-raj-jain commented 7 months ago

Does this require View Transitions to be enabled like for form data processing?

Adammatthiesen commented 7 months ago

I want every bit of this for StudioCMS....

bholmesdev commented 7 months ago

@rishi-raj-jain Not at all! We are targeting client components to show how JS state management could work. But forms are callable from an form element with or without view transitions. Let me know if this section addresses that concern: https://github.com/withastro/roadmap/blob/actions/proposals/0046-actions.md#add-progressive-enhancement-with-fallbacks

jdtjenkins commented 7 months ago

@bholmesdev Will this be available for SSG at all, or is it just Hybrid/SSR?

bholmesdev commented 7 months ago

@jdtjenkins Ah, good thing to note. Since actions allow you define backend handlers, you'll need a server adapter with output: 'hybrid'. You can keep all your existing routes static with that output

ascorbic commented 7 months ago

My turn to bikeshed you. getNameProps is a bit of a confusing name to me. Would getInputProps work better?

bholmesdev commented 7 months ago

@ascorbic That's fair! I agree that's a better name, happy to use it.

bholmesdev commented 6 months ago

@ascorbic Thinking it over, went with getActionProps(). Thought it paired nicely with Astro.getActionResult() to retrieve the return value.

zadeviggers commented 6 months ago

Hey @bholmesdev, it's me back again to make sweeping criticisms about one of your big proposals again 😁 (I'm the one who got you to vendor Zod back when you were working on content collections). Also, sorry for being so late to submit on this - I only just found out about this proposal from the 4.8 release!

Reading through the proposal, I noticed almost every example uses the onSubmit event of the form and then cancels the actual submit event. The use of JavaScript is understandable for many use cases, but it lacks a nice API for submitting actions without JS. This functionality is already very possible using an API route and regular form submissions. It might drive some people to use JS when they don't really need to since there isn't an obvious API for doing form submissions without it. In that light, adding an elegant path to JS-free forms through the actions API would be nice, even if it isn't a main goal.

Here's the single example of how to implement actions without JS:

import { actions, getActionProps } from 'astro:actions';

export function Comment({ postId }: { postId: string }) {
    return (
        <form method="POST" onSubmit={...}>
            <input {...getActionProps(actions.comment)} />
            {/* result:
            <input
                type="hidden"
                name="_astroAction"
                value="/_actions/comment" />
            */}
        </form>
    )
}

It seems like a bit of an afterthought, to be honest, which doesn't really fit with Astro's whole 'Ship less JS' thing, and I hope we can all agree that <input {...getActionProps(actions.comment)} /> looks pretty ugly.

My understanding is that under the hood, Astro sees the _astroAction field and rewrites the request to go to an action handler. If that's the case, why not make this a first-class behaviour? Provide another function, say getActionURL(actions.whatever), that just returns the path to the action handler. Then you can pass that string into the action attribute of a <form> element. This eliminates the jankiness of <input {...getActionProps(actions.comment)} /> while still allowing full client-side JS control when wanted and helps guide people not to use JS when they don't need to.

E.g.

import { actions, getActionURL } from 'astro:actions';

export function Comment({ postId }: { postId: string }) {
    return (
        <form method="POST" action={getActionURL(actions.comment)} onSubmit={...}>
            {/* Regular form goes here */}
        </form>
    )
}

Some people wanted something similar to this that involved directly importing actions with import attributes, but it seems like that wasn't viable. This seems like a nice middle ground, where you still (kinda) pass the handler to the action prop, and it seems much more possible: it's just a simplification of getActionProps(...).

Also, the rest of the proposal looks great. I can't wait to have a play with some Actions!

matthewp commented 6 months ago

@zadeviggers Form support was not an afterthought, it was the biggest part of the design and why the feature didn't ship sooner. An API like the one you're suggesting was considered but it's incomplete. What happens on the server-side after an action is executed? How do you redirect to a different page? How do you handle validation errors? @bholmesdev went through a few different designs to try and make all of those things possible but it came at the cost of making the action code much more complex.

The getActionProps design is nice because the form is submitted to whatever action="" you put in, and you are able to handle redirects / errors, etc all yourself in a normal Astro page. It gives you the freedom to do whatever you want, while keeping the Action API more a pure function-like interface.

That said, this RFC is still open so we can talk about other approaches.

bholmesdev commented 6 months ago

Thanks for the overview @matthewp. Actually, I would not consider the action suggestion incomplete! An alternative may be to pass a ?queryParam from his proposed getActionUrl(). This lets us use the same middleware strategy for retrieving the action result from frontmatter. We could also allow some sort of prefix parameter if you want to reroute the POST to another page.

This isn't perfect though. The main reason for a hidden input over this was challenges with React 19. When using actions in React, they own the action property when you pass a function:

<form action={actions.like}>

But now, how can we add a fallback? React stubs action to an empty field now. I am working with the React core team to see if we can control the server-rendered action to avoid this somehow. To be honest, I prefer your API to getActionProps(), so I would like to see this.

matthewp commented 6 months ago

@bholmesdev I'm not sure I understand. If the action attribute points to the Action then the action needs to be able to do redirects and other things. Where does that code live?

bholmesdev commented 6 months ago

@matthewp Well I should clarify: the action would NOT be a URL. It would be a query param to re-request the current page.

<form action={actions.like.url}>
<!--output: ?_actionName=like-->

This does the same work as passing a hidden _actionName input from my understanding

matthewp commented 6 months ago

Oh ok, I understand now. I still like getActionProps better than this idea. When I see <form action={actions.like.url}> I expect that the request goes directly to the action. That it actually posts to the current page would be unexpected. Also, I might want to direct the request to another page, with getActionProps I can do that by setting action="/other-page", but with this new idea it's not clear at all how to do that.

So I think this trades away flexibility for a subjective aesthetic gain only.

zadeviggers commented 6 months ago

@matthewp To address your flexibility concern, we can add an optional parameter to the function to specify a different path for it to submit to. I don't think this will come up super often, though, since people will probably just do all their handling in the action, and then rewrite to the page they want to render.

@bholmesdev I really like the actions.like.url idea over a whole extra function. To address @matthewp's concern, it could be a function instead: <form action={actions.like.url(/* optional path */)>

mayank99 commented 6 months ago

I think I agree with both Zade and Matthew here. I also like the actions.like(/* optional pathname */) suggestion (works similar to action={undefined} submitting to the current page).

The big advantage I can see over an extra hidden <input> is that developers can and will leave it out if it works without it (on their machines with high-speed internet and JS available). Progressive enhancement should ideally work by default, rather than being "opt-in".

jberglund commented 6 months ago

I've been toying around with actions the past week and I like it a lot, especially now that it works on the server side. ❤️

Speaking of progressive enhancement, would it be possible to return the values of FormData through the action, based on the schema? With a default form submission and no JS to prevent the reload, the values the user entered are lost after submission.

chiubaca commented 6 months ago

Does/will Astro have an opinionated way to re-sync backend data back with the UI after an action has been run?

I'm thinking like how in Next.js Server Actions they provide revalidatePath and revalidateTag functions. Is there an Astro equivalent?

(BTW I love how Astro is shaping up, I'm having a lot of fun tinkering)

Southpaw1496 commented 6 months ago

I've been experimenting with this API and enjoying it, especially the JavaScript-less option that is much cleaner than my old manual implementation. However, at the moment I can't use it with @astrojs/cloudflare, since it uses the node:async-hooks module, which isn't supported by the Cloudflare runtime. The error I receive is:

[ERROR] [vite] x Build failed in 105ms
[commonjs--resolver] [plugin vite:resolve] Cannot bundle Node.js built-in "node:async_hooks" imported from "node_modules/astro/dist/actions/runtime/store.js". Consider disabling ssr.noExternal or remove the built-in dependency.

The Cloudflare runtime does support some Node APIs, but async-hooks isn't one of them. Would it be possible to re-implement whatever functionality this provides without using Node.js, so that Cloudflare users don't miss out on the new API? Or is it something that the Cloudflare adapter can change to be compatible?

chiubaca commented 6 months ago

@Southpaw1496 Its been a bit fiddly but i'm using actions with @astrojs/cloudflare in my test repo here : https://github.com/chiubaca/fullstack-astro-cloudflare

Sounds like you're missing a compatibility flag, check out my wrangler.toml. Hope it helps.

wesleycoder commented 6 months ago

I've tried to access cookies inside the action handler as it seemed to be available on the TS side, but it has no effect over the request:

import { defineAction, z } from 'astro:actions'
import { THEME_CHOICES } from '~/constants'

export const server = {
  toggleDarkMode: defineAction({
    input: z.enum(THEME_CHOICES),
    handler: (input, ctx) => {
      console.log('actions toggleDarkMode', input)
      if (input === 'system') {
        ctx.cookies.delete('theme')
      } else {
        ctx.cookies.set('theme', input)
      }
    },
  }),
}

Can anyone confirm if it's suposed to work or am I misguided?

bholmesdev commented 6 months ago

@wesleycoder that could be a bug! I'll investigate

ernestoalejo commented 5 months ago

Love the feel of simplicity of use so far, but with complex objects the type safety is not completely correct yet.

For example if the handler returns a Date the types indicate it will be correctly received client side, but it is serialized as a string instead because it's JSON. I'm not familiar enough with tRPC source code, but it seems this file is responsible for that mapping.

As part of that issue: would it be possible to configure a custom serializer like superjson? tRPC recommends it to serialize complex objects like the previous Date example and receive the correct values, but it also increases the complexity of the types.

wesleycoder commented 5 months ago

@bholmesdev I was looking again at my code after looking into this new post on Discord about this.

I noticed that passing the path option to the cookie makes this work as expected Updating my previous code it becomes:

import { defineAction, z } from ['astro:actions'](astro:actions)
import { THEME_CHOICES } from '~/constants'

export const server = {
  toggleDarkMode: defineAction({
    input: z.enum(THEME_CHOICES),
    handler: (input, ctx) => {
      console.log('actions toggleDarkMode', input)
      if (input === 'system') {
        ctx.cookies.delete('theme', { path: '/' })
      } else {
        ctx.cookies.set('theme', input { path: '/' })
      }
    },
  }),
}

Notice the same occurs for the cookies.delete method. I believe this could help narrow down the issue for you, it seems related to default options behavior.

firxworx commented 5 months ago

I added some comments/feedback to the previous stage e.g. https://github.com/withastro/roadmap/issues/898#issuecomment-2189826220 and it looks like there are other more recent comments from others there as well after that one was closed.

I'll note here a few highlights

isInputError(..) issues

Importantly isInputError() input types are too narrow and either it or another guard exported alongside it should accept input type unknown to narrow to ActionInputError. An example use-case and discussion explaining the importance of this is at the link.

While more minor I do feel fairly strongly about the misleading name of isInputError and that type guards should be explicitly named what they check vs. something arbitrary (i.e. it should be named isActionInputError(..)) this will benefit new developers learning the actions API and overall readability and maintainability of not just Astro but any apps written using it.

Please stop with the z's

Please stop exporting z from everywhere under the sun in Astro (zod). There should only be one source. It is no more "convenient" especially with modern IDE's its confusing. It led me to believe the z for actions might be special and I was informed it wasn't and that I wasn't the only one asking the question.

When devs use Astro and hit z with autocomplete IDE they're faced with a growing wall of them and its not clear at all what the importance is (or the fact that it isn't.. unless I'm mistaken.. but then that's confusing too).

I get if you want to export the Astro version to help ensure versions of zod are consistent in a codebase, but this should only be done in one place, arguably the original place, and with the original explanation in the docs to go with it.

firxworx commented 5 months ago

Now I read the actual code for isInputError.

export function isInputError<T extends ErrorInferenceObject>(
    error?: ActionError<T>
): error is ActionInputError<T> {
    return error instanceof ActionInputError;
}

That's it! Not the safest of types but I'll take it and it also means its trivial to make it accept a broader type as input.

I can confirm that client-side useMutation() onError callback (react-query) returns true for this check.

  const { isPending, mutateAsync } = useMutation(
    {
      mutationFn: actions.products.update,
      onSuccess: (_data) => {
        rqc.invalidateQueries({ queryKey: KEYS.products.all() })
        onSaveSuccess?.()
      },
      onError: (error) => {
        // the `error` is typed "Error" which is incompatible with `isInputError`
        // confirming that this check returns `true` from client-side code 
        console.log(`there was an error and it is an actioninputerror: ${error instanceof ActionInputError}`)
      },
   },
   queryClient,
  }

Given that the type guard is so trivial I would lean towards nixing the type guard entirely and just documenting the if (error instanceof ActionInputError) {...} because otherwise it obfuscates such a simple thing that's going on and makes for a bigger learning curve.

Its like ErrorInferenceObject which is just an alias for Record<string, any> it just adds detail and complexity and obfuscates ability to reason about types from Intellisense (and similar).

wesleycoder commented 5 months ago

It seems the cookies issue is not restricted to the actions context as a similar post has been made on Discord. Should we open a new issue for that or is is already being handled somewhere I couldn't find?

bholmesdev commented 5 months ago

Thanks for following up @wesleycoder! In hindsight, I should've suggested an issue instead of tracking myself. I knew I would be stepping away from actions for the following few weeks, sorry about that.

Since I said I'd take a look, my goal is to wake up tomorrow, get a cup of coffee, and try to PR a fix.

bholmesdev commented 5 months ago

Haven't missed your comments @firxworx! All valid concerns here. There's some nuance to the API choices I could detail, but I'm not 100% sure on them. I'll take a second look to see what we can improve.

bholmesdev commented 5 months ago

Missed your note on serializing complex objects @ernestoalejo. You're correct that we only allow JSON serializable inputs, though we don't document this in the RFC. We avoided superjson to keep the client-side of actions as lean as possible. Alternatives like devalue or seroval may offer the same utility in a smaller footprint. I agree dates are fairly common to send over the wire. Do you have a use case where this should be configurable by you (i.e. editing serialization options yourself)? Or would a global default that covers Dates be sufficient?

firxworx commented 5 months ago

server.d.ts export does not export the type Handler.

In general I have found that TypeScript libraries that provide a smooth DX and avoid manufacturing headaches/issues generally export pretty much all of their types + interfaces even ones that seem internal or its hard to think of a use-case.

Especially with actions being new, to enable the community to work with them in ways that perhaps weren't foreseen and have low friction to building helpers + utilities without resorting to fake/wrong types with as casts or advanced typescript utilities + trickery to extract required types.

Example use-case: I thought perhaps I'd make a generic wrapper for handlers e.g. withErrorHandling(...) that maintains the type-safety and catches different errors including from my database client and maps them to the desired ActionError to eliminate try/catch repetition and clutter. There are many others I can think of e.g. sanitization, transformations, etc.

I'm not blocked per se but I was surprised to find some of the potentially relevant types aren't exported.

firxworx commented 4 months ago

ActionErrorCode isn't exported either! Another real case of writing a simple helper function is foiled!

I won't keep posting since you got the general spiel already above however this time I have something to add:

Beyond exporting it, I suggest as a general philosophy for the majority of cases when you have a type like type ActionErrorCode = 'BAD_REQUEST' | 'UNAUTHORIZED' | ... that a const assertion (as const) be exported as well:

export const ACTION_ERROR_CODES = ['BAD_REQUEST', 'UNAUTHORIZED', ...] as const

And when you define the type: export type ActionErrorCode = (typeof ACTION_ERROR_CODES )[number]

This opens up worlds of magic for downstream devs who now can write helpers and use the validation libraries of their choice (e.g. zod's enum() accepts const assertions, valibot), etc, and they can make a copy (since its as const) to find/filter/etc. from. When writing a library or framework its a choice that enables rather than restricts.


For fun -- example of what we presumably want to avoid pushing on devs:

import { ActionError } from 'astro:actions'

type ConstructorParameters<T> = T extends new (...args: infer P) => unknown ? P : never
type ActionErrorConstructorParams = ConstructorParameters<typeof ActionError>
type ActionErrorCode = ActionErrorConstructorParams[0]['code']
bholmesdev commented 4 months ago

Good point on utility exports @firxworx. There is a method to a madness: exposing internal utilities means an earlier maintenance burden. We could exposing all of our internals for library authors, but this subjects them to documentation, versioning, and caution with breaking changes. We prefer a more cautious route to only expose utilities when there is a strong user need, like what you've presented here.

Happy to expose an ASTRO_ERROR_CODES const! I think that makes a lot of sense.

Update: PR is up

bholmesdev commented 4 months ago

PR is now up to expand isInputError to accept unknown error types. There is a purpose for the ErrorInferenceObject that we needed to keep in-tact. I've added an in-line comment to explain the nuance if you're curious @firxworx

bholmesdev commented 4 months ago

Thanks for the feedback everyone! I've compiled all the notes from @jdtjenkins @wesleycoder @ernestoalejo @chiubaca @Southpaw1496 @mayank99 @zadeviggers.

Now, we're nearing a stable release. I've put up a few discussions in the #feedback-ideas channel on our discord to discuss final API changes. I'd love for y'all to hop in and share your thoughts. We're discussing:

Join the discord and give us feedback! Excited to shape version one with you all.

bholmesdev commented 4 months ago

Big news! We've reached consensus on those API choices above.

We also introduced a change to remove async local storage from the Actions internals. This means Cloudflare and Stackblitz work out of the box, no build config needed.

Now, we're leaving 2 more weeks as experimental to get final thoughts on these changes. If all goes well, we expect Actions to be baselined as stable in the next minor (4.14.0) two weeks from now.

Before then, I'm opening a call for consensus on this RFC. If you have thoughts, please try the latest release and leave feedback here. If there is no contention, my goal is to close this RFC as accepted next Tuesday.

cc to our active contributors: @jdtjenkins @wesleycoder @ernestoalejo @chiubaca @Southpaw1496 @mayank99 @zadeviggers

firxworx commented 3 months ago

In the current release 4.13.1 the types are broken for using orThrow with actions that do not define an input in their defineAction, i.e. actions that essentially play the role of a "GET" request.

I discovered the issue when upgrading from 4.11.1 and revising code to accommodate .safe() now being the default behaviour and .orThrow() now being the option.

Representative Example

Consider the following example with @tanstack/react-query that used to work fine in 4.11.1:

const x = useQuery({
    queryKey: QK.widgets.all(),
    queryFn: actions.widgets.getAll,
  })

If I revise the query function line to be queryFn: actions.widgets.getAll.orThrow then TypeScript will complain: Property 'orThrow' does not exist on type '((input?: any) => Promise<SafeResult<never....

However... the types lie!

If I revise to the following and force it to compile...

    queryFn: actions.patients.getAll.orThrow,

...the code works: the browser sends the expected request body and I get the expected response back from the server.

Workarounds

Adding @ts-expect-error to the line with .orThrow is no good because this forces the query result type to become unknown and will break the point of using typed Astro actions.

A hacky workaround is adding input to the action definition using z.object({}) as the schema and then calling with .orThrow({}). This is not desirable (and thus this is an issue that should be fixed) because:

Interesting Observations

Funnily enough if add the input: z.object({}) to the action definition and I keep the line actions.patients.getAll.orThrow then TypeScript will agree that "orThrow" exists however this time the result type inferred by React Query is bungled and it thinks the result is wrapped in a Promise (!!).

Use Cases

The use-case where .orThrow() is required is to support React Query: it needs a rejected promise to handle errors correctly.

The use-case for having a "GET"-like action without an input is the desire/experiment to define all CRUD operations of server-side code together and for there to be a consistent RPC-like API on the front-end. The resulting developer experience is similar to ts-rest, hono RPC, etc, etc.


Admittedly without reviewing the current code it makes me wonder if the underlying code is not type safe due to type cheats / escape hatches (as, any, etc.) or else I would expect the type system to have flagged this issue and prevented this issue from being shipped. It might be upstream (even react-query or zod) but off-hand this smells like type cheats to me.

bholmesdev commented 3 months ago

Alright, decided to keep us in experiment for one more release. We snuck in a request change: automatic redirects to avoid the "confirm post resubmission" dialog. You can try it out in the latest patch!

As part of this, I also overhauled the instructions for using Actions with plain HTML forms. We better explain how redirects are handled, and include some advice for success and error banners. You can find this on the RFC or the latest documentation draft:

https://deploy-preview-8980--astro-docs-2.netlify.app/en/guides/actions/#call-actions-from-an-html-form-action

Share your feedback and bug reports! If we're happy with this, we should be good for stable.

bholmesdev commented 3 months ago

Alright y'all, it's happening. Actions is scheduled to ship stable in 4.15 🥳

There are a few pieces we didn't manage to finish, but they're still on the list. Namely:

With that, I'm merging this RFC as accepted. Keep sharing bugs and feedback as always! Thanks everyone.