Fixing #184 Content type removed if data is false

[vandosant]

Should I run grunt build to bundle a new source?

[mzabriskie]

No need to run build until new version is released. Thanks for the PR!

[djsmith42]

Congrats @vandosant on your first PR. That's awesome!

Question: Do you really want to remove the Content-Type header if requestData === null? I ask because null is a valid JSON'able value just like false.

Seems like you might want this to be:

if (requestData === undefined && key.toLowerCase() === 'content-type') {

[mzabriskie]

I had wondered the same. This feels the most backwards compatible since previously anything falsey would remove the Content-Type header. With current implementation we are now just letting false through which is the minimal change for addressing the issue.

[djsmith42]

The old code would also remove the Content-Type header with a payload of empty string or the number 0. Since you're changing the behavior for those values, why not go ahead and change it for null as well?

[djsmith42]

Oh, and NaN. ;-P

[djsmith42]

By the way, I don't feel strongly about this. You should not make a change just because I made these comments. I won't have my feelings hurt no matter what you choose. :)

[mzabriskie]

You're right. Maybe we should remove the check for null. It's late and I'm trying to do several things at the same time. I'll sleep on it so I can think it through more clearly tomorrow.

[mzabriskie]

After looking at the code history it's been doing if (!data && key.toLowerCase() === 'content-type') { since day one. I had hoped to find an issue or PR that would help me understand why this was introduced. I don't remember why but the comment specifically says "Remove Content-Type if data is undefined".

At this point in the code the request data has already been transformed. So the data is one of either a primitive, FormData, ArrayBuffer, or ArrayBufferView. Which means we aren't worried about trying to JSON.stringify as it's already been handled.

I suspect that this condition is for one of two reasons:

1. Prevent an error when Content-Type is set but request body is empty
2. Prevent an error when Content-Type is set for verbs that don't send content (e.g., GET)

I think we are okay to remove the null check and just handle undefined.

[djsmith42]

👍

[vandosant]

Would hate to replace one bug with another! I was thinking the null check was overkill and might have side effects. Thanks for catching it.

[joshlangner]

... and there are indeed side effects. See: #362