Skip to content

Conversation

haramj
Copy link
Contributor

@haramj haramj commented Sep 4, 2025

Update deprecated() in lib/internal/util.js to check
getOptionValue('--no-deprecation') in addition to process.noDeprecation.

No test added yet. Suggested tests:

  1. Warning suppressed when process.noDeprecation is true.
  2. Warning suppressed when --no-deprecation CLI flag is set.
  3. Warning emitted when neither flag is set.

Feedback on how to best add these tests is welcome.

TODO (joyeecheung): use getOptionValue('--no-deprecation') instead.
Would appreciate a review whenever you have time. Thanks!
/cc @joyeecheung

->

This PR only removes the TODO comment in lib/internal/util.js
that suggested replacing process.noDeprecation with
getOptionValue('--no-deprecation').

As noted in review, changing the behavior could break more than expected,
so we keep the current logic as-is and just clean up the comment.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Sep 4, 2025
Copy link

codecov bot commented Sep 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.95%. Comparing base (6428e2e) to head (f35aec5).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59760      +/-   ##
==========================================
- Coverage   89.95%   89.95%   -0.01%     
==========================================
  Files         667      667              
  Lines      196813   196812       -1     
  Branches    38425    38422       -3     
==========================================
- Hits       177038   177036       -2     
+ Misses      12200    12199       -1     
- Partials     7575     7577       +2     
Files with missing lines Coverage Δ
lib/internal/util.js 96.17% <ø> (-0.01%) ⬇️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@joyeecheung
Copy link
Member

I think at this point, I would prefer to just leave it as-is and just remove that TODO - I feel that changing it could otherwise break more than I expected, based on some simple GitHub searches.

@haramj
Copy link
Contributor Author

haramj commented Sep 5, 2025

I think at this point, I would prefer to just leave it as-is and just remove that TODO - I feel that changing it could otherwise break more than I expected, based on some simple GitHub searches.

I’ll update the PR to just remove the TODO comment.

@haramj haramj changed the title util: use getOptionValue('--no-deprecation') in deprecated() util: remove outdated TODO comment Sep 5, 2025
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2025
@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 6, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 7, 2025

@jasnell jasnell added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 7, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 7, 2025
@nodejs-github-bot
Copy link
Collaborator

Landed in 6478dd0...3837993

nodejs-github-bot pushed a commit that referenced this pull request Sep 7, 2025
PR-URL: #59760
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Sep 7, 2025
PR-URL: #59760
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants