crypto: docs-only deprecate crypto.fips, replace by jasnell · Pull Request #18335 · nodejs/node

jasnell · 2018-01-24T00:35:13Z

Docs-only deprecate the getter/setter crypto.fips and replace with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support
Refs: #18131

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)

crypto

Trott · 2018-01-24T00:37:51Z

I prefer more direct wording:
The [`crypto.fips`][] property is deprecated. Please use `crypto.setFips()` and `crypto.getFips()`.

vsemozhetbyt · 2018-01-24T00:47:13Z

Typo sentence?

vsemozhetbyt · 2018-01-24T00:50:13Z

#crypto_crypto.fips -> #crypto_crypto_fips?

cjihrig · 2018-01-24T00:51:17Z

No need for the parentheses.

mcollina

LGTM

jasnell · 2018-01-24T15:29:12Z

Yep. That would be trivial. We are also allowed to reclassify docs only deprecations as semver-major if TSC agrees

…

On Jan 24, 2018 07:27, "Jordan Harband" ***@***.***> wrote: Even though the deprecation might require semver-major, could the new additions be added in a semver-minor and backported? That would maximize the ease of migration. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#18335 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eSaDmLSCYAKOUwzQTU6o8veytQygks5tN0v0gaJpZM4Rqim2> .

jasnell · 2018-01-26T18:41:51Z

Ping @nodejs/tsc ... I'd like to treat this docs-only deprecation as semver-minor. Any objections?

jasnell · 2018-01-26T18:42:42Z

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

ChALkeR · 2018-01-28T00:34:42Z

+1 for semver-minor for doc-only here.

Perhaps there should also be a pendingDeprecation-guarded runtime warning?

Upd: opened #18417.

jasnell · 2018-02-01T01:39:52Z

@ChALkeR ... let's add the --pending-deprecation warning after #18417 lands. This shouldn't have to wait for that tho.

ChALkeR · 2018-02-01T01:41:36Z

@jasnell #18417 is an issue, #18433 landed already.

jasnell · 2018-02-01T01:55:51Z

oh! I missed that... think I mentally mixed #18433 and #18417 and completely missed that it had landed already :-)

ChALkeR · 2018-02-01T02:03:22Z

Merge conflict (here and below)

Ugh. Forgot to save

BridgeAR · 2018-02-01T09:57:30Z

I changed it to semver-minor as there was enough time to speak up against it and there was only a voice pro.

mhdawson

LGTM

jasnell · 2018-02-01T15:29:30Z

Note: I will land this after I get #18492 landed.

Docs-only deprecate the getter/setter crypto.fips and replace with crypto.setFips() and crypto.getFips() This is specifically in preparation for ESM module support Refs: nodejs#18131

jasnell · 2018-02-01T16:19:21Z

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

Docs-only deprecate the getter/setter crypto.fips and replace with crypto.setFips() and crypto.getFips() This is specifically in preparation for ESM module support PR-URL: #18335 Refs: #18131 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

jasnell · 2018-02-02T15:57:12Z

Landed in 6e7992e

MylesBorins · 2018-02-20T23:32:08Z

This does not land cleanly on v9.x

This is because other deprecations have landed on master that have been given codes. If we are going to land this as semver minor would someone be willing to backport and assign an appropriate code to make sure we don't double dip

Docs-only deprecate the getter/setter crypto.fips and replace with crypto.setFips() and crypto.getFips() This is specifically in preparation for ESM module support PR-URL: nodejs#18335 Refs: nodejs#18131 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

jasnell added crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 24, 2018

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

jasnell requested a review from a teamJanuary 24, 2018 00:35

Trott reviewed Jan 24, 2018
View reviewed changes

addaleax approved these changes Jan 24, 2018
View reviewed changes

jasnell added this to the 10.0.0 milestone Jan 24, 2018

vsemozhetbyt reviewed Jan 24, 2018
View reviewed changes

Comment threaddoc/api/crypto.md Outdated
vsemozhetbytJan 24, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo sentence?
jasnell reacted with thumbs up emoji

vsemozhetbyt reviewed Jan 24, 2018
View reviewed changes

Comment threaddoc/api/deprecations.md Outdated
vsemozhetbytJan 24, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#crypto_crypto.fips -> #crypto_crypto_fips?

cjihrig approved these changes Jan 24, 2018
View reviewed changes

Comment threaddoc/api/crypto.md Outdated
cjihrigJan 24, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the parentheses.
jasnell reacted with thumbs up emoji

mcollina approved these changes Jan 24, 2018
View reviewed changes

guybedford approved these changes Jan 24, 2018
View reviewed changes

maclover7 approved these changes Jan 26, 2018
View reviewed changes

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

maclover7 force-pushed the master branch from bb5575a to 993b716Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952Compare January 26, 2018 22:36

jasnell force-pushed the crypto-replace-crypto.fips branch from 54bd37a to 877e1c9Compare February 1, 2018 01:48

ChALkeR reviewed Feb 1, 2018
View reviewed changes

mhdawson approved these changes Feb 1, 2018
View reviewed changes

jasnell force-pushed the crypto-replace-crypto.fips branch from d488810 to 900bf45Compare February 1, 2018 15:25

BridgeAR approved these changes Feb 1, 2018
View reviewed changes

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

crypto: docs-only deprecate crypto.fips, replace
1dece49
Docs-only deprecate the getter/setter crypto.fips and replace with crypto.setFips() and crypto.getFips() This is specifically in preparation for ESM module support Refs: nodejs#18131

jasnell force-pushed the crypto-replace-crypto.fips branch from 900bf45 to 1dece49Compare February 1, 2018 16:18

jasnell closed this Feb 2, 2018

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

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

devsnek mentioned this pull request Mar 15, 2018
events: deprecate EventEmitter.defaultMaxListeners getter/setter #18712
Closed
4 tasks

MylesBorins added the dont-land-on-v8.x label Aug 7, 2018

Uh oh!

Uh oh!

Conversation

jasnell commented Jan 24, 2018

Checklist

Affected core subsystem(s)

Uh oh!

TrottJan 24, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytJan 24, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytJan 24, 2018

Choose a reason for hiding this comment

Uh oh!

cjihrigJan 24, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 24, 2018 via email

Uh oh!

jasnell commented Jan 26, 2018

Uh oh!

jasnell commented Jan 26, 2018

Uh oh!

ChALkeR commented Jan 28, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Feb 1, 2018

Uh oh!

ChALkeR commented Feb 1, 2018

Uh oh!

jasnell commented Feb 1, 2018

Uh oh!

ChALkeRFeb 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnellFeb 1, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 1, 2018

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 1, 2018

Uh oh!

jasnell commented Feb 1, 2018

Uh oh!

jasnell commented Feb 2, 2018

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

ChALkeR commented Jan 28, 2018•
edited
Loading

ChALkeRFeb 1, 2018•
edited
Loading