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

Samples not language aware #29739

Closed
wetndusty opened this issue Sep 16, 2019 · 21 comments
Closed

Samples not language aware #29739

wetndusty opened this issue Sep 16, 2019 · 21 comments
Labels
doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem.

Comments

@wetndusty
Copy link

in many cases changing "Hello, World!" to "Привет, мир!" lead to encoding hell

i guess it not good for newcomers :(

here http2 sample

==========================

const http2 = require('http2');
const fs = require('fs');

const server = http2.createSecureServer({
  key: fs.readFileSync('localhost-privkey.pem'),
  cert: fs.readFileSync('localhost-cert.pem')
});
server.on('error', (err) => console.error(err));

server.on('stream', (stream, headers) => {
  // stream is a Duplex
  stream.respond({
    'content-type': 'text/html',
    ':status': 200
  });
  stream.end('<h1>Hello World (Привет, мир!)</h1>');
});

server.listen(8443);

==================

and how it look in browser -> Hello World (Привет, мир!)

:(

@XhmikosR
Copy link
Contributor

It depends on your editor and its settings. If the file is saved as UTF-8, you shouldn't have such problems.

But I agree, this is too much.

@wetndusty
Copy link
Author

my file saved as UTF-8

@XhmikosR
Copy link
Contributor

Wrap your sample code above in code blocks so that it's properly formatted. I cannot try unformatted code.

@wetndusty
Copy link
Author

To generate the certificate and key for this example, run:

openssl req -x509 -newkey rsa:2048 -nodes -sha256 -subj '/CN=localhost' \
  -keyout localhost-privkey.pem -out localhost-cert.pem

@XhmikosR
Copy link
Contributor

What happens if you use 'content-type': 'text/html; charset=utf-8'?

@wetndusty
Copy link
Author

What happens if you use 'content-type': 'text/html; charset=utf-8'?

not work

@XhmikosR
Copy link
Contributor

Maybe there's something else wrong, then.

Let's wait for others to chime in.

@wetndusty
Copy link
Author

wetndusty commented Sep 20, 2019

i recheck 'content-type': 'text/html; charset=utf-8' work as expected :) typo :) was utf8

so i guess sample at page https://nodejs.org/dist/latest-v12.x/docs/api/http2.html under title "Server-side example" should be changed at line with code
'content-type': 'text/html',
with
'content-type': 'text/html; charset=utf-8',

and sample become international friendly :)

@wetndusty
Copy link
Author

google search 'content-type': 'text/html' site:nodejs.org should return zero :)

@wetndusty
Copy link
Author

i tried search utf8 at site nodejs.org - it return many - maybe it not ok?

@XhmikosR
Copy link
Contributor

The thing is that the translators shouldn't have translated this in the first place, and secondly this should have been caught up in review.

But anyway mistakes happen so we are here to fix them, including yourself :)

I'm not sure what's the best solution. The easiest one would be to fix this specific snippet, but for the long term we probably need to standardize this.

/CC @Trott

@Trott
Copy link
Member

Trott commented Sep 22, 2019

@nodejs/i18n Opinions on how best to address this?

@XhmikosR
Copy link
Contributor

@wetndusty where's this snippet exactly on the website?

@wetndusty
Copy link
Author

@XhmikosR
Copy link
Contributor

That's in the core @Trott

@XhmikosR
Copy link
Contributor

Maybe move this issue there.

@Trott
Copy link
Member

Trott commented Sep 26, 2019

Maybe move this issue there.

I believe only the English docs are in core. I think the translations are down through CrowdIn. @nodejs/i18n

@wetndusty
Copy link
Author

believe only the English docs are in core

it still english if you just change 'content-type': 'text/html;' to 'content-type': 'text/html; charset=utf-8'

but if anyone change "Hello, world" to "你好世界" or "Привет, мир!" he or she don`t need to do something else to get working as expected sample

@Trott
Copy link
Member

Trott commented Sep 27, 2019

believe only the English docs are in core

it still english if you just change 'content-type': 'text/html;' to 'content-type': 'text/html; charset=utf-8'

but if anyone change "Hello, world" to "你好世界" or "Привет, мир!" he or she don`t need to do something else to get working as expected sample

Oh! I see now. Yeah, if you just want to change the content-type in the sample code, that's great, and can be done with a pull request to the doc/api/http2.md documentation file in https://github.com/nodejs/node.

@Trott Trott transferred this issue from nodejs/nodejs.org Sep 27, 2019
@jasnell jasnell added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Sep 27, 2019
@addaleax addaleax added http2 Issues or PRs related to the http2 subsystem. and removed http Issues or PRs related to the http subsystem. labels Sep 28, 2019
@lpinca
Copy link
Member

lpinca commented Sep 29, 2019

This is valid for many examples and not just this one https://nodejs.org/dist/latest-v12.x/docs/api/http2.html#http2_server_side_example. In my opinion we should either use Content-Type: text/html; charset=utf-8 for all of them or none.

To be pedantic the examples work as intended and the problem only happens when changing the original data so this is expected. I mean if you are changing the example you should understand your change and what are the consequences that this entails (also update the value of the Content-Type header in this particular case).

@wetndusty
Copy link
Author

it sometime hard for newcomers - change hello world to local language is common practice - so change base code to be newcomers friendly variant is not so bad idea (IMHO)

jasnell added a commit to jasnell/node that referenced this issue Jul 9, 2020
@jasnell jasnell closed this as completed in 99f0404 Jul 9, 2020
MylesBorins pushed a commit that referenced this issue Jul 14, 2020
Fixes: #29739

PR-URL: #34222
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 16, 2020
Fixes: #29739

PR-URL: #34222
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Fixes: #29739

PR-URL: #34222
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Fixes: #29739

PR-URL: #34222
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants