Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: runtime deprecate DEFAULT_ENCODING #18333

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 24, 2018

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

@jasnell 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 nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jan 24, 2018
@jasnell jasnell requested a review from a team January 24, 2018 00:03
@addaleax
Copy link
Member

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.

@jasnell
Copy link
Member Author

jasnell commented Jan 24, 2018

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

@addaleax
Copy link
Member

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

@jasnell
Copy link
Member Author

jasnell commented Jan 24, 2018

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?

@@ -831,6 +840,7 @@ is not included in this list will be considered invalid in compliance with
[`console.error()`]: console.html#console_console_error_data_args
[`console.log()`]: console.html#console_console_log_data_args
[`crypto.createCredentials()`]: crypto.html#crypto_crypto_createcredentials_details
[`crypto.DEFAULT_ENCODING`]: crypto.html#crypto_crypto_defaultencoding
Copy link
Contributor

Choose a reason for hiding this comment

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

#crypto_crypto_defaultencoding -> #crypto_crypto_default_encoding?

Copy link
Member

@ChALkeR ChALkeR Jan 25, 2018

Choose a reason for hiding this comment

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

@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 .


The [`crypto.DEFAULT_ENCODING`][] property has been deprecated and replaced
with `crypto.setDefaultEncoding()` and `crypto.getDefaultEncoding()`
alternatives.
Copy link
Member

@Trott Trott Jan 24, 2018

Choose a reason for hiding this comment

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

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.

@jasnell jasnell added this to the 10.0.0 milestone Jan 24, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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

@mcollina
Copy link
Member

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.

@devsnek
Copy link
Member

devsnek commented Jan 24, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Jan 24, 2018

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?

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

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.

@ChALkeR
Copy link
Member

ChALkeR commented Jan 24, 2018

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

@jasnell jasnell changed the title crypto: docs-only deprecate DEFAULT_ENCODING, replace crypto: runtime deprecate DEFAULT_ENCODING Jan 24, 2018
@jasnell
Copy link
Member Author

jasnell commented Jan 24, 2018

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

is passed to the callback:
The `crypto.DEFAULT_ENCODING` property can be used to change the way the
`derivedKey` is passed to the callback. This property, however, has been
deprecated and use should be avoideed.
Copy link
Member

Choose a reason for hiding this comment

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

avoided

is returned:
The `crypto.DEFAULT_ENCODING` property may be used to change the way the
`derivedKey` is returned. This property, however, has been deprecated and use
should be avoideed.
Copy link
Member

Choose a reason for hiding this comment

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

ditto

lib/crypto.js Outdated
@@ -202,11 +202,14 @@ Object.defineProperties(exports, {
set: !fipsMode ? setFipsDisabled :
fipsForced ? setFipsForced : setFipsCrypto
},
// DEFAULT_ENCODING is documentation-only deprecated: DEP00XX
Copy link
Member

Choose a reason for hiding this comment

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

remove this line?

const enc = normalizeEncoding(val);
if (enc === undefined)
throw new errors.TypeError('ERR_UNKNOWN_ENCODING', val);
defaultEncoding = enc;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right, yeah. missed this.

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member Author

jasnell commented Jan 26, 2018

@jasnell
Copy link
Member Author

jasnell commented Jan 26, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Jan 27, 2018

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
@jasnell
Copy link
Member Author

jasnell commented Jan 31, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Feb 1, 2018

Build bot failures on multiple CI bots but otherwise ok.

jasnell added a commit that referenced this pull request Feb 1, 2018
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 <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Feb 1, 2018

landed in 6035bee

@jasnell jasnell closed this Feb 1, 2018
jasnell added a commit to jasnell/node that referenced this pull request Feb 1, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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 <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
tniessen added a commit to tniessen/node that referenced this pull request Mar 21, 2023
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
tniessen added a commit to tniessen/node that referenced this pull request Mar 21, 2023
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
nodejs-github-bot pushed a commit that referenced this pull request Mar 26, 2023
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.