-
Notifications
You must be signed in to change notification settings - Fork 31
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/use formdata multipart #67
Conversation
|
||
function generateAppendData(params) { | ||
return Array.from(params).reduce((acc, [name, valueSet]) => { | ||
valueSet.forEach((value) => { |
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, would be nicer to use map instead of forEach :)
Array.from(params).reduce((rows, [name, valueSet]) => [
...rows,
Array.from(set).map((value) => {
return line
})
], [])
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.
yup, good catch
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 don't think there's anything major here. I agree with Simon that using map
instead of forEach
is neater.
{ | ||
"fileName": "jpeg-quality-30.jpg", | ||
"name": "file", | ||
"value": "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQEBLAEsAAD/2wBDABsSFBcUERsXFhceHBsgKEIrKCUlKFE6PTBCYFVlZF9VXVtqeJmBanGQc1tdhbWGkJ6jq62rZ4C8ybqmx5moq6T", |
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.
Technically, this wouldn't be a data-uri. It's kind of redundant. But it doesn't really matter and I guess you could send a data-uri in a multipart body.
@@ -53,6 +56,7 @@ function addBoundary(boundary, headers) { | |||
return item | |||
}) | |||
|
|||
headers.delete('Content-Type') // Remove uppercased content-type in case it exists |
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 cannot help but feel that this should be part of some kind of general sanitizing/normalization phase. Feels like we are doing this in other places as well?
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.
You're right, there are some inconsistencies between how the headers are being set (lower-cased or upper-cased). In this case that caused a bug under certain circumstances.
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.
Agree with @allansson , there should be normalization of the incoming headers in the parse stage really. Although I think this can be a future improvement for the sake of getting this one out 👍
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. Was just something I thought about when I saw it. :)
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.
Very nice to finally get rid of emailjs-mime-builder! 🎉
Change how we generate
multipart/form-data
payloads:Fixes #56