node icon indicating copy to clipboard operation
node copied to clipboard

repl: don't use deprecated `domain` module

Open avivkeller opened this issue 1 year ago • 11 comments

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 the domain module 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

avivkeller avatar Oct 01 '24 00:10 avivkeller

One test still needs updating.

avivkeller avatar Oct 01 '24 00:10 avivkeller

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:

... and 139 files with indirect coverage changes

: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.

codecov[bot] avatar Oct 01 '24 01:10 codecov[bot]

I have a rough feeling that you'd need to add a FinalizationRegistry to clear up any instances

Why? Is unsetting the callback on close() not enough?

avivkeller avatar Oct 07 '24 21:10 avivkeller

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

nodejs-github-bot avatar Oct 26 '24 10:10 nodejs-github-bot

Can someone start a CITGM if they thinks it's necessary. IMO we can use the most recent main branch CITGM for comparison

avivkeller avatar Oct 29 '24 20:10 avivkeller

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?

aduh95 avatar Nov 03 '24 11:11 aduh95

Got it! I've removed the label. IIRC semver-major PRs need two TSC approvals to land, would you mind reviewing?

avivkeller avatar Nov 04 '24 20:11 avivkeller

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

nodejs-github-bot avatar Nov 10 '24 20:11 nodejs-github-bot

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

nodejs-github-bot avatar Nov 12 '24 17:11 nodejs-github-bot

@nodejs/tsc add it to the agenda for visibility.

mcollina avatar Nov 18 '24 18:11 mcollina

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)

avivkeller avatar Nov 20 '24 13:11 avivkeller