repl: deprecate repl.builtinModules

[dario-piotrowicz]

Fixes #57504

[juanarbol]

Amazing, thanks! If CI is happy, I'm happy as well.

[aduh95]

Can you please open a PR that first doc-deprecates the API? Hum actually, I'm not sure whether --pending-deprecation changes would be semver-major, so that might be fine EDIT2: just checked, we have precedents for landing as semver-patch pending deprecations

[nodejs-github-bot]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (0cfc471) to head (37cfc74).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #57508   +/-   ##
=======================================
  Coverage   90.21%   90.22%           
=======================================
  Files         629      629           
  Lines      184845   184853    +8     
  Branches    36206    36211    +5     
=======================================
+ Hits       166766   166786   +20     
+ Misses      11024    11023    -1     
+ Partials     7055     7044   -11     

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

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

[BridgeAR]

I would actually like to keep this around and just fix the issue. The reason is that we expose underscored modules in Module one and those should never be used.

The current issue with the repl exposed one is that it filters node: prefixed ones out. That should not be the case.

We can also update the documentation to say "all besides underscored'.

[aduh95]

I would actually like to keep this around

Any particular reason for that? Do you have a use-case in mind?

[BridgeAR]

@aduh95

I tried to provide the reason in my comment above: we include e.g., _http_common. These should not be used and showing them in the REPL does not make any sense from a usability standpoint (you mostly care about regular modules).
They are just exposed due to legacy reasons.

[dario-piotrowicz]

@BridgeAR Sorry for asking something probably pretty stupid 😓

But I don't have the full context here, could you clarify what the underscore modules are and what they are there for?

Also, when you said that they shouldn't be used do you mean in the REPL? or in general? why? and if it is the latter, why are they in module.builtinModules in the first place? (could they be removed from there?)

[anonrig]

I would actually like to keep this around and just fix the issue. The reason is that we expose underscored modules in Module one and those should never be used.

The current issue with the repl exposed one is that it filters node: prefixed ones out. That should not be the case.

We can also update the documentation to say "all besides underscored'.

I didn't understand your reasoning behind this. Would you mind explaining more? @BridgeAR

• require('module').builtinModules shouldn't return modules that start with _
• require('repl').builtinModules shouldn't exist at all since it is almost same as module builtinModules.

My understanding is, we can deprecate repl builtinModules, and also in another PR remove the modules in module.builtinModules that starts with _ (which will be a semver-major PR, I suppose).

Any concerns with this path forward @BridgeAR?

[BridgeAR]

@anonrig
If we are able to remove modules from require('module').builtinModules that start with _, then I am all for removing the repl one as well! I am just not certain if that's possible and I would like to start with that before we remove the repl one.

[dario-piotrowicz]

I'm giving removing the underscore modules from require('module').builtinModules a shot here: #57540 🙂