Skip to content

lib: runtime deprecate calling Cipheriv(...) and Decipheriv(...) without new - #57268

Closed
jasnell wants to merge 3 commits into
nodejs:mainfrom
jasnell:jasnell/deprecate-cipher-decipher-without-new
Closed

lib: runtime deprecate calling Cipheriv(...) and Decipheriv(...) without new#57268
jasnell wants to merge 3 commits into
nodejs:mainfrom
jasnell:jasnell/deprecate-cipher-decipher-without-new

Conversation

@jasnell

Copy link
Copy Markdown
Member

The first commit here is from #57266 which is expected to land first. This PR is about the second commit... deprecating calling Cipheriv(...) and Decipheriv(...) without the new keyword. Technically we should probably also deprecate creating these directly even with the new keyword since we tell people to use createCipheriv(...) and createDecipheriv(...) to create these (both of which simply defer to calling new ... but still).

I did do a github code search trying to find any examples of folks calling these directly without the new keyword and there were zero hits suggesting that going straight to a runtime deprecation is quite safe.

/cc @nodejs/tsc

Cipher was removed from the public API a while ago but we were still
defining it and exporting `require('crypto').Cipher` as `undefined`.
Silly us.
@jasnelljasnell 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. deprecations Issues and PRs related to deprecations. labels Mar 2, 2025
@jasnell
jasnell requested review from anonrig and mcollinaMarch 2, 2025 01:08
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/web-infra

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Mar 2, 2025
Comment threaddoc/api/deprecations.md Outdated
@codecov

codecovBot commented Mar 2, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (6b0af17) to head (1bb34d4).
Report is 24 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57268 +/- ##
==========================================
- Coverage 90.24% 90.24% -0.01% 
==========================================
Files 630 630 Lines 184908 184883 -25 Branches 36181 36175 -6 ==========================================
- Hits 166874 166848 -26 + Misses 11061 11055 -6 - Partials 6973 6980 +7 
Files with missing linesCoverage Δ
lib/crypto.js92.74% <ø> (-0.08%)⬇️
lib/internal/crypto/cipher.js97.65% <100.00%> (+2.34%)⬆️

... and 33 files with indirect coverage changes

Comment on lines +3843 to +3855
### DEP0190: Instantiating `node:crypto` `Cipheriv` and `Decipheriv` classes without `new`

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/57268
description: Runtime deprecation.
-->

Type: Runtime

Instantiating the [`Cipheriv`][] and [`Decipheriv`][] classes exported by the `node:crypto`
module is deprecated.

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.

Can you do a first PR that doc-deprecates it, so that can be backported to existing release lines?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I've been unable to find any examples anywhere of anyone actually calling these (at least in any open source code). I'm not sure there's an actual value in backporting a doc-only deprecation to older lines.

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.

It simplifies backporting of other deprecations, as it's otherwise likely we would get conflicts. I'm happy to open the doc-only PR myself if you prefer

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@aduh95 ... let's open those as separate backport PRs after landing this is main.

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.

Why not follow the usual process? That'd be much less work

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@jasnelljasnell closed this Mar 4, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cryptoIssues and PRs related to the crypto subsystem.deprecationsIssues and PRs related to deprecations.docIssues and PRs related to the documentations.needs-ciPRs that need a full CI run.semver-majorPRs that contain breaking changes and should be released in the next major version.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jasnell@nodejs-github-bot@mcollina@anonrig@aduh95