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 4 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
21 changes: 21 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,27 @@ socket.on('end', () => {
});
```

When using an instance of `net.Socket`, use `net.Socket` to upgrade an existing socket. Do not wrap the `net.Socket` in a `TLSSocket`. See the example below for usage of upgrading an existing socket:
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.

When using an instance of net.Socket, use net.Socket to upgrade an existing socket.

I think this sentence is misleading. Users still need to use tls.connect(). Perhaps something like

To upgrade an existing instance of net.Socket to a tls.TLSSocket, pass it to tls.connect() as the socket option:

(code here)

What do you think?

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.

I think that's much more clear - the original issue had a user trying to do this:

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

And I think your wording will keep them from doing that much more clearly. Will update now!

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jul 3, 2017

Choose a reason for hiding this comment

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

Thank you!
Our doc style guide recommends to word-wrap lines at 80 characters)

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.

Will do! Making changes now, thank you @vsemozhetbyt


```js
const Socket = require('net').Socket;
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jul 3, 2017

Choose a reason for hiding this comment

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

Most of our examples use destructuring, so this can be:

const { Socket } = require('net');

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.

Thanks! Changed to the destructured format.

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({ socket: s }, () => {
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!

});
sock.connect({ port: 6697, host: 'irc.freenode.net' });
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 think this works, but our docs say

Usually, a socket is already connected when passed to tls.connect()

It might be a bit counter-intuitive to call tls.connect() before socket.connect(). I don't use the TLS module much, is there any reason for this order?

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.

Good question - I thought that seemed off as well, but worked when I tested it. Let me test it by calling sock.connect first and see; it does seem redundant to call sock.connect after tls.connect.

Copy link
Contributor Author

@VerteDinde VerteDinde Jul 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I replaced { socket: s } with the port and host information, and removed sock.connect entirely. It appears to be passing, and now the sample code is more in line with the docs' recommendations.

```

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

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