repl: don't use deprecated domain module
#55204
Open
+40
−216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RedYetiDev
added
the
semver-major
nodejs-github-bot
added
errors
This comment was marked as resolved.
This comment was marked as resolved.
anonrig
reviewed
Oct 1, 2024
| @@ -151,34 +151,18 @@ global or scoped variable, the input `fs` will be evaluated on-demand as | |||
|
|
|||
| <!-- YAML | |||
| changes: | |||
| - version: REPLACEME | |||
| pr-url: https://github.com/nodejs/node/pull/55204 | |||
| description: The `process.setUncaughtExceptionCaptureCallback()` | |||
There was a problem hiding this comment.
This is an implementation detail. Does it change the behavior of the function? If not, we should remove it from here. If it changes the behavior, this description should mention the behavior change rather than implementation specifics.
| } | ||
|
|
||
| domainSet.add(this._domain); | ||
| process.setUncaughtExceptionCaptureCallback(null); |
There was a problem hiding this comment.
Why do we call it twice? Please add a comment
| @@ -34,13 +34,6 @@ const works = [['inner.one'], 'inner.o']; | |||
| const putIn = new ArrayStream(); | |||
| const testMe = repl.start('', putIn); | |||
|
|
|||
| // Some errors might be passed to the domain. | |||
| testMe._domain.on('error', function(reason) { | |||
There was a problem hiding this comment.
This will require CITGM since you're altering a publicly accessible variable.
| assert.ok(result.status, 'testFile swallowed its error'); | ||
| const err = result.stderr.toString(); | ||
| assert.ok(err.includes('fhqwhgads'), err); | ||
| assert.ok(result.stdout.toString().includes('fhqwhgads')); |
| @@ -11,8 +11,6 @@ const testMe = repl.start('', putIn, function(cmd, context, filename, | |||
| callback(null, cmd); | |||
| }); | |||
|
|
|||
| testMe._domain.on('error', common.mustNotCall()); | |||
There was a problem hiding this comment.
This needs an equivalent change.
| @@ -174,7 +174,7 @@ async function ordinaryTests() { | |||
| 'undefined', | |||
| ]], | |||
| // Regression test for https://github.com/nodejs/node/issues/43777. | |||
| ['await Promise.resolve(123), Promise.resolve(456)', 'Promise {', { line: 0 }], | |||
| ['await Promise.resolve(123), Promise.resolve(456)', 'Promise { 456 }', { line: 0 }], | |||
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55204 +/- ##
==========================================
- Coverage 88.39% 88.34% -0.06%
==========================================
Files 652 650 -2
Lines 186565 186008 -557
Branches 36046 35885 -161
==========================================
- Hits 164916 164325 -591
- Misses 14908 15016 +108
+ Partials 6741 6667 -74
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the REPL module by eliminating the deprecated
node:domainmodule. It replaces its functionality with a modern approach using atry/catchblock andprocess.setUncaughtExceptionCaptureCallback.Why?
The
domainmodule 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 deprecateddomainmodule.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-majorbecause it introduces functional changes to the REPL: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).inspectno longer captures just-resolved promise prototype symbols: (IIUC) The symbols (e.g. async ids) are not applied in the same event loop tick as the promise resolution. Since the new implementation is faster, it inspects the promise before the symbols are added.uncaughtExceptionlisteners, while ingored, are no longer restricted from use. Along with this,ERR_INVALID_REPL_INPUTis no longer required, and has been removed.This PR is also marked
needs-citgmin case any of the above cause breakages.CC @nodejs/repl
CC @nodejs/tsc due to the semver-major