-
-
Notifications
You must be signed in to change notification settings - Fork 11k
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
Adds option to specify character set in responses (with http adapter) #869
Conversation
83406f8
to
f600705
Compare
@nickuraltsev @rubennorte thanks for maintaining this project. Would you mind taking a look at this PR? It's a simple, backwards-compatible improvement for server-side requests - even comes with updated README :D |
lib/adapters/http.js
Outdated
@@ -185,7 +185,7 @@ module.exports = function httpAdapter(config) { | |||
stream.on('end', function handleStreamEnd() { | |||
var responseData = Buffer.concat(responseBuffer); | |||
if (config.responseType !== 'arraybuffer') { | |||
responseData = responseData.toString('utf8'); | |||
responseData = responseData.toString(config.charset || 'utf-8'); |
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.
The default is utf8
(without the dash) but as it's the Node Buffer default value you don't need to set the || 'utf8
, you can just pass the config.charset
.
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.
Changed
README.md
Outdated
@@ -280,6 +280,10 @@ These are the available config options for making requests. Only the `url` is re | |||
// options are 'arraybuffer', 'blob', 'document', 'json', 'text', 'stream' | |||
responseType: 'json', // default | |||
|
|||
// `charset` indicates charset to use for decoding responses | |||
// Note: Ignored for `responseType` of 'stream' or client-side requests | |||
charset: 'utf-8', // default |
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'd use responseEncoding
(adding the response prefix to make it clearer and using encoding to match the Node.js naming).
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.
Changed
Thanks so much @rubennorte - I'll make those changes when I get home. |
Updated |
I think it would be better to extract charset from response header content-type. This way you don't have to manually pass the charset per request. |
I agree, but that can be done in a future PR based on the work I've done
here. No reason we can't support both methods.
…On Thu, Oct 5, 2017, 18:51 Tan Nhu ***@***.***> wrote:
I think it would be better to extract charset from response header
content-type. This way you don't have to manually pass the charset per
request.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#869 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIDeZ8yLgTrdZNJcpc3T1npR5aNmg1fks5spWuXgaJpZM4NKcyk>
.
|
2760755
to
48a7902
Compare
@rubennorte I made those changes in August of last year. Can this be merged yet? |
Hi @rubennorte ! Any progress on this? I have a similar use case. |
@johntron Sorry for the delay! I'm going to merge it now. |
Thanks for the merge! |
The implementation would be better in this way:
It seems that you need to use |
@lepture this PR has already been merged, but feel free to submit another PR. This one served my needs, and I would not like the added dependency for other encodings - I've never worked with an API that returned anything other than UTF-8 or latin-1. |
We have a service that returns responses encoded with the ISO-8859-1 character set, and needed a way to decode these correctly. This change allows us to decode correctly in our server: