Skip to content

fixed TLSSocket documentation error from Issue #3963. #14062

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.

Sign up for GitHub

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

Closed
VerteDinde wants to merge 13 commits into from
Closed

fixed TLSSocket documentation error from Issue #3963. #14062

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
55f3170
Fixing TLS documentation error in Issue #3963. See line 879 - 902 in …
VerteDinde Jul 3, 2017
09f8564
Changed documentation to remove pronoun usage
VerteDinde Jul 3, 2017
cbbe60c
Removed pronouns 'you'
VerteDinde Jul 3, 2017
7e766ff
Adjusted 'vars' to 'const', changed parsing error in last code exampl…
VerteDinde Jul 3, 2017
8b945f8
Word wrap at ~80 characters
VerteDinde Jul 3, 2017
5144cf4
Added destructuring to const Socket
VerteDinde Jul 3, 2017
fda1b66
Added codetype to `net.Socket`, updated word wrap, updated descriptio…
VerteDinde Jul 3, 2017
de3d050
Removed sock.connect, appears to be passing
VerteDinde Jul 3, 2017
872132e
Fixed socket update sample code, changed example url to https://examp…
VerteDinde Jul 3, 2017
5c41572
Fixed example.org port number
VerteDinde Jul 3, 2017
955ad0f
Fixed host name
VerteDinde Jul 3, 2017
9658ac3
Destructured socket, simplified net.Socket explanation, removed upgra…
VerteDinde Jul 5, 2017
077508d
Fixed variable name
VerteDinde Jul 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions doc/api/tls.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,7 @@ the socket option:
const { Socket } = require('net');
const tls = require('tls');
const sock = new Socket();
Copy link
Member

@addaleax addaleax Jul 3, 2017

Choose a reason for hiding this comment

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

Where is this variable used?

Copy link
Member

@tniessen tniessen Jul 3, 2017

Choose a reason for hiding this comment

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

Uhm something went wrong here... The original code (see first commit) looked like tls.connect({ socket: sock }). @VerteDinde You probably don't want to use tls.connect({ port: 6697, host: 'irc.freenode.net' }) in both cases, right? This won't upgrade an existing socket, it establishes a new connection.

Copy link
Contributor Author

@VerteDinde VerteDinde Jul 3, 2017

Choose a reason for hiding this comment

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

Yes, you're right! Just updated to reflect that variable and changed the irc.freenode.net to https://example.org:443/.

const secureSock = tls.connect({ port: 6697, host: 'irc.freenode.net' }, () => {
const secureSock = tls.connect({ socket: sock }, () => {
Copy link
Contributor

@cjihrig cjihrig Jul 4, 2017

Choose a reason for hiding this comment

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

If you rename sock to socket, you can drop the : sock.

Copy link
Contributor Author

@VerteDinde VerteDinde Jul 5, 2017

Choose a reason for hiding this comment

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

Thanks man! Destructured.

console.log('The tls socket connected.');
Copy link
Member

@TimothyGu TimothyGu Jul 4, 2017

Choose a reason for hiding this comment

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

I'd prefer 'The TLS socket has been connected.'

Copy link
Contributor Author

@VerteDinde VerteDinde Jul 5, 2017

Choose a reason for hiding this comment

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

Sounds good. 😄 Altered!

});
```
Expand All @@ -895,7 +895,7 @@ use only `tls.connect()` to upgrade the socket:

```js
const tls = require('tls');
const secureSock = tls.connect({ port: 6697, host: 'irc.freenode.net' }, () => {
const secureSock = tls.connect({ port: 6697, host: 'https://example.org:443/' }, () => {
Copy link
Member

@tniessen tniessen Jul 3, 2017

Choose a reason for hiding this comment

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

I am sorry if we were too vague about this, but this is not how TLS sockets work. You need to specify example.org as the host and 443 as the port instead of 6697. https://example.org:443/ is a URL, @addaleax just used it to point out that browsers can connect to port 443. Sockets are below HTTP level, HTTP(s) is built on top of (TLS) sockets.

Copy link
Contributor Author

@VerteDinde VerteDinde Jul 4, 2017

Choose a reason for hiding this comment

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

Ha, sorry - this is now fixed. Thanks so much for your patience, @tniessen 😝

console.log('The tls socket connected.');
});
```
Expand Down