node
node copied to clipboard
repl: don't use deprecated `domain` module
This PR refactors the REPL module by eliminating the deprecated node:domain module. It replaces its functionality with a modern approach using a try/catch block and process listener.
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 deprecated domain 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 https://github.com/nodejs/node/labels/semver-major because it introduces functional changes to the REPL:
- Removal of
options.domain: This undocumented feature is removed (without a formal deprecation cycle, given thedomainmodule itself is deprecated, and the feature was never documented nor tested). - Removal of
ERR_INVALID_REPL_INPUT: This error is no longer needed, as after this change, there are no restricted inputs.
This PR is also marked https://github.com/nodejs/node/labels/needs-citgm in case any of the above cause breakages.
CC @nodejs/repl CC @nodejs/tsc due to https://github.com/nodejs/node/labels/semver-major
One test still needs updating.
Codecov Report
Attention: Patch coverage is 92.94118% with 6 lines in your changes missing coverage. Please review.
Project coverage is 88.42%. Comparing base (
fa8f149) to head (b0d6991). Report is 1144 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| lib/repl.js | 92.94% | 5 Missing and 1 partial :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #55204 +/- ##
========================================
Coverage 88.41% 88.42%
========================================
Files 652 654 +2
Lines 186918 187688 +770
Branches 36072 36126 +54
========================================
+ Hits 165270 165956 +686
- Misses 14891 14963 +72
- Partials 6757 6769 +12
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/internal/errors.js | 96.99% <ø> (-0.01%) |
:arrow_down: |
| lib/repl.js | 94.72% <92.94%> (-0.25%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
I have a rough feeling that you'd need to add a
FinalizationRegistryto clear up any instances
Why? Is unsetting the callback on close() not enough?
CI: https://ci.nodejs.org/job/node-test-pull-request/63292/
Can someone start a CITGM if they thinks it's necessary. IMO we can use the most recent main branch CITGM for comparison
Can someone start a CITGM if they thinks it's necessary. IMO we can use the most recent main branch CITGM for comparison
I doubt we have any package on CITGM that use repl, do we?
Got it! I've removed the label. IIRC semver-major PRs need two TSC approvals to land, would you mind reviewing?
CI: https://ci.nodejs.org/job/node-test-pull-request/63504/
CI: https://ci.nodejs.org/job/node-test-pull-request/63530/
@nodejs/tsc add it to the agenda for visibility.
I'd also add a test for the specific behavior that we now have when two separate REPL instances receive the same uncaught exception.
That won't happen. It's specifically set up so that an uncaught exception triggered from REPL (A) won't affect REPL (B). A test to ensure this is https://github.com/nodejs/node/pull/55204/files#diff-ca6f6e3faa6e2dc3fba74eabfc212f36fa2f84b7a17e3f73662d24b678d18f73 (although it doesn't check two REPLs, rather it verifies that an uncaught exception triggered from outside the REPL does not affect the REPL, which [testing-wise] is a similar test)