Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

The TLS API is inconsistent with the TCP API #1467

Closed
AndreasMadsen opened this Issue · 17 comments

4 participants

@AndreasMadsen

To create a request using TLS the connect method should be used, to do the same with TCP the createConnection should be used. There are also no Socket constructor in the TLS API.

I would be nice if connect was renamed to createConnection and there was a Socket constructor in the TLS API.

@ry
ry commented

i like connect better. That should be added to the TCP API.

@AndreasMadsen

Well thats fine with me.
But I found another - when creating a request using createConnection using TCP there are emitted a connect event "when a socket connection successfully is established". However there are no such event in the TLS API.

I would be nice if the TLS connect function emitted a 'connect' or secureConnect event like the TCP API.

@AndreasMadsen

And there are no callback in the TCP 'createConnection' function.

It would be nice if the new connect function would have an additional callback argument: net.connect(port, host, callback)

@koichik
Owner

@AndreasMadsen - In v0.5, net.createConnection() has a callback.
http://nodejs.org/docs/v0.5.3/api/net.html#net.createConnection

with --use-uv, there is net.connect() (it is not documented yet).

exports.connect = exports.createConnection = function(port /* [host], [cb] */) {
@koichik koichik referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@koichik
Owner

please review.

@AndreasMadsen

Nice that the 'connect' method is documented, but why have two names for the same function. Please consider to remove the documentation for 'createConnection' or just mark it deprecated.

But the TLS api still need to emit a 'connect' or a 'secureConnect' event, when a connection is made.
There also need documentation about the 'tls.Socket' constructor.

@koichik koichik referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@koichik
Owner

@AndreasMadsen - Thanks for your review.
Because net.createConnection() is widely used, I do not want to remove it.
I marked it 'Deprecated'.

And I added 'secureConnect' event.
After d1a2628 was merged to the master, I document it.

@AndreasMadsen

The secureConnect is added but is not documented as I see it.
I also like the tls.CleartextStream documentation but I find it confusing that it is not merged to the master, I assume this is just a matter of time.

@koichik koichik referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@koichik
Owner

@AndreasMadsen - v0.4 branch was merged to the master, then I added 'secureConnect' event to API docs.

@AndreasMadsen

I have nothing more to say.
Thanks.

@koichik
Owner

@ry or @bnoordhuis, can you review 0f12a91?

@koichik
Owner

Reabased.
Only 'secureConnect' event is added to tls.cleartextStream (net.connect() has documented, net_legacy has removed).
@ry @bnoordhuis - Can you review?

@koichik
Owner

@bnoordhuis - Thanks!

@koichik koichik closed this issue from a commit
koichik tls: The TLS API is inconsistent with the TCP API
Add 'secureConnect' event to tls.CleartextStream.

Fixes #1467.
68cc173
@koichik koichik closed this in 68cc173
@koichik
Owner

@AndreasMadsen - Thanks for the suggestion.

@AndreasMadsen

Nice to help

@koichik
Owner

Thanks!

@japj japj referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@japj japj referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.