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

Fix net module to accept non-string header values #9062

Merged
merged 1 commit into from Mar 30, 2017

Conversation

Projects
None yet
2 participants
@tarruda
Contributor

tarruda commented Mar 30, 2017

This is required to be compatible with node.js http module.

For the record, I found this issue when trying to make the request module to work with electron's net module.

@tarruda tarruda requested a review from kevinsawicki Mar 30, 2017

@@ -255,7 +255,7 @@ class ClientRequest extends EventEmitter {
const key = name.toLowerCase()
this.extraHeaders[key] = value
this.urlRequest.setExtraHeader(name, value)
this.urlRequest.setExtraHeader(name, value.toString())

This comment has been minimized.

@kevinsawicki

kevinsawicki Mar 30, 2017

Contributor

Looking at line 249, we only check against undefined, which means null could get through to here and the error would be a bit confusing, maybe the value === undefined check could be updated to value == null since that is the check we typically use in lots of places. What do you think?

@kevinsawicki

kevinsawicki Mar 30, 2017

Contributor

Looking at line 249, we only check against undefined, which means null could get through to here and the error would be a bit confusing, maybe the value === undefined check could be updated to value == null since that is the check we typically use in lots of places. What do you think?

This comment has been minimized.

@tarruda

tarruda Mar 30, 2017

Contributor

👍

@tarruda

tarruda Mar 30, 2017

Contributor

👍

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Mar 30, 2017

Contributor

Maybe we should update the docs as well to broaden the value type?

https://electron.atom.io/docs/api/client-request/#requestsetheadername-value

Contributor

kevinsawicki commented Mar 30, 2017

Maybe we should update the docs as well to broaden the value type?

https://electron.atom.io/docs/api/client-request/#requestsetheadername-value

Fix net module to accept non-string header values
This is required to be compatible with node.js http module.
@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 30, 2017

Contributor

Maybe we should update the docs as well to broaden the value type?

Done

Contributor

tarruda commented Mar 30, 2017

Maybe we should update the docs as well to broaden the value type?

Done

@kevinsawicki kevinsawicki merged commit f805bbe into master Mar 30, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kevinsawicki kevinsawicki deleted the stringify-headers-in-net-module branch Mar 30, 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Mar 30, 2017

Contributor

Thanks for updating this 👍 ⚡️

Contributor

kevinsawicki commented Mar 30, 2017

Thanks for updating this 👍 ⚡️

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