Skip to content
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

fixed TLSSocket documentation error from Issue #3963. #14062

Closed
wants to merge 13 commits into from
Closed

fixed TLSSocket documentation error from Issue #3963. #14062

Changes from 8 commits
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
24 changes: 24 additions & 0 deletions doc/api/tls.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,30 @@ socket.on('end', () => {
});
```

Passing in `net.Socket` will start a new instance.
Copy link
Member

Choose a reason for hiding this comment

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

Is this line necessary? I am not entirely sure I understand what it is supposed to say, apart from the same thing the next sentence says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. In the original issue, the user was attempting to do this because he didn't understand that if you pass in a net.Socket, you start the net.Socket.:

const tlsSocket = new TlsSocket(new NetSocket());

I just wanted to make sure that that was clearly stated; but if it's clear in the subsequent lines, I'll just take it out 😄

To upgrade an existing instance of `net.Socket` to a
Copy link
Member

Choose a reason for hiding this comment

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

Please try to avoid trailing whitespace characters. Git can help you with that, see this.

(This is not critical, our team will usually fix this while landing the PR, but it is still good practice to take care of that! 😃 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, and thank you for pointing it out! 😄 Thanks so much for all of your help, this was my first open source PR and you were wonderful to work with.

`tls.TLSSocket`, pass it to `tls.connect()` as
the socket option:

```js
const { Socket } = require('net');
const tls = require('tls');
const sock = new Socket();
Copy link
Member

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

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

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' }, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Are we legally allowed and/or encouraged to use a third party domain here? @addaleax

Copy link
Member

Choose a reason for hiding this comment

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

In the documentation? I wouldn’t worry about that, although example.org:443 might be a somewhat more natural choice?

Copy link
Member

Choose a reason for hiding this comment

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

example.org exists for that sole purpose, but users won't be able to connect to it.

Copy link
Member

Choose a reason for hiding this comment

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

https://example.org:443/ seems to work fine for me?

Copy link
Member

Choose a reason for hiding this comment

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

Oh okay, I thought example.com rejects all IP traffic, seems like HTTP and HTTPS work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Added 'https://example.org:443/' instead of the freenode url.

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

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

Choose a reason for hiding this comment

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

Sounds good. 😄 Altered!

});
```

If using TLS as the initial default rather than `net.Socket`,
use only `tls.connect()` to upgrade the socket:
Copy link
Member

Choose a reason for hiding this comment

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

I think "upgrade" is a bit of a misnomer as there is no initial socket to be upgraded. I'd go with just:

If no socket is provided, this function will create a new TLS socket.

as opposed to

If a net.Socket is provided, this function will upgrade that TCP socket to a TLS one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhh this is actually really clarifying - I was wondering what the distinction was with net.Socket. Have clarified by removing my old sentence, and adding yours. Thanks!


```js
const tls = require('tls');
const secureSock = tls.connect({ port: 6697, host: 'irc.freenode.net' }, () => {
console.log('The tls socket connected.');
});
```

## tls.connect(path[, options][, callback])
<!-- YAML
added: v0.11.3
Expand Down