-
Notifications
You must be signed in to change notification settings - Fork 29.5k
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
repl: don't use deprecated domain
module
#55204
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55204 +/- ##
==========================================
- Coverage 88.41% 88.40% -0.02%
==========================================
Files 652 653 +1
Lines 186918 187451 +533
Branches 36072 36082 +10
==========================================
+ Hits 165270 165718 +448
- Misses 14891 14965 +74
- Partials 6757 6768 +11
|
42e6441
to
fb9057d
Compare
47e132b
to
9be6f17
Compare
9be6f17
to
5f6d5ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a rough feeling that you'd need to add
a FinalizationRegistry
to clear up any instances
Why? Is unsetting the callback on |
5f6d5ec
to
dcf5e94
Compare
|
||
if (!kHasSetUncaughtListener) { | ||
kAsyncHook.enable(); | ||
process.on('uncaughtException', handleUncaughtException); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still leaking things in the process object, and domain is a much safer way to do so anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and domain is a much safer way to do so anyway.
I disagree. This is different than the domain. The domain bypasses the listener and inserts a callback before these listeners can even be called. This merely adds a listener, and later removes it. How is that "leaking"? The attachment to the process object is controlled.
This PR refactors the REPL module by eliminating the deprecated
node:domain
module. It replaces its functionality with a modern approach using atry/catch
block andprocess.setUncaughtExceptionCaptureCallback
.Why?
The
domain
module has been deprecated for a while, and its use is discouraged. Despite this, Node.js still relies on it in a key component: the REPL. To lead by example and promote best practices, this PR updates the REPL module to use more up-to-date error-handling mechanisms, completely removing reliance on the deprecateddomain
module.Additionally, this update simplifies the REPL's code and results in a slight performance improvement—though the gains are give-or-take a few ticks and may not be noticeable.
Breaking Changes
This PR is marked as semver-majorPRs that contain breaking changes and should be released in the next major version.
because it introduces functional changes to the REPL:
options.domain
: This undocumented feature is removed (without a formal deprecation cycle, given thedomain
module itself is deprecated, and the feature was never documented nor tested).ERR_INVALID_REPL_INPUT
: This error is no longer needed, as after this change, there are no restricted inputs.This PR is also marked needs-citgmPRs that need a CITGM CI run.
in case any of the above cause breakages.
CC @nodejs/replPRs that contain breaking changes and should be released in the next major version.
CC @nodejs/tsc due to semver-major