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

File uploads / content-type: multipart/form-data #25

Open
knubie opened this issue Dec 3, 2023 · 2 comments
Open

File uploads / content-type: multipart/form-data #25

knubie opened this issue Dec 3, 2023 · 2 comments

Comments

@knubie
Copy link

knubie commented Dec 3, 2023

At the moment it doesn't seem possible to send files as multipart/form-data as described here: Using the Fetch API - Web APIs | MDN.

For example with js/fetch we could do something like this:

(-> (js/fetch "https://example.com/profile/avatar"
              #js{:method "POST"
                  :body (doto (js/FormData.) (.append "file" blob file-name))})

and js/fetch will automatically set the Content-Type header to multipart/form-data and encode the body properly.

However in lambdaisland/fetch the body is always encoded according to the :content-type option before it's passed to js/fetch (unless it's a string):

(j/assoc! :body (if (string? body)
body
(encode-body content-type body opts))))]

There isn't really a way to leave the body decoded without passing a string.

I think the simplest workaround would probably be to add a new :raw or nil option for :content-type that just passes the :body through to js/fetch decoded.

It's possible to bypass the encoding today by using :default as the :content-type, but that will also explicitly set the Content-Type header to nil, since it's not in the content-types map and is really just a hack for the encoding multimethod.

(let [fetch-headers #js {"Accept" (c/get content-types accept)
"Content-Type" (c/get content-types content-type)}]

@plexus
Copy link
Member

plexus commented Dec 14, 2023

Thanks @knubie for reporting this. I think there's a few sides to this. Currently we accept a string body as being already encoded, but js/fetch also accepts a FormData, and (I assume?) a Blob, so it makes sense to detect those and not further try to encode them. It doesn't really make sense to encode a FormData or Blob to Transit anyway. That seems like a no-brainer change to me, it's potentially a breaking change, but I don't see how anyone could be meaningfully relying on the current behavior for FormData or Blob as body (or am I missing something? are there cases where this would make sense? perhaps people have set up custom Transit writers/readers for these types, that could happen...)

I'm also not against introducing something like :raw as an escape hatch to opt-out of certain processing behavior. We can't anticipate every use case, and sometimes it's better to just get out of the way and let the user handle things.

I think it also makes sense for people to want to separately decide the encoding, and the content-type header. E.g. serializing as json, but have a more specific content-type like application/x-my-app-data. I guess you can do this by setting both :content-type and :headers {"Content-Type" ...}? not sure, would have to do some testing.

Ideally a high level library like this would relieve people from having to deal with FormData objects directly. POSTing a file/blob seems common enough that we can support it directly. I'm thinking fetch could introduce a :multipart-form-data content-type, allowing one to specify the content-type, content-disposition, and content of each part.

(fetch/post "/foo" {:content-type :multipart-form-data
                    :body [{:content-type "application/json" ;; string or keyword, so e.g. :json or :transit+json also works
                            :content-disposition "my_file.json"
                            :content blob-or-string}]})

Does that all make sense @knubie ? Patches for these things are welcome, as is further comments to make sure what we're planning to do makes sense and doesn't have adverse side effects for existing users.

Thanks!

@knubie
Copy link
Author

knubie commented Dec 22, 2023

@plexus

I think I agree with pretty much everything you've said here. I know that js/fetch accepts a Blob, but I'm not sure how that works or what the use cases are for passing a Blob there, so I don't know what the expectation would be for how this library should handle that.

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

No branches or pull requests

2 participants