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

zlib: deprecate classes usage without new #55718

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Nov 4, 2024

Since we documentation-only deprecated the usage without new qualifier, we can make it runtime deprecated.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib subsystem. labels Nov 4, 2024
@RedYetiDev RedYetiDev added semver-major PRs that contain breaking changes and should be released in the next major version. notable-change PRs with changes that should be highlighted in changelogs. labels Nov 4, 2024
Copy link
Contributor

github-actions bot commented Nov 4, 2024

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @RedYetiDev.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

doc/api/deprecations.md Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev added the deprecations Issues and PRs related to deprecations. label Nov 4, 2024
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (5d85b05) to head (245e4f3).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55718      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         654      654              
  Lines      187763   187775      +12     
  Branches    36135    36137       +2     
==========================================
+ Hits       166008   166016       +8     
- Misses      14996    15003       +7     
+ Partials     6759     6756       -3     
Files with missing lines Coverage Δ
lib/zlib.js 98.48% <100.00%> (+0.01%) ⬆️

... and 25 files with indirect coverage changes

Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

We usually add a test, to check warning is emitted correctly

lib/zlib.js Outdated
Comment on lines 690 to 691
process.emitWarning(`Instantiating Deflate class without 'new' is deprecated.`,
'DeprecationWarning', 'DEP0184');
Copy link
Member

Choose a reason for hiding this comment

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

FWIW when #54869 lands there will be a helper utility for this warning

Copy link
Member

Choose a reason for hiding this comment

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

Now that that PR has landed, I recommend using the new utility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants