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 #912

Merged
merged 17 commits into from
Aug 27, 2024
Merged

Actions #912

merged 17 commits into from
Aug 27, 2024

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented May 2, 2024

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

@bholmesdev bholmesdev mentioned this pull request May 2, 2024
@rishi-raj-jain
Copy link

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

@Adammatthiesen
Copy link

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

@bholmesdev
Copy link
Contributor Author

@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
Copy link

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

@bholmesdev
Copy link
Contributor Author

@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
Copy link
Contributor

ascorbic commented May 4, 2024

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

@bholmesdev
Copy link
Contributor Author

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

proposals/0046-actions.md Outdated Show resolved Hide resolved
proposals/0046-actions.md Outdated Show resolved Hide resolved
@bholmesdev
Copy link
Contributor Author

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

@zadeviggers
Copy link

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
Copy link
Contributor

matthewp commented May 9, 2024

@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
Copy link
Contributor Author

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
Copy link
Contributor

matthewp commented May 9, 2024

@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
Copy link
Contributor Author

bholmesdev commented May 9, 2024

@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
Copy link
Contributor

matthewp commented May 9, 2024

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
Copy link

zadeviggers commented May 9, 2024

@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 */)>

@firxworx
Copy link

firxworx commented Jun 25, 2024

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

bholmesdev commented Jul 3, 2024

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
Copy link

firxworx commented Jul 4, 2024

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
Copy link

firxworx commented Jul 6, 2024

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
Copy link
Contributor Author

bholmesdev commented Jul 9, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

  • action={actions.name} is the new way to handle plain POST requests from forms. Shipped in the latest release. ✅
  • .safe() is now the default for all action calls. To allow exceptions to throw, you can chain orThrow() to your function call. Shipped in the latest release ✅
  • z will now come from the astro:schema module. This will become the single place to import z across Actions and Content Collections. z will stick around on the astro:content module marked deprecated for 5.0 in the coming months. Since Actions is experimental, we can remove from astro:actions today. PR inbound ⌛

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

---
import { actions } from 'astro:actions';

const result = await Astro.callAction(actions.searchPosts, {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would feel more natural to call action.searchPosts() directly on the server too, similar to how it's done on the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this was removed recently due to technical limitations of injecting APIContext. This did work in an earlier version, but it was removed due to environment issues with async local storage. I agree it's a compromise, but we unfortunately don't have alternatives today.

Copy link

@mayank99 mayank99 Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that's a bit unfortunate. Maybe one day!

This is a very minor issue anyway, since the big problem/use-case that actions solve is calling backend code from frontend.

(Feel free to resolve this thread; I can't seem to do it myself)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mayank99 Yeah, that was my thinking. I agree that "just call it" is the dream! There's an avenue using compiler hacks that I dreamt up, but the maintenance burden was hard to justify. Open to any JS heroes that can find a path.

@firxworx
Copy link

firxworx commented Aug 8, 2024

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:

  • {} is a misleading type and is considered a bad practice (the following is valid TypeScript: const x: {} = 'I am a teapot')
  • the intent of server-side actions code is misleading because this action should have no input
  • by using .orThrow({}) on the client-side it actually sends that {}

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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:

  • discriminated union support for form submissions
  • encrypting action data when using <form action={action}>
  • flashing form inputs to preserve state (transition:persist is a documented workaround though)

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

@bholmesdev bholmesdev merged commit b689cd4 into main Aug 27, 2024
@bholmesdev bholmesdev deleted the actions branch August 27, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.