-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
feat(router): introduce server support for form actions #1346
Conversation
✅ Deploy Preview for analog-blog ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for analog-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for analog-app ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for analog-ng-app ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
||
submitted($event: { target: HTMLFormElement } & Event) { | ||
$event.preventDefault(); | ||
const body = new FormData($event.target); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
fetch(this.action() || `/api/_analog/pages${window.location.pathname}`, { | ||
method: $event.target.method, | ||
body, | ||
}).then((res) => { |
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.
question: I'm not against fetch
, but would it be possible to support/use HttpClient? Does the internal implementation even matter? I genuinely don't know.
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.
Its possible to use HttpClient, but I don't think it matters. fetch is more flexible out of the box in terms of determining what to do with the response
const email = body.get('email') as string; | ||
|
||
if (!email) { | ||
return fail(422, { errors: { email: 'Email is required' } }); |
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.
question: If JavaScript is disabled, what happens in this case? Can we set up something like [...not-found].page.ts
but like [...422].page.ts
to handle this?
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 tried handling it manually by adding a format: 'json'
field if the call is made via Js:
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 is still TBD. Ideally, if JS is off, you come back to this page with errors displayed as part of the rendered HTML
whahoo! |
}; | ||
} | ||
|
||
export async function action({ event }: PageServerAction) { |
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.
nit: this is copy-pasta so not needed? If this is for demo purposes, can we change the body to something related to the search page instead?
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 yea, copy-pasta. Will remove
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.
Approved with one small nit
} else { | ||
res.text().then((result) => { | ||
this.onSuccess.emit(result); | ||
this.onSuccess.emit('success'); |
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.
shouldn't it be this.state.emit('success');
?
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.
Yep
PR Checklist
Closes #935
What is the new behavior?
In
.server.ts
files, anaction
function can be defined to be used with forms to progressively enhance forms and build the foundation for handling form actions on the server.A follow-up PR will land to add a form directive to bridge the form to the form server action.
Does this PR introduce a breaking change?
Other information
[optional] What gif best describes this PR or how it makes you feel?