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

Fixing #184 Content type removed if data is false #195

Merged
merged 1 commit into from Jan 18, 2016

Conversation

@vandosant
Copy link
Contributor

commented Jan 18, 2016

Should I run grunt build to bundle a new source?

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

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

mzabriskie added a commit that referenced this pull request Jan 18, 2016

Merge pull request #195 from vandosant/master
Fixing #184 Content type removed if data is false

@mzabriskie mzabriskie merged commit dea613a into axios:master Jan 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@djsmith42

This comment has been minimized.

Copy link

commented Jan 18, 2016

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.

image

Seems like you might want this to be:

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

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

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

This comment has been minimized.

Copy link

commented Jan 18, 2016

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

This comment has been minimized.

Copy link

commented Jan 18, 2016

Oh, and NaN. ;-P

@djsmith42

This comment has been minimized.

Copy link

commented Jan 18, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

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

This comment has been minimized.

Copy link

commented Jan 18, 2016

👍

@vandosant

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2016

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

This comment has been minimized.

Copy link

commented Feb 7, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.