crypto: runtime deprecate DEFAULT_ENCODING by jasnell · Pull Request #18333 · nodejs/node · GitHub

Docs-only Runtime deprecate the crypto.DEFAULT_ENCODING and replace by directly exposing the getDefaultEncoding/setDefaultEncoding functions that are used as it's getter and setter.

This is specifically in preparation for eventual ESM 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

I am reluctant to introduce new APIs for a legacy feature; this is really just here to support old applications (pre-2012!).

Like, I’m totally 👍 on the deprecation. But if you want to retrieve a string in a specific encoding, you should pass that option explicitly when calling then function anyway.

So I know, is that -1 reluctant or -0 reluctant?

@jasnell That is -1 unless there’s a good reason that I don’t see, which is totally possible.

Ok. Fwiw, these methods already exist as the getter/setter. The only additional cost is the overhead of introducing new public API. Let's see how others feel about it, ok?

#crypto_crypto_defaultencoding -> #crypto_crypto_default_encoding?

@jasnell This wasn't fixed in the «nits»-fix commit ;-)

https://nodejs.org/api/crypto.html#crypto_crypto_default_encoding is the actual link, so it indeed should be crypto.html#crypto_crypto_default_encoding .

Can we keep this wording consistent with other messages by changing it to this?:

The [`crypto.DEFAULT_ENCODING`][] property is deprecated. Please use
`crypto.getDefaultEncoding()` and `crypto.setDefaultEncoding()` instead.

Just marking this with the red X so my objection doesn’t get lost

I would prefer we deprecate this and remove it in 11. Unless this feature is needed by some very popular module, in which case we might have to do what you propose.

fwiw if its needed by some very popular module we can just ping the author and ask them to refactor the calls

Ok, so that brings up the next question: instead of docs-deprecation-with-replacement, should we just go with a full runtime deprecation of the property and not worry about replacing it?

This property was added 5 years ago as a temporary measure, documented for future deprecation in 2015 (in 6.0 release). I'm pretty sure we don't want to introduce a new API around it, unless there is some valid usecase that we would want to support and are missing.

+1 to doc or runtime deprecation.

The usage seems to be really low, I'm for runtime-deprecation.

Updated to use a runtime deprecation with no new API exposed.

avoided

ditto

remove this line?

this change does not seem to be directly related to the deprecation

This should probably go into a separate PR if still wanted.

Ah, right, yeah. missed this.

Updated.

LGTM with already mentioned nits and undoing (or explaining) changes in util.js.

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/12758/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1236/

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

CI looks good, although there is an issue with the arm build bots

Runtime deprecate the crypto.DEFAULT_ENCODING property. This is specifically in preparation for eventual ESM support Refs: nodejs#18131

New CI, just to be safe: https://ci.nodejs.org/job/node-test-pull-request/12863/

Build bot failures on multiple CI bots but otherwise ok.

Runtime deprecate the crypto.DEFAULT_ENCODING property. This is specifically in preparation for eventual ESM support Refs: #18131 PR-URL: #18333 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

landed in 6035bee

Missed when landing nodejs#18333

Runtime deprecate the crypto.DEFAULT_ENCODING property. This is specifically in preparation for eventual ESM support Refs: nodejs#18131 PR-URL: nodejs#18333 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Citing 76b0bdf from 2012, "only use this as a temporary measure." Getting or setting DEFAULT_ENCODING has emitted a warning ever since Node.js 10, so it seems appropriate to remove it in Node.js 20 five years later. The last Node.js version that did not emit a warning reached its end-of-life status at the end of 2019. This commit only removes the public API so that the change can land in time for Node.js 20. Refs: nodejs/node-v0.x-archive#4179 Refs: nodejs#18333

Citing 76b0bdf from 2012, "only use this as a temporary measure." Getting or setting DEFAULT_ENCODING has emitted a warning ever since Node.js 10, so it seems appropriate to remove it in Node.js 20 five years later. The last Node.js version that did not emit a warning reached its end-of-life status at the end of 2019. This commit only removes the public API so that the change can land in time for Node.js 20. Refs: nodejs/node-v0.x-archive#4179 Refs: #18333 PR-URL: #47182 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com> Reviewed-By: Erick Wendel <erick.workspace@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

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 team January 24, 2018 00:03

vsemozhetbyt reviewed Jan 24, 2018

View reviewed changes

Trott reviewed Jan 24, 2018

View reviewed changes

jasnell added this to the 10.0.0 milestone Jan 24, 2018

addaleax requested changes Jan 24, 2018

View reviewed changes

ChALkeR previously requested changes Jan 24, 2018

View reviewed changes

jasnell force-pushed the refactor-default-encoding branch from 1dd37a1 to dbb4e9b Compare January 24, 2018 18:35

jasnell changed the title crypto: docs-only deprecate DEFAULT_ENCODING, replace crypto: runtime deprecate DEFAULT_ENCODING Jan 24, 2018

jasnell force-pushed the refactor-default-encoding branch from dbb4e9b to 144ab92 Compare January 24, 2018 18:40

addaleax approved these changes Jan 24, 2018

View reviewed changes

targos reviewed Jan 24, 2018

View reviewed changes

ChALkeR approved these changes Jan 24, 2018

View reviewed changes

mcollina approved these changes Jan 24, 2018

View reviewed changes

JungMinu approved these changes Jan 25, 2018

View reviewed changes

tniessen approved these changes Jan 25, 2018

View reviewed changes

jasnell force-pushed the refactor-default-encoding branch from 4736855 to 909643a Compare January 26, 2018 20:12

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

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

crypto: runtime deprecate DEFAULT_ENCODING

79a4683

Runtime deprecate the crypto.DEFAULT_ENCODING property. This is specifically in preparation for eventual ESM support Refs: nodejs#18131

jasnell force-pushed the refactor-default-encoding branch from 4bf301f to 79a4683 Compare January 31, 2018 23:35

jasnell closed this Feb 1, 2018

jasnell mentioned this pull request Feb 1, 2018

doc: fix unassigned deprecation codes #18492

Closed

4 tasks

jasnell added a commit to jasnell/node that referenced this pull request Feb 1, 2018

crypto: fix unassignd deprecation code

4b18fa1

Missed when landing nodejs#18333

tniessen mentioned this pull request Mar 21, 2023

crypto: remove DEFAULT_ENCODING #47182

Merged

crypto: runtime deprecate DEFAULT_ENCODING by jasnell · Pull Request #18333 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: runtime deprecate DEFAULT_ENCODING - #18333

crypto: runtime deprecate DEFAULT_ENCODING#18333jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:refactor-default-encodingjasnell/node:refactor-default-encodingCopy head branch name to clipboard

Conversation

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

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Jan 24, 2018

Uh oh!

jasnell commented Jan 24, 2018

Uh oh!

addaleax commented Jan 24, 2018

Uh oh!

jasnell commented Jan 24, 2018

Uh oh!

vsemozhetbyt Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

ChALkeR Jan 25, 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!

Trott Jan 24, 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!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jan 24, 2018

Uh oh!

devsnek commented Jan 24, 2018

Uh oh!

jasnell commented Jan 24, 2018

Uh oh!

ChALkeR left a comment • 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!

ChALkeR commented Jan 24, 2018

Uh oh!

jasnell commented Jan 24, 2018

Uh oh!

targos Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

targos Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

targos Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

targos Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

ChALkeR Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

ChALkeR left a comment • 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!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 26, 2018

Uh oh!

jasnell commented Jan 26, 2018

Uh oh!

jasnell commented Jan 27, 2018

Uh oh!

jasnell commented Jan 31, 2018

Uh oh!

crypto: runtime deprecate DEFAULT_ENCODING#18333
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:refactor-default-encoding

jasnell commented Jan 24, 2018 •
edited
Loading

ChALkeR Jan 25, 2018 •
edited
Loading

Trott Jan 24, 2018 •
edited
Loading

ChALkeR left a comment •
edited
Loading

ChALkeR left a comment •
edited
Loading