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

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

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

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

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

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

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

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

+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

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

(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

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

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

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 72 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/api/http2.md Outdated

Copy link

Contributor

vsemozhetbyt Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same.

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

Uh oh!

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

Milestone

Development

Uh oh!

7 participants

styfle commented Mar 20, 2018 •

edited

Loading