repl: move completion logic to internal module #59889

dario-piotrowicz · 2025-09-14T23:27:29Z

This PR moves the repl completion logic into its own internal module, my reasoning for proposing this change is that the repl completion logic currently occupies a big chunk of the repl module and it's hard to tell which functions are relevant to it and which are not.

So, also given the fact that the completion logic is pretty self contained, I believe that isolating it will lead to a better, more maintainable and more understandable code structure.

This is also consistent with the repl history which has its own internal module (and also the await module).

Later I am also planning to do other code restructuring/cleanups, but for this PR, in order to make it easy to review I tried to minimize as much as possible code changes and just tried to move things over the new internal completion module. So in this PR I basically only moved code from lib/repl.js to lib/internal/repl/completion.js, with the only exception being that I moved shared logic into the lib/internal/repl/utils.js (and imported it in the other 2 modules).

codecov · 2025-09-15T00:53:22Z

Codecov Report

❌ Patch coverage is 94.87472% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.53%. Comparing base (ed94bc4) to head (8663bb6).
⚠️ Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/repl/completion.js	94.75%	38 Missing and 4 partials ⚠️
lib/internal/repl/utils.js	95.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #59889   +/-   ##
=======================================
  Coverage   88.52%   88.53%           
=======================================
  Files         703      704    +1     
  Lines      207825   207917   +92     
  Branches    40011    40014    +3     
=======================================
+ Hits       183982   184072   +90     
+ Misses      15864    15860    -4     
- Partials     7979     7985    +6

Files with missing lines	Coverage Δ
lib/repl.js	`94.12% <100.00%> (-0.15%)`	⬇️
lib/internal/repl/utils.js	`96.37% <95.00%> (-0.11%)`	⬇️
lib/internal/repl/completion.js	`94.75% <94.75%> (ø)`

... and 31 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.

nodejs-github-bot · 2025-09-16T08:48:44Z

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

joyeecheung

LGTM with a comment

joyeecheung · 2025-09-19T15:13:39Z

lib/internal/repl/utils.js

+  new SafeSet(vm.runInNewContext('Object.getOwnPropertyNames(globalThis)'));
+
+const replBuiltinLibsWrapper = {
+  _builtinLibs: ArrayPrototypeFilter(


It seems fine for this to just be it's own property that gets re-exported instead of being nested here and then accessed again in the public module (I am a bit confused what this is wrapping to get named a "wrapper")

I am using a wrapper here because the _builtinLibs value can be updated by users in the setters here:

node/lib/repl.js

Lines 1399 to 1429 in 446cedc

ObjectDefineProperty(module.exports, 'builtinModules', {

__proto__: null,

get: pendingDeprecation ? deprecate(

() => replBuiltinLibsWrapper._builtinLibs,

'repl.builtinModules is deprecated. Check module.builtinModules instead',

'DEP0191',

) : () => replBuiltinLibsWrapper._builtinLibs,

set: pendingDeprecation ? deprecate(

(val) => replBuiltinLibsWrapper._builtinLibs = val,

'repl.builtinModules is deprecated. Check module.builtinModules instead',

'DEP0191',

) : (val) => replBuiltinLibsWrapper._builtinLibs = val,

enumerable: false,

configurable: true,

});

ObjectDefineProperty(module.exports, '_builtinLibs', {

__proto__: null,

get: pendingDeprecation ? deprecate(

() => replBuiltinLibsWrapper._builtinLibs,

'repl._builtinLibs is deprecated. Check module.builtinModules instead',

'DEP0142',

) : () => replBuiltinLibsWrapper._builtinLibs,

set: pendingDeprecation ? deprecate(

(val) => replBuiltinLibsWrapper._builtinLibs = val,

'repl._builtinLibs is deprecated. Check module.builtinModules instead',

'DEP0142',

) : (val) => replBuiltinLibsWrapper._builtinLibs = val,

enumerable: false,

configurable: true,

});

And I wanted to make sure that all modules do get the potentially updated value in such case, that's why I used a wrapper to make sure that all get the same object and the _builtinLibs fields (updated or not) is shared among them.

Is this the wrong approach? do you see a better/simpler way to achieve this?

PS: note that the setters are deprecated to this should also be temporary, once those are removed in the future the wrapper can be removed anyways

friendly ping @joyeecheung 😊

Is this actually used here? If not maybe it can just be left in the public module?

Or you can just export methods for get/set it in this module, and let the public module's accessors forward to them? I think that more typically done for values kept in a module's singleton

Is this actually used here? If not maybe it can just be left in the public module?

it's used both in the public one and in a few places in the completion module (you can search for replBuiltinLibsWrapper._builtinLibs in the lib/internal/repl/completion.js file)

that's why I moved it here

Or you can just export methods for get/set it in this module, and let the public module's accessors forward to them? I think that more typically done for values kept in a module's singleton

right, good call, that does sound a bit cleaner actually 😃

Does this look ok to you? 🙂
1baac2f

I see, it might be useful to add a comment there about why this has to be accessed like this to maintain liveness in case someone tries to use it from a cached access.

done: 167ecc0 🙂

nodejs-github-bot · 2025-09-27T16:08:43Z

Commit Queue failed

- Loading data for nodejs/node/pull/59889
✔  Done loading data for nodejs/node/pull/59889
----------------------------------- PR info ------------------------------------
Title      repl: move completion logic to internal module (#59889)
Author     Dario Piotrowicz <dario.piotrowicz@gmail.com> (@dario-piotrowicz)
Branch     dario-piotrowicz:dario/repl-completion -> nodejs:main
Labels     repl, author ready, needs-ci, commit-queue-squash
Commits    3
 - repl: move completion logic to internal module
 - fixup! repl: move completion logic to internal module
 - fixup! repl: move completion logic to internal module
Committers 1
 - Dario Piotrowicz <dario.piotrowicz@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/59889
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/59889
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - repl: move completion logic to internal module
   ⚠  - fixup! repl: move completion logic to internal module
   ⚠  - fixup! repl: move completion logic to internal module
   ℹ  This PR was created on Sun, 14 Sep 2025 23:27:29 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/59889#pullrequestreview-3224294119
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/59889#pullrequestreview-3245397051
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-09-16T08:48:44Z: https://ci.nodejs.org/job/node-test-pull-request/69245/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - repl: move completion logic to internal module
   ⚠  - fixup! repl: move completion logic to internal module
   ⚠  - fixup! repl: move completion logic to internal module
- Querying data for job/node-test-pull-request/69245/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

dario-piotrowicz · 2025-09-27T16:13:52Z

@jasnell @joyeecheung would you mind re-approving the PR after the latest commits so that commit-queue can succeed? 🙂🙏

remove replBuiltinLibsWrapper

add comment to `getReplBuiltinLibs` and `setReplyBuiltinLibs` functions

nodejs-github-bot · 2025-10-07T15:06:06Z

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

nodejs-github-bot · 2025-10-07T21:47:00Z

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

nodejs-github-bot · 2025-10-07T23:05:53Z

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

nodejs-github-bot · 2025-10-08T06:31:44Z

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

nodejs-github-bot · 2025-10-08T07:37:24Z

Landed in 200fe9e

nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Sep 14, 2025

dario-piotrowicz force-pushed the dario/repl-completion branch 4 times, most recently from 0db4da2 to 446cedc Compare September 14, 2025 23:54

jasnell approved these changes Sep 15, 2025

View reviewed changes

dario-piotrowicz added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 15, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 16, 2025

joyeecheung approved these changes Sep 19, 2025

View reviewed changes

dario-piotrowicz added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 24, 2025

dario-piotrowicz force-pushed the dario/repl-completion branch from e25494e to 167ecc0 Compare September 25, 2025 14:43

dario-piotrowicz added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 27, 2025

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 27, 2025

dario-piotrowicz removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 5, 2025

dario-piotrowicz added 3 commits October 5, 2025 17:03

repl: move completion logic to internal module

71b8416

fixup! repl: move completion logic to internal module

9516b3f

remove replBuiltinLibsWrapper

fixup! repl: move completion logic to internal module

8663bb6

add comment to `getReplBuiltinLibs` and `setReplyBuiltinLibs` functions

dario-piotrowicz force-pushed the dario/repl-completion branch from 167ecc0 to 8663bb6 Compare October 5, 2025 16:04

anonrig approved these changes Oct 7, 2025

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2025

dario-piotrowicz added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2025

dario-piotrowicz added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2025

nodejs-github-bot merged commit 200fe9e into nodejs:main Oct 8, 2025
64 of 66 checks passed

	ObjectDefineProperty(module.exports, 'builtinModules', {
	__proto__: null,
	get: pendingDeprecation ? deprecate(
	() => replBuiltinLibsWrapper._builtinLibs,
	'repl.builtinModules is deprecated. Check module.builtinModules instead',
	'DEP0191',
	) : () => replBuiltinLibsWrapper._builtinLibs,
	set: pendingDeprecation ? deprecate(
	(val) => replBuiltinLibsWrapper._builtinLibs = val,
	'repl.builtinModules is deprecated. Check module.builtinModules instead',
	'DEP0191',
	) : (val) => replBuiltinLibsWrapper._builtinLibs = val,
	enumerable: false,
	configurable: true,
	});

	ObjectDefineProperty(module.exports, '_builtinLibs', {
	__proto__: null,
	get: pendingDeprecation ? deprecate(
	() => replBuiltinLibsWrapper._builtinLibs,
	'repl._builtinLibs is deprecated. Check module.builtinModules instead',
	'DEP0142',
	) : () => replBuiltinLibsWrapper._builtinLibs,
	set: pendingDeprecation ? deprecate(
	(val) => replBuiltinLibsWrapper._builtinLibs = val,
	'repl._builtinLibs is deprecated. Check module.builtinModules instead',
	'DEP0142',
	) : (val) => replBuiltinLibsWrapper._builtinLibs = val,
	enumerable: false,
	configurable: true,
	});

Uh oh!

repl: move completion logic to internal module #59889

repl: move completion logic to internal module #59889

Conversation

dario-piotrowicz commented Sep 14, 2025

Uh oh!

codecov bot commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Sep 16, 2025

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 27, 2025

Uh oh!

dario-piotrowicz commented Sep 27, 2025

Uh oh!

nodejs-github-bot commented Oct 7, 2025

Uh oh!

nodejs-github-bot commented Oct 7, 2025

Uh oh!

nodejs-github-bot commented Oct 7, 2025

Uh oh!

nodejs-github-bot commented Oct 8, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 8, 2025

Uh oh!

Uh oh!

codecov bot commented Sep 15, 2025 •

edited

Loading

dario-piotrowicz Sep 24, 2025 •

edited

Loading