GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc, tls: mark parseCertString() as deprecated by XadillaX · Pull Request #14245 · nodejs/node

Conversation

@XadillaX

@XadillaX XadillaX commented Jul 15, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

tls.parseCertString() was made public by mistack. So mark it as
deprecated.

Refs: #14193
Refs: af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

tls, doc

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. v8.x labels Jul 15, 2017
@XadillaX

XadillaX commented Jul 15, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

This is a sub-PR of #14218 (comment).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Jul 15, 2017

Copy link
Copy Markdown
Member

/cc @jasnell
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@XadillaX

XadillaX commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

/ping @jasnell
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. blocked PRs that are blocked by other issues or PRs. labels Jul 18, 2017
@addaleax

addaleax commented Jul 18, 2017

Copy link
Copy Markdown
Member

I assume this is blocked by #14218 to begin with
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax
addaleax force-pushed the v8.x-staging branch from 71c15d5 to e421f08 Compare July 18, 2017 18:48
@XadillaX

XadillaX commented Jul 20, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@addaleax Why it blocked?

And I've rebased the code. As @refack said, it's a sub-PR of #14218. I will split that PR into 3 sub-PRs and this is the first one of them.

@refack Did you mean:

  1. mark the function as deprecated with documented-only
  2. move the function and modify the document with runtime
  3. move the constant

Yes, exactly (so that we could land just (1) in v8.x)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@XadillaX XadillaX changed the title doc,tls: mark parseCertString() as deprecated [v8.x] doc,tls: mark parseCertString() as deprecated Jul 20, 2017
@XadillaX
XadillaX force-pushed the mark-tls.parseCertString-as-deprecated branch from 4387926 to 1def55b Compare July 20, 2017 03:33
@jasnell

jasnell commented Jul 20, 2017

Copy link
Copy Markdown
Member

@nodejs/ctc ... a decision needs to be made whether deprecating this in 8.x is appropriate.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Jul 20, 2017
View reviewed changes
Trott
Trott reviewed Jul 20, 2017
View reviewed changes
Comment thread doc/api/deprecations.md Outdated

@Trott Trott Jul 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit: Replace comma with a period. One then will begin a new sentence.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Trott

Trott commented Jul 20, 2017

Copy link
Copy Markdown
Member

a decision needs to be made whether deprecating this in 8.x is appropriate.

Deprecations are supposed to be semver-major, right? We should stick to that unless there is a compelling user-focused argument otherwise.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Jul 20, 2017
View reviewed changes

@Trott Trott left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM with a nit.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Jul 20, 2017

Copy link
Copy Markdown
Member

Yes, deprecations are semver-major, which means this PR would not be able to land as a backport to 8.x... unless we special case it as we're done in other cases. The question is whether this one rises to the level that a special case needs to be made. I'm not so sure. I signed off because the code change itself looks fine, but I'm not convinced that this should land.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123
Fishrock123 force-pushed the v8.x-staging branch from e9491f1 to 7c3fed5 Compare July 20, 2017 16:00
@Trott

Trott commented Jul 20, 2017

Copy link
Copy Markdown
Member

I'm not convinced that this should land.

@jasnell You mean you're not convinced that it should land in 8.x but it's fine for 9.0.0? Or you mean not land at all?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Jul 20, 2017

Copy link
Copy Markdown
Member

Oh, never mind, I see, this is an 8.x-specific PR. Yeah, I'm not sure it should land either. Fine for 9.x...And I'm OK if CTC decides to special-case it, but that really really makes me wonder if we need to revise our deprecation policy because these special cases seem to come up more than I expect them to....
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack

refack commented Jul 21, 2017

Copy link
Copy Markdown
Contributor

Just repeating the observation that tls.parseCertString() is undocumented in v8.x so the options for 8 are:

  1. Do nothing
  2. Document
  3. Document as deprecated

IMHO (3) is reasonable since it's definitely an edge case for deprecation, but not obviously semver-major. An academic argument could be made that since it's currently undocumented, it was "latently deprecated" all along, we are just documenting it now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Jul 21, 2017

Copy link
Copy Markdown
Member

I wouldn't stand in the way of a doc-only deprecation. If we can get a clean diff for that, maybe we can ping CTC and take it as consensus if no one objects after 72 hours.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@XadillaX
XadillaX force-pushed the mark-tls.parseCertString-as-deprecated branch from b95abf4 to 1cb3e56 Compare July 23, 2017 07:54
@XadillaX

XadillaX commented Jul 23, 2017

Copy link
Copy Markdown
Contributor Author

/ping @nodejs/ctc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Jul 23, 2017

Copy link
Copy Markdown
Member

@nodejs/ctc Any objections to landing this doc-only deprecation in 8.x? It is for a property that was made public by mistake.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Jul 23, 2017

Copy link
Copy Markdown
Member

@addaleax I'm not clear on why this would be blocked by #14218. Can you explain?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added the ctc-review label Jul 23, 2017
@addaleax

addaleax commented Jul 23, 2017

Copy link
Copy Markdown
Member

@Trott It seems to me like if we introduce a deprecation in 8.x, we should also deprecate in master so that we don’t accidentally un-deprecate when releasing 9.0.0?
Trott, gibfahn, and mhdawson reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@XadillaX

XadillaX commented Jul 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@addaleax I think we should land #14249 before releasing 9.0.

One is document-only and another is runtime.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@XadillaX
update after reviewing
66d7e59
@XadillaX

XadillaX commented Sep 1, 2017

Copy link
Copy Markdown
Contributor Author

I think this PR should be landed before #14249.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Sep 2, 2017

Copy link
Copy Markdown
Member

If there are no objections from @nodejs/tsc I would land this as a semver-minor.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@XadillaX

XadillaX commented Sep 6, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

/ping @nodejs/tsc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Sep 6, 2017

Copy link
Copy Markdown
Member

With no objections by now, I'd say this is likely good to go as a semver-minor
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 6, 2017
@BridgeAR

BridgeAR commented Sep 8, 2017

Copy link
Copy Markdown
Member

Landed in 703a3b5
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR closed this Sep 8, 2017
BridgeAR pushed a commit that referenced this pull request Sep 8, 2017
@XadillaX @BridgeAR
doc, tls: mark parseCertString() as deprecated
703a3b5 
`tls.parseCertString()` was made public by mistack. So mark it as
deprecated.

PR-URL: #14245
Refs: #14193
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins

MylesBorins commented Sep 10, 2017

Copy link
Copy Markdown
Contributor

Thi needs to be backported to v8.x-staging. You can follow the guide and raise a backport PR against v8.x-staging
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins added the backport-requested-v8.x label Sep 10, 2017
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
@XadillaX @addaleax
doc, tls: mark parseCertString() as deprecated
e322be6 
`tls.parseCertString()` was made public by mistack. So mark it as
deprecated.

PR-URL: nodejs#14245
Refs: nodejs#14193
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
@XadillaX @jasnell
doc, tls: mark parseCertString() as deprecated
b09eeb4 
`tls.parseCertString()` was made public by mistack. So mark it as
deprecated.

PR-URL: #14245
Refs: #14193
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell added a commit that referenced this pull request Sep 20, 2017
@jasnell
2017-09-26, Node.js Version 8.6.0 (Current)
3e3091a 
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
@jasnell jasnell mentioned this pull request Sep 20, 2017
Node.js 8.6.0 Release Proposal #15509
Merged
jasnell added a commit that referenced this pull request Sep 21, 2017
@jasnell
2017-09-26, Node.js Version 8.6.0 (Current)
14d445d 
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
jasnell added a commit that referenced this pull request Sep 25, 2017
@jasnell
2017-09-26, Node.js Version 8.6.0 (Current)
883cd78 
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
jasnell added a commit that referenced this pull request Sep 26, 2017
@jasnell
2017-09-26, Node.js Version 8.6.0 (Current)
b48ff2a 
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
jasnell added a commit that referenced this pull request Sep 26, 2017
@jasnell
2017-09-26, Node.js Version 8.6.0 (Current)
05e4c1d 
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
@jasnell @MylesBorins
2017-09-26, Node.js Version 8.6.0 (Current)
8f07f00 
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
@jasnell @addaleax
2017-09-26, Node.js Version 8.6.0 (Current)
b7374d9 
* **crypto**
  * Support for multiple ECDH curves. [#15206](nodejs/node#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](nodejs/node#7855)
  * Custom lookup functions are now supported. [#14560](nodejs/node#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](nodejs/node#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](nodejs/node#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](nodejs/node#15354)
@MylesBorins MylesBorins removed the backport-requested-v8.x label Oct 7, 2017
@MylesBorins MylesBorins added the dont-land-on-v6.x label Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes
@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes
+3 more reviewers
@thefourtheye thefourtheye thefourtheye left review comments
@refack refack refack approved these changes
@rvagg rvagg rvagg approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@XadillaX @Trott @addaleax @jasnell @refack @mhdawson @mcollina @BridgeAR @MylesBorins @rvagg @thefourtheye @cjihrig @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.