doc: add note about browsers and HTTP/2 by styfle · Pull Request #19476 · nodejs/node · GitHub

The docs were not clear for new users of HTTP/2 requires TLS in most browsers.
This adds a couple notes to direct new users to http2.createSecureServer().

Fixes #19406

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

/cc @apapirovski @a0viedo @mcollina @trivikr

I'm not sure the best way to word this or how to make it more explicit.
But at least if the user copy/paste code snippets now they will get the comments too.

A nit: // Try -> // try after the comma above?

Any thoughts on usage of periods . anywhere?

Let's see what others think, I am not a native speaker)

I added periods.

No browsers support unencrypted HTTP2 (HTTP/2 spec FAQ)
You can change this to "since no browsers support unencrypted HTTP2" with the link to HTTP/2 spec FAQ

Ditto for line 1735

I think the term plain-text is more specific.

How about using the term unencrypted over plain-text?

I'd be good with that.

Sounds good to me 👍

can you restore this to plain-text?

I changed to unencrypted

Thanks for the comments. I pushed an update with the revised docs.

A nit: we usually sort references in ASCII order, so this should go after [HTTP2 Settings Object] ref.

I fixed this by renaming to [HTTP/2 Unencrypted] since its not linking to a section anchor but its more like the external link above.

A nit: missing line break at the end of the file.

Oops! Fixed! 🔧

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/280/

LGTM

I would say something like...

Since there are no browsers known that support [unencrypted HTTP/2](HTTP/2 Unencrypted),
the use of [`http2.createSecureServer()`][] is necessary when communicating with browser
clients.

Using Unencrypted HTTP/2 would actually be the preference when running purely within internal environments with non-browser clients.

Okie dokie 👍

Again,

Since there are no browsers known that support [unencrypted HTTP/2](HTTP/2 Unencrypted),
the use of [`http2.createSecureServer()`][] is necessary when communicating with browser
clients.

Okie dokie 👍

LGTM

(HTTP/2 Unencrypted) -> [HTTP/2 Unencrypted] (otherwise it is not rendered as a link).
This and next line exceed 80 character length, so this may be a problem for doc linting

Whoa good catch, I just copy/pasted the code review comment so there must have been a typo.

The same.

Thanks for all the feedback! I think I got it right this time 🤞

Confirmed the doc edits are appearing correctly at https://github.com/styfle/node/blob/patch-2/doc/api/http2.md

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/282/

PR-URL: #19476 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in cb69a7d

Thank you for the patience!

PR-URL: #19476 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#19476 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #20456 PR-URL: #19476 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

docs: add note about browsers and HTTP/2

b19e4da

nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Mar 20, 2018

vsemozhetbyt reviewed Mar 20, 2018

View reviewed changes

trivikr requested changes Mar 20, 2018

View reviewed changes

mcollina reviewed Mar 20, 2018

View reviewed changes

docs: fix hyperlink to http2 unencrypted

f26470d

vsemozhetbyt reviewed Mar 20, 2018

View reviewed changes

doc: fix name of link

9796ce3

trivikr approved these changes Mar 20, 2018

View reviewed changes

mcollina approved these changes Mar 20, 2018

View reviewed changes

vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 20, 2018

vsemozhetbyt approved these changes Mar 20, 2018

View reviewed changes

vsemozhetbyt added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 20, 2018

jasnell reviewed Mar 20, 2018

View reviewed changes

doc: update comments

c83205d

trivikr approved these changes Mar 20, 2018

View reviewed changes

vsemozhetbyt reviewed Mar 20, 2018

View reviewed changes

doc: fix md hyperlink

7d2ff38

styfle mentioned this pull request Mar 20, 2018

node http2 error #19406

Closed

trivikr approved these changes Mar 20, 2018

View reviewed changes

jasnell approved these changes Mar 20, 2018

View reviewed changes

vsemozhetbyt closed this Mar 20, 2018

styfle deleted the patch-2 branch March 20, 2018 23:04

tniessen removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 24, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

-              // Create a plain-text HTTP/2 server
+              // Create a simple HTTP/2 server
+              // This will not work in most browsers,
+              // Try http2.createSecureServer instead

-              Core API:
+              The following illustrates a simple HTTP/2 server using the Core API.
+              Note the use of [`http2.createSecureServer()`][] since HTTPS is required
+              for [most web browsers](https://caniuse.com/#feat=http2).

               [Compatibility API]: #http2_compatibility_api
               [HTTP/1]: http.html
               [HTTP/2]: https://tools.ietf.org/html/rfc7540
+              [HTTP2 Unencrypted]: https://http2.github.io/faq/#does-http2-require-encryption

               [`tls.connect()`]: tls.html#tls_tls_connect_options_callback
               [`tls.createServer()`]: tls.html#tls_tls_createserver_options_secureconnectionlistener
-              [error code]: #error_codes
+              [error code]: #error_codes

                
                    No newline at end of file

-              The following illustrates a simple, plain-text HTTP/2 server using the
-              Core API:
+              The following illustrates a simple HTTP/2 server using the Core API.
+              Since there are no browsers known that support [unencrypted HTTP/2](HTTP/2 Unencrypted),

+              The following illustrates a simple HTTP/2 server using the Core API.
+              Since no browsers support [unencrypted HTTP/2][HTTP/2 Unencrypted],
+              the use of [`http2.createSecureServer()`][] is preferred.

               Returns a `net.Server` instance that creates and manages `Http2Session`
               instances.
+              Since there are no browsers known that support [unencrypted HTTP/2](HTTP/2 Unencrypted),

doc: add note about browsers and HTTP/2 by styfle · Pull Request #19476 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: add note about browsers and HTTP/2 - #19476

doc: add note about browsers and HTTP/2#19476styfle wants to merge 5 commits intonodejs:masternodejs/node:masterfrom styfle:patch-2styfle/node:patch-2Copy head branch name to clipboard

Conversation

styfle commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

styfle commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

styfle commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Mar 20, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trivikr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

styfle commented Mar 20, 2018

Uh oh!

trivikr left a comment

Choose a reason for hiding this comment

Uh oh!

trivikr commented Mar 20, 2018

Uh oh!

vsemozhetbyt commented Mar 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

doc: add note about browsers and HTTP/2#19476
styfle wants to merge 5 commits into
nodejs:masterfrom
styfle:patch-2

styfle commented Mar 20, 2018 •
edited
Loading