-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 middleware #12373
base: next
Are you sure you want to change the base?
Actions middleware #12373
Conversation
🦋 Changeset detectedLatest commit: 5cba3e7 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
b266772
to
e179f4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is blocked because it contains a minor
changeset. A reviewer will merge this at the next release if approved.
e179f4f
to
1e31f7a
Compare
<body> | ||
<form | ||
method="POST" | ||
action={actions.blog.lotsOfStuff + '&actionCookieForwarding=true'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Triggers cookie forwarding middleware. The rest of this file is a formatting change
status: actionResult?.error ? actionResult?.error.status : status, | ||
statusText: actionResult?.error ? actionResult?.error.type : 'OK', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Status used to be set by middleware by re-wrapping the Response
. Now that users can implement action middleware themselves, I moved that handling here to ensure status codes are always set
The e2e test failure for rewrites is expected. @ematipico this is the middleware call to debug: https://github.com/withastro/astro/pull/12373/files#diff-c72437aa5779663ea970b6c3c63ca75180a64942d8f3a4689ef408f2eb31d765R39-R44 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I spotted a couple of bugs that we should address. Mainly:
- the fact that we bind an action to the incorrect context
- the fact that we don't do a safer check for
_actionPayload
insiderender-context.ts
@@ -311,9 +311,12 @@ export class RenderContext { | |||
(await pipeline.componentMetadata(routeData)) ?? manifest.componentMetadata; | |||
const headers = new Headers({ 'Content-Type': 'text/html' }); | |||
const partial = typeof this.partial === 'boolean' ? this.partial : Boolean(mod.partial); | |||
const actionResult = hasActionPayload(this.locals) | |||
? deserializeActionResult(this.locals._actionPayload.actionResult) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a slight chance that _actionPayload
isn't there, in case locals get cleaned by the user, so we should use a safer approach.
If we're inside an action, and we must have _actionPayload
, then we it's safer to:
- Check if rendering the result of action (
this.routePattern
should be enough) - Throw an error if
_actionPayload
isn't there. I think throwing an error is reasonable because users have wipedlocals
, when they should not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hm, I don't think that's possible for actions submitted by a form. _actionPayload
may be populated after a redirect using cookies or session storage, so we can't rely on the route to determine if _actionPayload
should exist.
It doesn't look like this PR is changing the behavior here either (unless I'm missing something nuanced). I'd vote to address this in a separate PR if there's a solution. Only thought I have is using Object.create()
to set the action payload to avoid accidental deletion, or forward it with a different internal property other than locals
@bholmesdev I fixed the problem with e2e, there was already a rewrite in place, so it's not needed to have one in the middleware too |
Co-authored-by: Emanuele Stoppa <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! I like the compromise of adding getActionContext
.
!preview astro-actions-middleware |
Change Action cookie redirects to an opt-in feature users can implement using middleware. This means Actions submitted from an HTML form action will no longer redirect to the destination as a GET request. Instead, the page will be rendered as a POST result.
This is meant to address the 4 KB size limit users have encountered when calling actions from an HTML form action. It can be difficult to predict the size of an action result for large validation errors or longer return values, like AI chatbot results.
So, we've introduced a new
getActionContext()
utility to let you decide how action results are handled from middleware. You may choose to implement the existing cookie redirect from Astro v4, or implement forwarding with your own session storage.Here is an example of how our built-in POST middleware works. This calls the appropriate action handler and sets the result to be accessible from
Astro.getActionResult()
:See the changeset for a sample implementation of cookie forwarding from Astro v4.
Changes
getActionContext()
utility that exposes utilities to get an action request and set the resultruntime/middleware
andruntime/route
to dogfood this functioncontext.originPathname
property. This is used to retrieve the original pathname through any rewrites. Needed for users to DIY cookie forwarding, and should be useful for middleware authors in general.Testing
Docs
TODO