FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls: deprecate undocumented tlsSocket.ssl property, add docs by jasnell · Pull Request #23915 · nodejs/node · GitHub

/ node Public

tls: deprecate undocumented tlsSocket.ssl property, add docs - #23915

Closed
jasnell wants to merge 3 commits into
nodejs:masterfrom
jasnell:tls-socket-take2
ConversationCommits (3)ChecksFiles changed
Closed

tls: deprecate undocumented tlsSocket.ssl property, add docs#23915
jasnell wants to merge 3 commits into
nodejs:masterfrom
jasnell:tls-socket-take2

Conversation

jasnell commented Oct 26, 2018

Copy link
Copy Markdown
Member

Updated take on #10846

Fixes: #10555

/cc @sam-github

Checklist

nodejs-github-bot commented Oct 26, 2018

Copy link
Copy Markdown
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1385/pipeline

nodejs-github-bot added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. labels Oct 26, 2018
jasnell commented Oct 26, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
jasnell commented Oct 26, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
vsemozhetbyt reviewed Oct 27, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
vsemozhetbyt reviewed Oct 27, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
vsemozhetbyt reviewed Oct 27, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
addaleax reviewed Oct 27, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
Comment thread doc/api/tls.md Outdated
vsemozhetbyt reviewed Oct 27, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
vsemozhetbyt reviewed Oct 27, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
jasnell commented Oct 29, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
jasnell commented Oct 29, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
jasnell commented Oct 29, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated
jasnell force-pushed the tls-socket-take2 branch 2 times, most recently from 02e937e to 66849e7 Compare October 30, 2018 15:44
jasnell changed the title doc: tls API for direct TLS socket use tls: deprecate undocumented tlsSocket.ssl property, add docs Oct 30, 2018
jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Oct 30, 2018

jasnell commented Oct 30, 2018

Copy link
Copy Markdown
Member Author

@addaleax @sam-github @bnoordhuis @nodejs/tsc ... please take another look over. This has changed significantly and is now a semver-major deprecation.

sam-github commented Oct 30, 2018

Copy link
Copy Markdown
Contributor

Are the things people were using .ssl for possible via public APIs?

jasnell commented Oct 30, 2018

Copy link
Copy Markdown
Member Author

I've added verifyError() as a function on TLSSocket.prototype but I'm really not sure what else folks have been using beyond that. We can address those cases as needed.

vsemozhetbyt reviewed Oct 30, 2018
View reviewed changes
Comment thread doc/api/deprecations.md Outdated
vsemozhetbyt reviewed Oct 30, 2018
View reviewed changes
Comment thread doc/api/tls.md Outdated

jasnell commented Nov 1, 2018

Copy link
Copy Markdown
Member Author

Ping @nodejs/tsc

jasnell requested a review from addaleax November 1, 2018 14:35
addaleax approved these changes Nov 1, 2018
View reviewed changes
Comment thread lib/_tls_wrap.js Outdated
tls: deprecate undocumented .ssl property, add docs
4ca0e4c 
deprecate the legacy undocumented `.ssl` alias for the
`TLSSocket._handle` and document alternatives. Document
how to properly use the `TLSSocket` constructor directly.

Updated take on nodejs#10846

Fixes: nodejs#10555
[Squash] use kHandle
c82010d
Trott reviewed Nov 2, 2018
View reviewed changes
Comment thread doc/api/deprecations.md Outdated
Trott reviewed Nov 2, 2018
View reviewed changes
Comment thread doc/api/tls.md
smaller fragments add extra TLS framing bytes and CPU overhead, which may
decrease overall server throughput.

### tlsSocket.verifyError()

Trott Nov 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry if these questions are naïve or otherwise ill-informed: Is this the best name for this? I know it's what we used previously, but it wasn't exposed publicly, was it? Might `validatePeerCertificate() or something like that be a better name?

jasnell Nov 19, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm fine with whatever name folks would like to see.

[Squash] suggestion
5116f58 
Co-Authored-By: jasnell <jasnell@gmail.com>

jasnell commented Nov 19, 2018

Copy link
Copy Markdown
Member Author

Ping @nodejs/tsc

rvagg commented Nov 19, 2018

Copy link
Copy Markdown
Member

there's an outstanding question from @Trott inline

lgtm I think, although the commit description doesn't hint at the addition of features. So it might be best to split out verifyError() to a separate commit.

jasnell closed this Jan 28, 2019
armanbilge mentioned this pull request Jul 26, 2022
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis left review comments
Trott Trott left review comments
addaleax addaleax approved these changes
+2 more reviewers
vsemozhetbyt vsemozhetbyt left review comments
sam-github sam-github approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL