-
-
Notifications
You must be signed in to change notification settings - Fork 797
Allow MultipartData ActionForms #4278
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
base: main
Are you sure you want to change the base?
Allow MultipartData ActionForms #4278
Conversation
Instead of only allowing `PostUrl`, we take a generic `InputProtocol` argument, which is restricted to either `PostUrl` or `MultiPartFormData`. Also, we don't require `DeserializeOwned` on `FromFormData`, so that we can implement it for server functions.
This currently panics on the server side, which is far from ideal.
All we need to do is derive `Clone` and implement `FromFormData`. The former is trivial. The later however is quite hacky, as it requires us to 1. reference `leptos`, which is not necessarily there when just using `server_fn`, and 2. makes the assumption that the only field is of type `MultipartData`. I think that's a fair assumption though.
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.
Thanks for the PR! Apologies for my delay in reviewing it.
I think this is great. Making multipart data easier to work on is something that comes up occasionally, and this is a good step in that direction. I left some comments, which I hope are helpful.
let struct_name = self.struct_name(); | ||
|
||
quote! { | ||
impl ::leptos::form::FromFormData for #struct_name { |
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'd suggest that this should be handled in a similar way to the server_fn_path
: have a from_form_data
field in the ServerFnCall
struct that takes Option<Path>
, which allows leptos to provide ::leptos::form::FromFormData
, but would allow other crates to provide their own path here. If from_form_data
is None
, omit this implementation.
Self::from(form_data) | ||
} | ||
Self::Server(_) => { | ||
panic!("Cannot clone server side multipart data") |
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 agree with you that this is awkward. We should not implement Clone
on a type that is not actually cloneable by panicking on the server. I think the piece of context I'm missing is — Why does this need to have a Clone
implementation at all? (If you have a MultipartData
and need to clone it you can call .into_client_data().map(|data| data.take())
and clone that instead)
In the chapter about
ActionForm
s, it says:This, however, prevents file uploads from working without JavaScript being enabled. As multi part forms are also supported natively, I feel like it should be possible to allow using them as well.
This is a very naive - but functional - approach that implements support for this. There are currently a few issues though:
Clone for MultipartData
impl on the server side panics. I'm not sure how to best clone the server value. It's not required for this use case, but should obviously be fixed. I think I need some guidance here.server_fn_macro
impl now refers toleptos
, which is probably not desirable, as it is a standalone crate. I don't know the code base well enough to move this implementation somewhere else. How could I best solve this?server_fn/multipart
feature is now enabled unconditionally. The easy solution would be to add another feature toleptos
. But I feel like resolving issue2
will also lead to a nicer solution here.Also, while testing, I found what I believe is a bug. Before specifying the
enctype
on theActionForm
, the server-side code would panic withcouldn't parse boundary
when submitting the form. It seems like theFromReq<MultipartFormData, Request, E>
impl doesn't handle encoding errors at all.Here is some example code for testing: