Remove crypto.Credentials API #21153

aduh95 · 2018-06-05T18:34:04Z

Fix #20793

The crypto.Credentials legacy API has been Runtime deprecated since
v0.11.13 and users had been adviced to use tls.SecureContext instead.

This PR removes it from the crypto module.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

targos · 2018-06-05T18:59:53Z

lib/crypto.js

@@ -211,7 +211,7 @@ Object.defineProperties(exports, {
      fipsForced ? setFipsForced : setFipsCrypto
  },
  DEFAULT_ENCODING: {
-    enumerable: true,
+    enumerable: false,


this change seems unrelated

targos · 2018-06-05T19:00:16Z

@nodejs/crypto @nodejs/tsc

mcollina

LGTM with the changes @targos mentioned

trivikr · 2018-06-06T01:40:27Z

doc/api/deprecations.md


-The [`crypto.createCredentials()`][] API is deprecated. Please use
+The [`crypto.createCredentials()`][] API has been removed. Please use


has been removed -> was removed (ditto on line 155)
Similar to PR #21043

tniessen · 2018-06-06T10:05:01Z

lib/crypto.js

@@ -223,22 +223,4 @@ Object.defineProperties(exports, {
    enumerable: true,
    value: constants
  },


The rest of the file does not use trailing commas, please remove this. I don't want to start any new discussions about trailing commas, but I'd go for consistency in this case.

The line right above doesn't have a trailing comma either, but @bnoordhuis suggests we use trailing commas moving forwards?

Idk, sounds reasonable to just add them wherever you go.

Are you referring to #20703 (comment)? ESLint still doesn't have a file-by-file rule as far as I know, so that doesn't really apply here. Personally, I am not a fan of dangling commas, and we discussed that topic numerous times (e.g. #19131 (comment)). I totally understand Ben's reasoning, but this kind of "churn" isn't that common from my perspective, correct me if I am wrong.

^{Edit: Striked my own comment because I promised not to start a new debate about trailing commas.}

Yeah, let's not. We should probably skip the comma for consistency, you're right.

Trott · 2018-06-06T21:06:04Z

semver-major so needs two TSC approvals (already has one) + CITGM before landing

vsemozhetbyt · 2018-06-06T21:28:24Z

doc/api/deprecations.md


-The [`crypto.createCredentials()`][] API is deprecated. Please use
+The [`crypto.createCredentials()`][] API was removed. Please use


It seems this link will not be valid after this PR, so we need to delinkify `crypto.createCredentials()` here and remove the bottom reference.

addaleax

LGTM, but only because of the existing runtime deprecation.

If we had had our process back then, this should just have gotten stuck at a documentation deprecation and a one-line alias.

vsemozhetbyt · 2018-06-24T10:09:58Z

@nodejs/crypto is this ready to land?

tniessen · 2018-06-24T22:42:15Z

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

apapirovski · 2018-06-25T06:13:21Z

@aduh95 could you have a look at the test failures please?

tniessen · 2018-06-25T09:22:38Z

You need to remove the Credentials line from test-crypto-classes.js.

aduh95 · 2018-06-25T09:38:02Z

@tniessen Thank you for spotting that, I'll take care of that this evening (that is to say in ten hours from my time zone).

The `crypto.Credentials` legacy API has been Runtime deprecated since v0.11.13 and users had been adviced to use `tls.SecureContext` instead. Fixes: nodejs#20793

vsemozhetbyt · 2018-06-26T16:59:07Z

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

mhdawson · 2018-06-26T19:04:15Z

@shigeki any concerns ?

vsemozhetbyt · 2018-07-09T19:53:41Z

@nodejs/crypto is this landable? Are there any concerns?

tniessen · 2018-07-10T11:23:23Z

I am fine with this change, I don't know of any potential breakage or downsides.

vsemozhetbyt · 2018-07-12T14:40:27Z

Let's land this tomorrow if there are no objections then.

vsemozhetbyt · 2018-07-13T14:49:25Z

Landed in d2ee7d6
Thank you!

The `crypto.Credentials` legacy API has been Runtime deprecated since v0.11.13 and users had been adviced to use `tls.SecureContext` instead. PR-URL: #21153 Fixes: #20793 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jun 5, 2018

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 5, 2018

targos reviewed Jun 5, 2018

View reviewed changes

mcollina approved these changes Jun 5, 2018

View reviewed changes

aduh95 force-pushed the fix-crypto-deprecation-warnings branch from 8163a85 to e0acbef Compare June 5, 2018 21:07

trivikr reviewed Jun 6, 2018

View reviewed changes

ryzokuken approved these changes Jun 6, 2018

View reviewed changes

tniessen reviewed Jun 6, 2018

View reviewed changes

aduh95 force-pushed the fix-crypto-deprecation-warnings branch from e0acbef to 966bdf8 Compare June 6, 2018 17:51

vsemozhetbyt reviewed Jun 6, 2018

View reviewed changes

vsemozhetbyt added the deprecations Issues and PRs related to deprecations. label Jun 6, 2018

aduh95 force-pushed the fix-crypto-deprecation-warnings branch from 966bdf8 to 29be62f Compare June 6, 2018 21:49

addaleax approved these changes Jun 7, 2018

View reviewed changes

trivikr approved these changes Jun 8, 2018

View reviewed changes

tniessen approved these changes Jun 8, 2018

View reviewed changes

TimothyGu approved these changes Jun 13, 2018

View reviewed changes

bnoordhuis approved these changes Jun 24, 2018

View reviewed changes

ryzokuken approved these changes Jun 24, 2018

View reviewed changes

jasnell approved these changes Jun 24, 2018

View reviewed changes

tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 24, 2018

apapirovski removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 25, 2018

crypto: remove deprecated legacy API

f758e89

The `crypto.Credentials` legacy API has been Runtime deprecated since v0.11.13 and users had been adviced to use `tls.SecureContext` instead. Fixes: nodejs#20793

aduh95 force-pushed the fix-crypto-deprecation-warnings branch from 29be62f to f758e89 Compare June 25, 2018 21:33

tniessen mentioned this pull request Jul 12, 2018

crypto: Provide name for anonymous function in crypto.js #21758

Closed

2 tasks

vsemozhetbyt closed this Jul 13, 2018

This was referenced Oct 2, 2018

crypto: set DEFAULT_ENCODING property to non-enumerable aduh95/node#1

Closed

crypto: set DEFAULT_ENCODING property to non-enumerable #23222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove crypto.Credentials API #21153

Remove crypto.Credentials API #21153

aduh95 commented Jun 5, 2018

targos Jun 5, 2018

targos commented Jun 5, 2018

mcollina left a comment

trivikr Jun 6, 2018

tniessen Jun 6, 2018

ryzokuken Jun 6, 2018

tniessen Jun 6, 2018 •

edited

Loading

ryzokuken Jun 6, 2018

Trott commented Jun 6, 2018

vsemozhetbyt Jun 6, 2018 •

edited

Loading

addaleax left a comment

vsemozhetbyt commented Jun 24, 2018

tniessen commented Jun 24, 2018

apapirovski commented Jun 25, 2018 •

edited

Loading

tniessen commented Jun 25, 2018

aduh95 commented Jun 25, 2018

vsemozhetbyt commented Jun 26, 2018

mhdawson commented Jun 26, 2018

vsemozhetbyt commented Jul 9, 2018

tniessen commented Jul 10, 2018

vsemozhetbyt commented Jul 12, 2018

vsemozhetbyt commented Jul 13, 2018


		The [`crypto.createCredentials()`][] API is deprecated. Please use
		The [`crypto.createCredentials()`][] API has been removed. Please use

Remove crypto.Credentials API #21153

Remove crypto.Credentials API #21153

Conversation

aduh95 commented Jun 5, 2018

Checklist

targos Jun 5, 2018

Choose a reason for hiding this comment

targos commented Jun 5, 2018

mcollina left a comment

Choose a reason for hiding this comment

trivikr Jun 6, 2018

Choose a reason for hiding this comment

tniessen Jun 6, 2018

Choose a reason for hiding this comment

ryzokuken Jun 6, 2018

Choose a reason for hiding this comment

tniessen Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

ryzokuken Jun 6, 2018

Choose a reason for hiding this comment

Trott commented Jun 6, 2018

vsemozhetbyt Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

vsemozhetbyt commented Jun 24, 2018

tniessen commented Jun 24, 2018

apapirovski commented Jun 25, 2018 • edited Loading

tniessen commented Jun 25, 2018

aduh95 commented Jun 25, 2018

vsemozhetbyt commented Jun 26, 2018

mhdawson commented Jun 26, 2018

vsemozhetbyt commented Jul 9, 2018

tniessen commented Jul 10, 2018

vsemozhetbyt commented Jul 12, 2018

vsemozhetbyt commented Jul 13, 2018

tniessen Jun 6, 2018 •

edited

Loading

vsemozhetbyt Jun 6, 2018 •

edited

Loading

apapirovski commented Jun 25, 2018 •

edited

Loading