Use multipart POST instead of GET #4

Merged
merged 4 commits into from Jan 30, 2012

Projects

None yet

2 participants

@apeace
Contributor
apeace commented Dec 30, 2011

This will fix jfhbrook/node-sendgrid-web#3

The solution is to use a multipart POST request. SendGrid will not accept any other type of POST request.

However, this is dependent on request/request#145. SendGrid will not accept with content-type of multipart/related, which is currently forced by request.

Tests pass when request is modified to use multipart/form-data

@jfhbrook
Owner

Cool! I'll give Mikeal a day or so to get your changes merged. If not, I can always bundle. ;) Thanks!

@apeace
Contributor
apeace commented Dec 30, 2011

Well, I have not submitted a pull request with Mikeal so I'm not sure how soon that change will come. I'm just...to tired to think of the cleanest way to make that configurable. Maybe over the weekend.

@jfhbrook
Owner

Yeah, sure.

@jfhbrook
Owner
jfhbrook commented Jan 2, 2012

I was reading the request docs this morning, and it looks like you might be able to overwrite the header using the "header" field by-hand.

I'll give this a try today.

@apeace
Contributor
apeace commented Jan 2, 2012

I tried that the other day, but unfortunately it seems that the multipart feature sets content-type after the custom headers are applied

@jfhbrook
Owner
jfhbrook commented Jan 2, 2012

Sounds like a bug to me.

@jfhbrook
Owner
jfhbrook commented Jan 3, 2012

https://github.com/mikeal/request/blob/master/main.js#L267

Probably this line should only set the header if it hasn't been previously set. I don't know what ;boundary='frontier' is about though.

Edit: Probably related to https://github.com/mikeal/request/blob/master/main.js#L283 .

@jfhbrook
Owner
jfhbrook commented Jan 3, 2012

@apeace When you were working with this, did you set the header to multipart/form-data or multipart/form-data;boundary="frontier" ?

@apeace
Contributor
apeace commented Jan 3, 2012

@jesusabdullah I set it to multipart/form-data;boundary="frontier"

I would say that your previous comment is right: the multipart feature should only set the content-type header if it is not already set. However, as you saw, the boundary is also hard coded at another point in the code.

I haven't had time to whip up an elegant fix to that

@jfhbrook
Owner
jfhbrook commented Jan 3, 2012

I haven't had time to whip up an elegant fix to that

That's fine. I have an idea on how to fix this, so I might do the PR myself later today.

@apeace
Contributor
apeace commented Jan 4, 2012

98592be works in conjunction with request/request#146

@apeace
Contributor
apeace commented Jan 24, 2012

Looks like mikeal published a new version!

@jfhbrook
Owner

Awesome! I'll try to get this published today then.

Edit: So much for today. I had laptop promblems most of the afternoon. Tomorrow then! >_<

@jfhbrook jfhbrook merged commit b8867d6 into jfhbrook:master Jan 30, 2012
@jfhbrook
Owner

Cool, merged and published. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment