repl: runtime deprecate instantiating without new

[RedYetiDev]

Followup #54842

This PR runtime-deprecates instantiating the REPL classes without the new keyword.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.32%. Comparing base (f0e5b6a) to head (9bf85bc).
Report is 89 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54869      +/-   ##
==========================================
- Coverage   88.43%   88.32%   -0.11%     
==========================================
  Files         654      654              
  Lines      187662   187673      +11     
  Branches    36120    36044      -76     
==========================================
- Hits       165955   165763     -192     
- Misses      14948    15133     +185     
- Partials     6759     6777      +18     

Files with missing lines	Coverage Δ
lib/internal/util.js	97.11% <100.00%> (+0.13%)	⬆️
lib/repl.js	94.95% <100.00%> (-0.02%)	⬇️

... and 29 files with indirect coverage changes

[LiviaMedeiros]

LGTM with updates to existing codebase. Example:

node/test/parallel/test-repl-preview-without-inspector.js

Line 68 in 741004a

const repl = REPLServer({

[RedYetiDev]

CC @nodejs/repl

[jasnell]

I think we need a documentation-only deprecation on this first and we need to get a sense of how impactful a runtime deprecation would be on the ecosystem here. Given the nature of the repl module, this would most likely lead to a fair bit of console output noise for modules that are using the repl and users may not actually be able to do anything about it. So let's be careful.

[RedYetiDev]

I think we need a documentation-only deprecation on this first and we need to get a sense of how impactful a runtime deprecation would be on the ecosystem here.

FWIW It has already been doc-deprecated (but very recently).

[RedYetiDev]

@jasnell would you be opposed to landing this in v23 entirely-I'm happy to wait until v24?

[jasnell]

I prefer to wait

[RedYetiDev]

Got it. While I'd prefer for this to release in v23, I respect your opinion. I'll look into other optimizations in the meantime.

[RedYetiDev]

@nodejs/tsc per nodejs/Release#1034

---

My main question is: is this something that could happen in v23? Or should it wait (like @jasnell suggested)?

[benjamingr]

I agree with James on this

[anonrig]

Let's give people some time to make arrangements, before polluting their console.

[mcollina]

lgtm

[RedYetiDev]

I've added the blocked label, as this should go out with v24.

@jasnell, Putting aside the 'dont release yet', could you re-review?

[RedYetiDev]

Hey, I assume this needs a new CI since there has been commits since the latest change, can someone start it?

The CITGM shouldn't need a rerun tho (IIUC)

[nodejs-github-bot]

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

[RedYetiDev]

Can someone restart the fialed build so this can land? Thanks!

[nodejs-github-bot]

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

[RedYetiDev]

The CI is 📗, can this land 🚀?

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/54869
✔  Done loading data for nodejs/node/pull/54869
----------------------------------- PR info ------------------------------------
Title      repl: runtime deprecate instantiating without new (#54869)
Author     Aviv Keller <redyetidev@gmail.com> (@RedYetiDev)
Branch     RedYetiDev:repl-deprecate-2 -> nodejs:main
Labels     util, repl, semver-major, author ready, deprecations, needs-ci, needs-citgm, commit-queue-squash
Commits    6
 - repl: runtime deprecate instantiating without new
 - fixup!
 - fixup! fixup!
 - fixup! fixup! fixup!
 - fix flaky test
 - fixup! fix flaky test
Committers 2
 - RedYetiDev <38299977+RedYetiDev@users.noreply.github.com>
 - GitHub <noreply@github.com>
PR-URL: https://github.com/nodejs/node/pull/54869
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54869
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - repl: runtime deprecate instantiating without new
   ⚠  - fixup!
   ⚠  - fixup! fixup!
   ⚠  - fixup! fixup! fixup!
   ⚠  - fix flaky test
   ⚠  - fixup! fix flaky test
   ℹ  This PR was created on Mon, 09 Sep 2024 23:11:31 GMT
   ✔  Approvals: 4
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54869#pullrequestreview-2383013809
   ✔  - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/54869#pullrequestreview-2302928904
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54869#pullrequestreview-2374242695
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/54869#pullrequestreview-2397344156
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-11-03T06:43:31Z: https://ci.nodejs.org/job/node-test-pull-request/63402/
   ℹ  Last CITGM CI on 2024-10-24T21:16:40Z: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3494/
- Querying data for job/node-test-pull-request/63402/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

[nodejs-github-bot]

Landed in 0368f2f