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

proper UTF-8 handling #125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

proper UTF-8 handling #125

wants to merge 1 commit into from

Conversation

AyrA
Copy link

@AyrA AyrA commented Aug 13, 2015

This code properly handles UTF-8 server responses and prevents double encoding.

This code properly handles UTF-8 server responses and prevents double encoding.
@AyrA AyrA mentioned this pull request Aug 13, 2015
@mscdex
Copy link
Owner

mscdex commented Aug 14, 2015

I still prefer to check and only use utf8 if the server explicitly supports it. A slightly better way to handle the encoding might be to just do source.setEncoding('binary') or source.setEncoding('utf8') depending on the server. That way data events always get properly formed strings.

@sitexw
Copy link

sitexw commented Sep 3, 2015

I agree with you (@mscdex) and @AyrA. But for now, nothing happens ...
I think it would be good to implement the code of @AyrA and add a variable "encoding" the options that allows you to manually set the encoding with a default value of "binary", not to break compatibility with an older application.

It would be really nice if it could be done quickly because for myself, I have to manually edit the package ... Thank you.

@hdjarv
Copy link

hdjarv commented Sep 7, 2015

Hi, I'm really interested in proper UTF-8 support for this great node module. Any news wether this PR (#123 as well?) is going to be merged into master?

@mscdex
Copy link
Owner

mscdex commented Sep 7, 2015

I've decided to go about this slightly differently in ebb0fab, which should be more efficient.

Can you please test the master branch and let me know if that works for you all?

@hdjarv
Copy link

hdjarv commented Sep 8, 2015

I've tested current master (ebb0fab) and it works fine for me.

@jpxd jpxd mentioned this pull request Dec 9, 2015
@AntonBryansky
Copy link

@AyrA
What's about changing in parser lib? There used "buffer += chunk.toString("binary")" construction. May be it'll help.

@bachvtuan
Copy link

Hi everyone. how are thing going now ?
I see this issue isn't fixed for a long time.

@lerit
Copy link

lerit commented Feb 17, 2017

it work for me ,resolve my problem,can you merge it to master?
@mscdex

@actiontwo
Copy link

Yeah, It work with me to, but no one merge to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants