repl: handle errors from getters during completion

[islandryu]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.07%. Comparing base (b16a0e7) to head (11222b5).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/repl.js	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59044      +/-   ##
==========================================
+ Coverage   90.05%   90.07%   +0.01%     
==========================================
  Files         645      645              
  Lines      189153   189162       +9     
  Branches    37091    37099       +8     
==========================================
+ Hits       170348   170380      +32     
+ Misses      11528    11478      -50     
- Partials     7277     7304      +27     

Files with missing lines	Coverage Δ
lib/repl.js	94.24% <91.66%> (+0.02%)	⬆️

... and 45 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[islandryu]

I noticed this while trying to add a test for a getter in the following PR, it seems that if an error is thrown inside a getter, the server itself crashes.
#58943

@dario-piotrowicz
I'd appreciate it if you could take a look.

[dario-piotrowicz]

@islandryu thanks so much for spotting the issue 🙏

The change looks good to me 😃

But I think the test can be cleaned up a bit (it looks more complex than it needs to me in my opinion), in it I see:

• the unnecessary definition of a class
• unnecessary multiple evals
• missing spacing
• duplicated code

I would suggest to change the test to something like the following instead:

'use strict';

const common = require('../common');
const repl = require('repl');
const ArrayStream = require('../common/arraystream');
const assert = require('assert');

(async function() {
 await runTest();
})().then(common.mustCall());

async function runTest() {
 const input = new ArrayStream();
 const output = new ArrayStream();

 const replServer = repl.start({
   prompt: '',
   input,
   output: output,
   allowBlockingCompletions: true,
   terminal: true
 });

 replServer._domain.on('error', (e) => {
   assert.fail(`Error in REPL domain: ${e}`);
 });

 await new Promise((resolve, reject) => {
   replServer.eval(`
     const getNameText = () => "name";
     const foo = { get name() { throw new Error(); } };
     `, replServer.context, '', (err) => {
     if (err) {
       reject(err);
     } else {
       resolve();
     }
   });
 });

 ['foo.name.', 'foo["name"].', 'foo[getNameText()].'].forEach(test => {
   replServer.complete(
     test,
     common.mustCall((error, data) => {
       assert.strictEqual(error, null);
       assert.strictEqual(data.length, 2);
       assert.strictEqual(data[1], test);
     })
   );
 })
}

But besides the test the change looks great to me 😄

[islandryu]

Thanks for the feedback!
Indeed, my test was unnecessarily complex.
I've updated it following your suggestion!

[dario-piotrowicz]

Thanks a lot for updating the test @islandryu 🫶

Looks good to me, again thanks a lot for spotting this! 🚀

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/59044
✔  Done loading data for nodejs/node/pull/59044
----------------------------------- PR info ------------------------------------
Title      repl: handle errors from getters during completion (#59044)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     islandryu:fix/repl-complete-getter-error -> nodejs:main
Labels     repl, needs-ci, commit-queue-squash
Commits    2
 - repl: handle errors from getters during completion
 - fix test
Committers 1
 - islandryu <shimaryuhei@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/59044
Reviewed-By: Dario Piotrowicz <dario.piotrowicz@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/59044
Reviewed-By: Dario Piotrowicz <dario.piotrowicz@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 13 Jul 2025 07:07:49 GMT
   ✔  Approvals: 1
   ✔  - Dario Piotrowicz (@dario-piotrowicz): https://github.com/nodejs/node/pull/59044#pullrequestreview-3014134305
   ✘  This PR needs to wait 29 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-07-18T06:32:35Z: https://ci.nodejs.org/job/node-test-pull-request/68000/
- Querying data for job/node-test-pull-request/68000/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/16383640289

[dario-piotrowicz]

oh.... this needs one more day before it can be merged 😓

[dario-piotrowicz]

ah alright, thanks James 🙂

[nodejs-github-bot]

Landed in c8d5b39