lib: add options to util.deprecate

[RafaelGSS]

It seems this was missed in the PRs that added both options.

---

cc: @legendecas

[legendecas]

#44711 didn't add a test case for sync emit deprecation. I believe the original intention was that the new option is an internal option.

I also personally believe that the public api should keep the signature util.deprecate(fn, msg[, code]) as is.

[RafaelGSS]

#44711 didn't add a test case for sync emit deprecation. I believe the original intention was that the new option is an internal option.

I also personally believe that the public api should keep the signature util.deprecate(fn, msg[, code]) as is.

Currently, they aren't. We are exposing the same internalUtil.deprecate to util.deprecate. We could consider change that, but that's how the state of the art is.

[legendecas]

Currently, they aren't. We are exposing the same internalUtil.deprecate to util.deprecate. We could consider change that, but that's how the state of the art is.

yes, you are right that internalUtil.deprecate is exposed as util.deprecate directly. Given that these positional arguments were never documented, I'm suggesting that we should change it.

[RafaelGSS]

@legendecas the modifyPrototype specifically I'd like to have it exposed as it reduces a performance bottleneck the util.deprecate does to the deprecated object.

Since I was about to expose the modifyPrototype, I thought it might be interesting to expose the emitSync, too, as it might be valuable to other users.

What's your suggestion? Exposing both or exposing just modifyPrototype?

[legendecas]

We need test coverages. I searched the test/ directory for deprecate tests, and I didn't find any coverage for emitSync and modifyPrototype.

I'd think a public API exposing these options in an option bag could be future proof. Instead of positional args.

[RafaelGSS]

I concur.

I will adjust that.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.54%. Comparing base (c9578dc) to head (5ae64ea).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59982      +/-   ##
==========================================
- Coverage   88.55%   88.54%   -0.02%     
==========================================
  Files         704      704              
  Lines      207989   207994       +5     
  Branches    40068    40076       +8     
==========================================
- Hits       184180   184158      -22     
- Misses      15841    15855      +14     
- Partials     7968     7981      +13     

Files with missing lines	Coverage Δ
lib/util.js	97.97% <100.00%> (+0.01%)	⬆️

... and 29 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.

[ljharb]

doesn't this mean it's a breaking change?

[legendecas]

These are not a documented argument. Do you have a package in mind that will be broken by this undocumented behavior?

[ljharb]

Nope, but whether it's documented or not generally has no effect on whether something is theoretically breaking.

[legendecas]

We would not consider fixing an undocumented behavior as an effective breaking change if the impact is trivial. This is one of the main reason that we need a public API contract such as documentation.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[RafaelGSS]

I had to rebase on main as OSX was failing constantly.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in cfa11ba