Skip to content

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

Closed
styfle wants to merge 5 commits into
nodejs:masterfrom
styfle:patch-2
Closed

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

Conversation

@styfle
Copy link
Copy Markdown
Member

@styfle styfle commented Mar 20, 2018 •
edited
Loading

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
@nodejs-github-bot 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
@styfle
Copy link
Copy Markdown
Member Author

styfle commented Mar 20, 2018

/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
vsemozhetbyt reviewed Mar 20, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
// Create a plain-text HTTP/2 server
// Create a simple HTTP/2 server
// This will not work in most browsers,
// Try http2.createSecureServer instead
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Mar 20, 2018

Choose a reason for hiding this comment

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

A nit: // Try -> // try after the comma above?
Copy link
Copy Markdown
Member Author

@styfle styfle Mar 20, 2018

Choose a reason for hiding this comment

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

Any thoughts on usage of periods . anywhere?
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Mar 20, 2018

Choose a reason for hiding this comment

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

Let's see what others think, I am not a native speaker)
Copy link
Copy Markdown
Member Author

@styfle styfle Mar 20, 2018

Choose a reason for hiding this comment

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

I added periods.
trivikr
trivikr requested changes Mar 20, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
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).
Copy link
Copy Markdown
Member

@trivikr trivikr Mar 20, 2018

Choose a reason for hiding this comment

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

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
mcollina reviewed Mar 20, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
const http2 = require('http2');

// Create a plain-text HTTP/2 server
// Create a simple HTTP/2 server
Copy link
Copy Markdown
Member

@mcollina mcollina Mar 20, 2018

Choose a reason for hiding this comment

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

I think the term plain-text is more specific.
Copy link
Copy Markdown
Member

@trivikr trivikr Mar 20, 2018

Choose a reason for hiding this comment

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

How about using the term unencrypted over plain-text?
Copy link
Copy Markdown
Member

@mcollina mcollina Mar 20, 2018

Choose a reason for hiding this comment

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

I'd be good with that.
Copy link
Copy Markdown
Member Author

@styfle styfle Mar 20, 2018

Choose a reason for hiding this comment

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

Sounds good to me 👍
Comment thread doc/api/http2.md Outdated
const http2 = require('http2');

// Create a plain-text HTTP/2 server
// Create a simple HTTP/2 server
Copy link
Copy Markdown
Member

@mcollina mcollina Mar 20, 2018

Choose a reason for hiding this comment

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

can you restore this to plain-text?
Copy link
Copy Markdown
Member Author

@styfle styfle Mar 20, 2018

Choose a reason for hiding this comment

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

I changed to unencrypted
@styfle
Copy link
Copy Markdown
Member Author

styfle commented Mar 20, 2018

Thanks for the comments. I pushed an update with the revised docs.
vsemozhetbyt
vsemozhetbyt reviewed Mar 20, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
[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
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Mar 20, 2018

Choose a reason for hiding this comment

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

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

@styfle styfle Mar 20, 2018

Choose a reason for hiding this comment

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

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
vsemozhetbyt reviewed Mar 20, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
[`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
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Mar 20, 2018

Choose a reason for hiding this comment

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

A nit: missing line break at the end of the file.
Copy link
Copy Markdown
Member Author

@styfle styfle Mar 20, 2018

Choose a reason for hiding this comment

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

Oops! Fixed! 🔧
@vsemozhetbyt
Copy link
Copy Markdown
Contributor

vsemozhetbyt commented Mar 20, 2018

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/280/
mcollina
mcollina approved these changes Mar 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM
@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 72 hours to land. label Mar 20, 2018
@vsemozhetbyt 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
jasnell reviewed Mar 20, 2018
View reviewed changes
Comment thread 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.

Copy link
Copy Markdown
Member

@jasnell jasnell Mar 20, 2018

Choose a reason for hiding this comment

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

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.
Copy link
Copy Markdown
Member Author

@styfle styfle Mar 20, 2018

Choose a reason for hiding this comment

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

Okie dokie 👍
jasnell
jasnell reviewed Mar 20, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
instances.

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

@jasnell jasnell Mar 20, 2018

Choose a reason for hiding this comment

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

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.
Copy link
Copy Markdown
Member Author

@styfle styfle Mar 20, 2018

Choose a reason for hiding this comment

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

Okie dokie 👍
trivikr
trivikr approved these changes Mar 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

LGTM
vsemozhetbyt
vsemozhetbyt reviewed Mar 20, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
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),
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Mar 20, 2018

Choose a reason for hiding this comment

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

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

@styfle styfle Mar 20, 2018

Choose a reason for hiding this comment

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

Whoa good catch, I just copy/pasted the code review comment so there must have been a typo.
vsemozhetbyt
vsemozhetbyt reviewed Mar 20, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated
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),
Copy link
Copy Markdown
Contributor

@vsemozhetbyt 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.
@styfle
Copy link
Copy Markdown
Member Author

styfle commented Mar 20, 2018

Thanks for all the feedback! I think I got it right this time 🤞
@styfle styfle mentioned this pull request Mar 20, 2018
Closed
trivikr
trivikr approved these changes Mar 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

Confirmed the doc edits are appearing correctly at https://github.com/styfle/node/blob/patch-2/doc/api/http2.md
@trivikr
Copy link
Copy Markdown
Member

trivikr commented Mar 20, 2018

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/282/
vsemozhetbyt pushed a commit that referenced this pull request Mar 20, 2018
@styfle @vsemozhetbyt
doc: add note about browsers and HTTP/2
cb69a7d 
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
Copy link
Copy Markdown
Contributor

vsemozhetbyt commented Mar 20, 2018

Landed in cb69a7d

Thank you for the patience!
@styfle styfle deleted the patch-2 branch March 20, 2018 23:04
MylesBorins pushed a commit that referenced this pull request Mar 22, 2018
@styfle @MylesBorins
doc: add note about browsers and HTTP/2
c3886b5 
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>
@tniessen 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
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
@styfle @kjin
doc: add note about browsers and HTTP/2
89c42a3 
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>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
@styfle @MylesBorins
doc: add note about browsers and HTTP/2
8daddfb 
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>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 15, 2018
@styfle @MylesBorins
doc: add note about browsers and HTTP/2
1f420fc 
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>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
@styfle @MylesBorins
doc: add note about browsers and HTTP/2
da185ce 
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>
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. fast-track PRs that do not need to wait for 72 hours to land. http2 Issues or PRs related to the http2 subsystem.

7 participants

@styfle @vsemozhetbyt @trivikr @mcollina @jasnell @tniessen @nodejs-github-bot