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

Add End-of-Life deprecation on crypto.Credentials #20793

Closed
aduh95 opened this issue May 16, 2018 · 7 comments
Closed

Add End-of-Life deprecation on crypto.Credentials #20793

aduh95 opened this issue May 16, 2018 · 7 comments
Labels
crypto Issues and PRs related to the crypto subsystem. esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@aduh95
Copy link
Contributor

aduh95 commented May 16, 2018

  • Version: v11.0.0-nightly201805162b8cd93246
  • Platform: all
  • Subsystem: crypto

Back in v0.11.13 according to the docs, crypto.createCredentials and crypto.Credentials have been deprecated (DEP0010 and DEP0011) in favor of tls.createSecureContext . I am not familiar with these functions, but I am willing to bet it's been enough time since the deprecation to consider legitimately to move the deprecation to End-of-Life and talk about removing them.

I am raising this issue because of the warnings I got when I dynamically import the crypto module:

$ echo "import('crypto')" > ./test.mjs
$ node --experimental-modules --trace-warnings ./test.mjs
(node:30509) ExperimentalWarning: The ESM module loader is experimental.
    at startup (internal/bootstrap/node.js:115:17)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:577:3)
(node:30509) [DEP0091] DeprecationWarning: crypto.DEFAULT_ENCODING is deprecated.
    at createDynamicModule (internal/modules/esm/translators.js:71:48)
    at setExecutor (internal/modules/esm/create_dynamic_module.js:50:23)
    at node:crypto:11:5
    at ModuleJob.run (internal/modules/esm/module_job.js:106:14)
(node:30509) [DEP0010] DeprecationWarning: crypto.createCredentials is deprecated. Use tls.createSecureContext instead.
    at createDynamicModule (internal/modules/esm/translators.js:71:48)
    at setExecutor (internal/modules/esm/create_dynamic_module.js:50:23)
    at node:crypto:11:5
    at ModuleJob.run (internal/modules/esm/module_job.js:106:14)
(node:30509) [DEP0011] DeprecationWarning: crypto.Credentials is deprecated. Use tls.SecureContext instead.
    at createDynamicModule (internal/modules/esm/translators.js:71:48)
    at setExecutor (internal/modules/esm/create_dynamic_module.js:50:23)
    at node:crypto:11:5
    at ModuleJob.run (internal/modules/esm/module_job.js:106:14)

crypto.DEFAULT_ENCODING has been deprecated in Node 10, so one could argue it is too soon to break it. However, we might want to find a way to get rid of the deprecation warning before ES modules land without a flag ― core module raising warnings on load looks quite messy, I'd say.

@hiroppy hiroppy added the crypto Issues and PRs related to the crypto subsystem. label May 17, 2018
@Trott
Copy link
Member

Trott commented May 17, 2018

@nodejs/crypto @nodejs/modules

@Trott Trott added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 17, 2018
@ljharb
Copy link
Member

ljharb commented May 17, 2018

I think regardless, we could ensure that the deprecated items aren’t in the ESM implementation?

@jdalton
Copy link
Member

jdalton commented May 17, 2018

@ljharb There's a process in place for deprecation and removal of APIs. Following that process in CJS as well as ESM allows for things to shake out naturally without maintaining multiple lists or applying extra filters for things.

One way to approach deprecated APIs to avoid runtime warnings for things like dynamic imports is to make the deprecated API non-enumerable. This is what has been done in other areas like the experimental fs.promises method to avoid user-land issues (#20632).

@aduh95
Copy link
Contributor Author

aduh95 commented May 17, 2018

Making deprecated API non-enumerable make the warnings disappear indeed, I can make a PR for that.

Still, it will be a breaking change and won't land until Node v11 (I believe), which is why I think it may be a good time to talk about retiring those deprecated APIs. @jdalton Where can I find the process you are referring to? Would it allow such APIs to be removed in the near future or is it a silly thing to say?

@jdalton
Copy link
Member

jdalton commented May 17, 2018

@aduh95

Where can I find the process you are referring to?

Here's a doc explaining various types of deprecations. Doc -> Runtime Warning -> Removal.

Would it allow such APIs to be removed in the near future or is it a silly thing to say?

Major version releases are the time for removals for sure.
For this specific one we should cc the @nodejs/crypto team.

@Trott
Copy link
Member

Trott commented May 17, 2018

@aduh95 In addition to the doc linked by @jdalton, there is useful information in the Node.js Collaborator Guide in two different sections:

aduh95 added a commit to aduh95/node that referenced this issue Jun 25, 2018
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
@jdalton jdalton added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jul 13, 2018
@hktalent

This comment has been minimized.

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. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

No branches or pull requests

6 participants