tls: deprecate parseCertString & move to internal #14249

XadillaX · 2017-07-15T04:40:59Z

tls.parseCertString() exposed by accident. Now move this function to
internal/tls and mark the original one as deprecated.

Refs: #14193
Refs: af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tls, doc

XadillaX · 2017-07-15T04:41:38Z

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

jasnell · 2017-07-17T15:30:02Z

lib/tls.js

-};
+exports.parseCertString = internalUtil.deprecate(
+  internalTLS.parseCertString,
+  'tls.parseCertString is deprecated',


I would expand this message a bit...

tls.parseCertString() is deprecated. Please use querystring.parse() instead.

jasnell · 2017-07-17T15:31:10Z

test/parallel/test-tls-parse-cert-string.js

+  common.hijackStderr(function(data) {
+    common.restoreStderr();
+    process.nextTick(hijackTest.bind(null, data));
+  });


Please just use common.expectWarning() to detect the deprecation warning. There's no need to hijack the stderr.

XadillaX · 2017-07-21T05:02:02Z

/ping @jasnell

refack · 2017-07-25T09:09:41Z

Ping @nodejs/ctc this is a runtime deprecation hence semver-major

bnoordhuis · 2017-07-25T09:18:14Z

I'm going to be selfish and request that #14447 (edit: #14473) lands before this PR. They conflict and that PR will need to be back-ported to the release branches.

XadillaX · 2017-08-04T06:59:39Z

@bnoordhuis Could this PR be landed now?

bnoordhuis · 2017-08-04T07:01:21Z

Sure. Will need a rebase, though.

XadillaX · 2017-08-04T07:32:03Z

@bnoordhuis rebased 👌

XadillaX · 2017-08-04T14:36:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9477/

refack · 2017-08-04T14:38:41Z

@nodejs/ctc PTAL, this needs another approval.
tl;dr parseCertString was never documented and AFAICT "leaked" to public surface. Anyway it performs a trivial operation.

addaleax · 2017-08-04T14:59:06Z

I have to admit that I don’t see the point in deprecating this as opposed to just documenting it.

We should need stronger motivation to deprecate something than “it was not exposed intentionally”.

bnoordhuis · 2017-08-04T16:43:04Z

We should need stronger motivation to deprecate something than “it was not exposed intentionally”.

How about "it's not very useful"?

(For me the first reason is already good enough.)

XadillaX · 2017-08-06T05:39:33Z

@addaleax But if we move this function into internal without any notification, how do we explain to people use this function? (Maybe they just use this function like wow I found a pirate treasure that not documented)

If we deleted this function, they may be confused like wow the treasure flied away?

addaleax · 2017-08-07T01:40:39Z

@XadillaX Right, not deprecating it means not removing it. I just don’t/didn’t see much of a reason to touch it at all.

Maybe they just use this function like wow I found a pirate treasure that not documented

That’s my thought exactly on why documenting might be better than deprecating. :)

XadillaX · 2017-08-07T03:13:45Z

So what's the final decision😂 Both are OK to me.

deprecate or document?

/cc @addaleax @refack @bnoordhuis @jasnell

bnoordhuis · 2017-08-07T07:51:52Z

@addaleax mentioned in the other issue she didn't object strenuously and this PR has two sign-offs from CTC members. It's good to go.

BridgeAR · 2017-08-30T20:15:25Z

@XadillaX this needs a rebase

XadillaX · 2017-09-01T07:22:17Z

@XadillaX this needs a rebase

@ BridgeAR done. 👌

jasnell · 2017-09-01T17:45:43Z

@nodejs/tsc ... since this is a deprecation, one last call for objections.

jasnell · 2017-09-01T17:46:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9919/

BridgeAR

LGTM with two small suggestions

BridgeAR · 2017-09-01T18:01:24Z

lib/_tls_common.js

@@ -21,6 +21,7 @@

 'use strict';

+const internalTLS = require('internal/tls');


Nit: you could write const { parseCertString } = require(...)

BridgeAR · 2017-09-01T18:01:55Z

test/parallel/test-tls-parse-cert-string.js

@@ -1,16 +1,24 @@
 /* eslint-disable no-proto */
 'use strict';
+
+// Flags: --expose_internals


Nit: it would be good to move the Flags to the top of the function.

I actually meant to line one with my comment (every loaded file is wrapped in a function) but I guess it does not matter all that much.

thefourtheye · 2017-09-07T15:21:02Z

doc/api/deprecations.md

+```
+
+*Note*: This function is not completely equivalent to `querystring.parse()`, one
+notable difference is that `querystring.parse()` does url encoding, e.g.:


Did you mean decoding?

`tls.parseCertString()` exposed by accident. Now move this function to `internal/tls` and mark the original one as deprecated. Refs: nodejs#14193 Refs: nodejs@af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188

XadillaX · 2017-09-08T11:34:28Z

I've rebased this PR. @BridgeAR

BridgeAR · 2017-09-13T19:55:38Z

Landed in 468110b

`tls.parseCertString()` exposed by accident. Now move this function to `internal/tls` and mark the original one as deprecated. PR-URL: #14249 Refs: #14193 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

`tls.parseCertString()` exposed by accident. Now move this function to `internal/tls` and mark the original one as deprecated. PR-URL: nodejs/node#14249 Refs: nodejs/node#14193 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

* **async_hooks** * Older experimental `async_hooks` APIs have been removed [[`d731369b1d`](d731369b1d)] **(SEMVER-MAJOR)** [#14414](#14414) * **Errors** * Multiple built in modules have been migrated to use static error codes * **Domains** * The long deprecated `.dispose()` method has been removed [[`602fd36d95`](602fd36d95)] **(SEMVER-MAJOR)** [#15412](#15412) * **File system** * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()` [[`e5c290bed9`](e5c290bed9)] **(SEMVER-MAJOR)** [#15407](#15407) * `fs` callbacks are now invoked with an undefined `this` context [[`2249234fee`](2249234fee)] **(SEMVER-MAJOR)** [#14645](#14645) * **HTTP** * Socket timeout is set when the socket connects [[`10be20a0e8`](10be20a0e8)] **(SEMVER-MAJOR)** [#8895](#8895) * A bug causing the request `error` event to fire twice has been fixed [[`620ba41694`](620ba41694)] **(SEMVER-MAJOR)** [#14659](#14659) * The `pipe` method on `OutgoingMessage` has been disabled [[`156549d8ff`](156549d8ff)] **(SEMVER-MAJOR)** [#14358](#14358) * **HTTP/2** * The `--expose-http2` command-line argument is no longer required [[`f55ee6e24a`](f55ee6e24a)] **(SEMVER-MAJOR)** [#15535](#15535) * **Internationalization** * The `Intl.v8BreakIterator` class has been removed [[`668ad44922`](668ad44922)] **(SEMVER-MAJOR)** [#15238](#15238) * **OS** * `os.EOL` is now read-only [[`f6caeb9526`](f6caeb9526)] **(SEMVER-MAJOR)** [#14622](#14622) * **Process** * It is now possible to pass additional flags to `dlopen` [[`5f22375922`](5f22375922)] **(SEMVER-MAJOR)** [#12794](#12794) * **Timers** * Using a timeout duration larger than 32-bits will now emit a warning [[`ce3586da31`](ce3586da31)] **(SEMVER-MAJOR)** [#15627](#15627) * **TLS** * `parseCertString` has been deprecated [[`468110b327`](468110b327)] **(SEMVER-MAJOR)** [#14249](#14249) * Type-checking for `key`, `cert`, and `ca` options has been added [[`a7dccd040d`](a7dccd040d)] **(SEMVER-MAJOR)** [#14807](#14807)

nodejs-github-bot added build Issues and PRs related to build files or the CI. tls Issues and PRs related to the tls subsystem. labels Jul 15, 2017

jasnell reviewed Jul 17, 2017

View reviewed changes

XadillaX force-pushed the move-tls.parseCertString branch from e8108cc to dd2099f Compare July 18, 2017 10:00

XadillaX mentioned this pull request Jul 24, 2017

doc, tls: mark parseCertString() as deprecated #14245

Closed

2 tasks

refack approved these changes Jul 25, 2017

View reviewed changes

refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 25, 2017

XadillaX mentioned this pull request Jul 26, 2017

Tracking Issue: Node.js 9.0.0 #14391

Closed

Trott added this to the 9.0.0 milestone Jul 26, 2017

XadillaX force-pushed the move-tls.parseCertString branch from dd2099f to 6a8b5b2 Compare August 4, 2017 07:31

bnoordhuis approved these changes Aug 4, 2017

View reviewed changes

jasnell approved these changes Aug 4, 2017

View reviewed changes

XadillaX force-pushed the move-tls.parseCertString branch from 6a8b5b2 to 89781dd Compare September 1, 2017 07:22

BridgeAR approved these changes Sep 1, 2017

View reviewed changes

thefourtheye reviewed Sep 7, 2017

View reviewed changes

tls: deprecate parseCertString & move to internal

6b12b7e

`tls.parseCertString()` exposed by accident. Now move this function to `internal/tls` and mark the original one as deprecated. Refs: nodejs#14193 Refs: nodejs@af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188

XadillaX force-pushed the move-tls.parseCertString branch from 2e8e31e to 6b12b7e Compare September 8, 2017 08:00

thefourtheye approved these changes Sep 8, 2017

View reviewed changes

BridgeAR approved these changes Sep 13, 2017

View reviewed changes

BridgeAR closed this Sep 13, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

bnoordhuis mentioned this pull request May 28, 2018

Missing documentation for tls.parseCertString #14193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: deprecate parseCertString & move to internal #14249

tls: deprecate parseCertString & move to internal #14249

XadillaX commented Jul 15, 2017 •

edited

Loading

XadillaX commented Jul 15, 2017 •

edited

Loading

jasnell Jul 17, 2017

jasnell Jul 17, 2017

XadillaX commented Jul 21, 2017

refack commented Jul 25, 2017

bnoordhuis commented Jul 25, 2017 •

edited

Loading

XadillaX commented Aug 4, 2017

bnoordhuis commented Aug 4, 2017

XadillaX commented Aug 4, 2017

XadillaX commented Aug 4, 2017

refack commented Aug 4, 2017

addaleax commented Aug 4, 2017

bnoordhuis commented Aug 4, 2017

XadillaX commented Aug 6, 2017

addaleax commented Aug 7, 2017

XadillaX commented Aug 7, 2017

bnoordhuis commented Aug 7, 2017

BridgeAR commented Aug 30, 2017

XadillaX commented Sep 1, 2017

jasnell commented Sep 1, 2017

jasnell commented Sep 1, 2017

BridgeAR left a comment

BridgeAR Sep 1, 2017

BridgeAR Sep 1, 2017

BridgeAR Sep 13, 2017

thefourtheye Sep 7, 2017

XadillaX commented Sep 8, 2017

BridgeAR commented Sep 13, 2017

		@@ -21,6 +21,7 @@

		'use strict';

		const internalTLS = require('internal/tls');

tls: deprecate parseCertString & move to internal #14249

tls: deprecate parseCertString & move to internal #14249

Conversation

XadillaX commented Jul 15, 2017 • edited Loading

Checklist

Affected core subsystem(s)

XadillaX commented Jul 15, 2017 • edited Loading

jasnell Jul 17, 2017

Choose a reason for hiding this comment

jasnell Jul 17, 2017

Choose a reason for hiding this comment

XadillaX commented Jul 21, 2017

refack commented Jul 25, 2017

bnoordhuis commented Jul 25, 2017 • edited Loading

XadillaX commented Aug 4, 2017

bnoordhuis commented Aug 4, 2017

XadillaX commented Aug 4, 2017

XadillaX commented Aug 4, 2017

refack commented Aug 4, 2017

addaleax commented Aug 4, 2017

bnoordhuis commented Aug 4, 2017

XadillaX commented Aug 6, 2017

addaleax commented Aug 7, 2017

XadillaX commented Aug 7, 2017

bnoordhuis commented Aug 7, 2017

BridgeAR commented Aug 30, 2017

XadillaX commented Sep 1, 2017

jasnell commented Sep 1, 2017

jasnell commented Sep 1, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Sep 1, 2017

Choose a reason for hiding this comment

BridgeAR Sep 1, 2017

Choose a reason for hiding this comment

BridgeAR Sep 13, 2017

Choose a reason for hiding this comment

thefourtheye Sep 7, 2017

Choose a reason for hiding this comment

XadillaX commented Sep 8, 2017

BridgeAR commented Sep 13, 2017

XadillaX commented Jul 15, 2017 •

edited

Loading

XadillaX commented Jul 15, 2017 •

edited

Loading

bnoordhuis commented Jul 25, 2017 •

edited

Loading