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

Support multipart form data #188

Merged
merged 4 commits into from
Sep 13, 2020
Merged

Conversation

tmattio
Copy link
Collaborator

@tmattio tmattio commented Sep 11, 2020

This adds support to decode multipart/form-data-encoded requests.

The parsing is delegated to https://github.com/cryptosense/multipart-form-data, so nothing too fancy here.

I added an example of a file upload using the provided functions. The example comes in two variants:

  • examples/file_upload/main.exe that provides a nice UX and uses TailwindCSS and AlpineJS
    image

  • examples/file_upload/simple.exe that is much simpler and only uses what's necessary to implement the file upload
    image

It would be nice to have support for file uploads directly in Opium, but I think it should live in Opium_kernel and we don't have a solution to implement IO functions at the moment. At the same time, passing an IO module doesn't make for a very nice API, so in the meantime, people can just copy-paste the example.

Copy link
Owner

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Looks good. However, Lwt_stream is gross and we should avoid/discourage its use. Is it possible to use multipart without it?

@tmattio
Copy link
Collaborator Author

tmattio commented Sep 13, 2020

I initially wanted to use https://github.com/dinosaure/multipart_form, which does not use Lwt_stream, but it's not released on Opam. I'd be happy to use it instead of multipart-form-data if @dinosaure releases it on Opam at some point though.

But I'm curious: what's wrong with Lwt_stream that we should avoid using it?

@tmattio tmattio merged commit 8334656 into rgrinberg:master Sep 13, 2020
@tmattio tmattio deleted the multipart-form branch September 13, 2020 14:13
@dinosaure
Copy link

@tmattio I can do a release of it if you want 👍

@tmattio
Copy link
Collaborator Author

tmattio commented Sep 13, 2020

@dinosaure That would be awesome! I'll update the code to use your implementation when it's on Opam :)

@rgrinberg
Copy link
Owner

what's wrong with Lwt_stream that we should avoid using it?

It has poor semantics

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.

3 participants