Repository navigation
refactor!: remove unused legacy errors.js functions - #5835
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes a legacy process.emitWarning polyfill in lib/errors.js, intending to rely solely on the native Node.js implementation now that Mocha’s minimum supported Node version is ^20.19.0 || >=22.12.0.
Changes:
- Remove the
emitWarningpolyfill wrapper fromlib/errors.jsand callprocess.emitWarningdirectly fromdeprecate()andwarn(). - Simplify unit tests by always stubbing
process.emitWarningintest/unit/errors.spec.js.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/errors.js | Removes the emitWarning wrapper and calls process.emitWarning directly for warnings/deprecations. |
| test/unit/errors.spec.js | Simplifies setup/teardown around process.emitWarning stubbing in deprecate/warn tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5835 +/- ##
=======================================
Coverage 88.55% 88.56%
=======================================
Files 65 65
Lines 4659 4643 -16
Branches 978 974 -4
=======================================
- Hits 4126 4112 -14
+ Misses 533 531 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f05cc93 to
0f77459
Compare
process.emitWarning polyfillerrors.js functions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
test/unit/suite.spec.js:533
- The removed
errors.deprecatestub means theerrorsimport at the top of this file is now unused (only appears in the require). With ESLintjs/recommended, this will trigger ano-unused-varsfailure; please remove the unusederrorsimport.
describe("constructor", function () {
/* eslint no-new: off */
it("should throw an error if the title isn't a string", function () {
expect(function () {
new Suite(undefined, "root");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- `warn()` is no longer used anywhere - `createInvalidPluginError()` is no longer used anywhere and has been deprecated since #4360, for 6 years now - `deprecate()` was only used by `createInvalidPluginError()` - `emitWarning()` was only used by `warn()` and `deprecate()` and contained an unnecessary polyfill for `process.emitWarning`, a function that has existed since Node 6.0.0 with us now requiring at least Node 20.19.0 Ref: #5357 (v12 Release Plan)
0f77459 to
29de573
Compare
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
LGTM, with the caveat that removing createInvalidPluginError is actually a breaking change and this PR should be rephrased as such.
Can I suggest manually writing the PR description?
|
@JoshuaKGoldberg I did manually write the majority of the final PR description. As far as I can see the |
|
errors.js functionserrors.js functions
|
@JoshuaKGoldberg I'm good to merge this into v12 if you are now that we've updated #5357. One of the callouts is explicitly "removing long-deprecated options" :) |
|
I will fix the formatting issue separately |
|
@JoshuaKGoldberg you OK to merge this into v12? |
mark-wiemer
left a comment
There was a problem hiding this comment.
For the record I did review the actual code :)
|
At this point I don't feel strongly. If you guys are ok with breaking the letter of the law to go with this interpretation of the spirit, I won't stand in the way. ✅ |
|
#5357 says "Mocha 12 will be used to ... Remove long-deprecated options and any other known dead code", I think this counts :) |
Removes these from
errors.js:warn()is no longer used anywhere and not publicly exportedcreateInvalidPluginError()is no longer used anywhere,isn't publicly exportedhad to be explicitly imported from the specific file, has been deprecated since implementation of global setup/teardown; closes #4308 #4360, for 6 years now and has no public usage across GitHubdeprecate()was only used bycreateInvalidPluginError()and not publicly exportedemitWarning()was only used bywarn()anddeprecate()and contained an unnecessary polyfill forprocess.emitWarning, a function that has existed since Node 6.0.0 with us now requiring at least Node 20.19.0 - and it wasn't publicly exported.This change simplifies the codebase by eliminating unused functions and their tests as well as their redundant polyfills.
PR Checklist
status: accepting prsApplicable issues