req icon indicating copy to clipboard operation
req copied to clipboard

Add naive multipart/form-data capabilities to `Req.Steps.encode_body/1`.

Open Doerge opened this issue 2 years ago • 3 comments

Hi,

Thanks for a great package! I have simplified a couple of projects a lot with it! 🎉

This PR adds multipart/form-data encoding for very simple forms. (I ran into an API that only accepts this).

Some notes:

  • Files are not handled.
  • I have a branch here with Stream body. Not sure if that is a good or bad idea in the overall architecture of Req, so I left it out for now. Maybe File streaming could be implemented on top of that?
  • Tesla has a more solid implementation here. Maybe someone wiser than me could cook up a better solution.

Let me know what you think of both this basic multipart/form-data PR, and the Stream branch.

Best,

Doerge avatar Sep 05 '22 17:09 Doerge

Thank you for the PR. Before moving forward with this, I'd like to ask if you'd be interested in proposing API for a complete solution. There's no rush in implementing any of it but I'd like to get an idea to avoid a situation where we paint ourselves into the corner.

Regarding request body streaming, setting body: {:stream, enum}, I'd like to hold off on that unless you think it is essential for multipart.

Thank you!

wojtekmach avatar Sep 06 '22 06:09 wojtekmach

Re. streams, I asked in the Elixir Slack channel and LostKobraKai was kind enough to answer. According to him there is no way to match on streams. Hence {:stream, stream} (or something similar) is necessary.

Doerge avatar Sep 07 '22 13:09 Doerge

Yeah, when we go with request streaming it will most likely be {:stream, enumerable} indeed as that's what Finch uses. :)

wojtekmach avatar Sep 07 '22 18:09 wojtekmach

As mentioned earlier I'm very keen on a more comprehensive proposal before moving forward with any code changes so I'm going to close this for now. Thank you for starting the discussion and implementation, looking forward to continuing it!

wojtekmach avatar Nov 10 '22 10:11 wojtekmach