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

styfle · 2018-03-20T13:09:20Z

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

styfle · 2018-03-20T13:11:07Z

/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.

vsemozhetbyt · 2018-03-20T13:18:40Z

doc/api/http2.md

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


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.

trivikr · 2018-03-20T13:27:00Z

doc/api/http2.md

-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).


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

mcollina · 2018-03-20T14:18:38Z

doc/api/http2.md

@@ -252,7 +253,7 @@ and would instead register a handler for the `'stream'` event emitted by the
 ```js
 const http2 = require('http2');

-// Create a plain-text HTTP/2 server
+// Create a simple HTTP/2 server


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 👍

mcollina · 2018-03-20T14:18:54Z

doc/api/http2.md

@@ -1730,7 +1731,9 @@ instances.
 ```js
 const http2 = require('http2');

-// Create a plain-text HTTP/2 server
+// Create a simple HTTP/2 server


can you restore this to plain-text?

I changed to unencrypted

styfle · 2018-03-20T15:45:42Z

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

vsemozhetbyt · 2018-03-20T16:10:53Z

doc/api/http2.md

@@ -3085,6 +3091,7 @@ following additional properties:
 [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


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.

vsemozhetbyt · 2018-03-20T16:11:51Z

doc/api/http2.md

@@ -3124,4 +3131,4 @@ following additional properties:
 [`tls.TLSSocket`]: tls.html#tls_class_tls_tlssocket
 [`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


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

Oops! Fixed! 🔧

vsemozhetbyt · 2018-03-20T16:24:10Z

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

mcollina

LGTM

jasnell · 2018-03-20T18:40:49Z

doc/api/http2.md

-Core API:
+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.



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 👍

jasnell · 2018-03-20T18:41:36Z

doc/api/http2.md

@@ -1727,10 +1728,15 @@ changes:
 Returns a `net.Server` instance that creates and manages `Http2Session`
 instances.

+Since no browsers support [unencrypted HTTP/2][HTTP/2 Unencrypted],
+the use of [`http2.createSecureServer()`][] is preferred.


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 👍

trivikr

LGTM

vsemozhetbyt · 2018-03-20T19:25:36Z

doc/api/http2.md

-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),


(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.

vsemozhetbyt · 2018-03-20T19:31:33Z

doc/api/http2.md

@@ -1727,10 +1729,17 @@ changes:
 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),


styfle · 2018-03-20T20:54:37Z

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

trivikr

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

trivikr · 2018-03-20T21:05:04Z

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

PR-URL: #19476 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]>

vsemozhetbyt · 2018-03-20T21:52:16Z

Landed in cb69a7d

Thank you for the patience!

PR-URL: #19476 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#19476 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #20456 PR-URL: #19476 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 pending requests for changes, 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 pending requests for changes, and a CI started. label Mar 24, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

styfle commented Mar 20, 2018 •

edited

Loading

styfle commented Mar 20, 2018

vsemozhetbyt Mar 20, 2018

styfle Mar 20, 2018

vsemozhetbyt Mar 20, 2018

styfle Mar 20, 2018

trivikr Mar 20, 2018

mcollina Mar 20, 2018

trivikr Mar 20, 2018

mcollina Mar 20, 2018

styfle Mar 20, 2018

mcollina Mar 20, 2018

styfle Mar 20, 2018

styfle commented Mar 20, 2018

vsemozhetbyt Mar 20, 2018

styfle Mar 20, 2018

vsemozhetbyt Mar 20, 2018

styfle Mar 20, 2018

vsemozhetbyt commented Mar 20, 2018

mcollina left a comment

jasnell Mar 20, 2018

styfle Mar 20, 2018

jasnell Mar 20, 2018

styfle Mar 20, 2018

trivikr left a comment

vsemozhetbyt Mar 20, 2018

styfle Mar 20, 2018

vsemozhetbyt Mar 20, 2018

styfle commented Mar 20, 2018

trivikr left a comment

trivikr commented Mar 20, 2018

vsemozhetbyt commented Mar 20, 2018

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

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

Conversation

styfle commented Mar 20, 2018 • edited Loading

Checklist

Affected core subsystem(s)

styfle commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

styfle commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Mar 20, 2018

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trivikr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

styfle commented Mar 20, 2018

trivikr left a comment

Choose a reason for hiding this comment

trivikr commented Mar 20, 2018

vsemozhetbyt commented Mar 20, 2018

styfle commented Mar 20, 2018 •

edited

Loading