repl: runtime deprecate inputStream, outputStream and _builtinLibs

[avivkeller]

These features have been doc-only deprecated for years (since v14!), so maybe they should finally be runtime deprecated?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.62%. Comparing base (5949e16) to head (f5f2f29).
Report is 273 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #54750 +/- ##
==========================================
+ Coverage 87.60% 87.62% +0.01% 
==========================================
Files 650 650 Lines 182829 182934 +105 Branches 35379 35383 +4 ==========================================
+ Hits 160173 160292 +119 + Misses 15928 15910 -18 - Partials 6728 6732 +4 

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

... and 42 files with indirect coverage changes

[aduh95]

Can you make the commit message clearer? e.g. repl: runtime deprecate `inputStream`, `outputStream` and `_builtinLibs` . Also, what is the rational for deprecating those?

[avivkeller]

Also, what is the rational for deprecating those?

These properties have been pending deprecation for years, so I believe it's finally time to runtime deprecate them. IMO things shouldn't be pending deprecation for as long as these were.

[aduh95]

These properties have been pending deprecation for years

Why are there deprecated in the first place?

IMO things shouldn't be pending deprecation for as long as these were.

Deprecations can stay at the same stage forever – I don't know about this one, but it is not a valid argument for runtime deprecate something.

node/doc/contributing/collaborator-guide.md

Lines 492 to 495 in 27f1306

	No API can change to End-of-Life without going through a Runtime Deprecation
	cycle. There is no rule that deprecated code must progress to End-of-Life.
	Documentation-Only and Runtime Deprecations can remain in place for an unlimited
	duration.

[avivkeller]

Deprecations can stay at the same stage forever – I don't know about this one, but it is not a valid argument for runtime deprecate something.

Apologies. A better argument for this is that these features have been replaced. inputStream has been replaced with input, and the same for output. _builtinLibs isn't best-practice (IIRC). I believe that runtime deprecating these features will encourage users to use the newer, better, replacements.

[aduh95]

A better argument for this is that these features have been replaced.

It is a much better argument, but I'm not sure it's true. Looking at #33294, those already existed when the deprecation was decided.

I believe that runtime deprecating these features will encourage users to use the newer, better, replacements.

I don't know if the replacements are better, IIUC it's perfectly equivalent. It will for sure create some friction for our users forcing them to either update the code to use the non-deprecated alias (which may or may not be code that they maintain), or disable/ignore the warnings. As Anna said in #33294 (review), shouldn't we prefer to keep the alias around forever?

[avivkeller]

Ping @nodejs/tsc per the plan for v23

[mcollina]

lgtm

[aduh95]

I'm still not convinced we should runtime deprecate this as I said in #54750 (comment).

[avivkeller]

I'm still not convinced we should runtime deprecate this as I said in #54750 (comment).

It will for sure create some friction for our users forcing them to either update the code to use the non-deprecated alias

Isn't that the point of deprecations? To encourage users to upgrade to the better maintained code? Runtime deprecating this will encourage users to move to code that is currently supported.

Maybe a few versions ago it would be a concern for friction for users, but these have been runtime deprecated for (IMO) long enough that if the users haven't upgraded to non-deprecated features, it's okay to present them with a warning.

[nodejs-github-bot]

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

[aduh95]

if the users haven't upgraded to non-deprecated features, it's okay to present them with a warning.

Why though? It seems to be adding friction for.. the sake of it?

[avivkeller]

Why though? It seems to be adding friction for.. the sake of it?

I wouldn’t say it’s just for the "sake" of it. It’s about tying up loose ends. These deprecations are meant to nudge users toward upgrading, and I think it's time to start issuing warnings for this feature. I don’t see it causing much friction—users should have been aware of this since it was documented as deprecated in v14.

If they choose not to upgrade, they can stick with V22, but that’s their loss, in my opinion. It’s crucial to keep the codebase current, and by gradually phasing out deprecated features, we encourage users to do the same.

---

I've added author ready, as there are enough TSC approvals for this to land in the next semver (and a CI run). @aduh95 are you blocking?

[aduh95]

Each deprecation should be its own PR. Also this shouldn't land without a CITGM run (and probably shouldn't land at all until there's a rational for it).

[avivkeller]

Each deprecation should be its own PR.

Got it. I currently have a lot of open PRs, and I'm trying to narrow my focus down to where it's really needed, so I'm closing this until a time where I can find a good rationale to not waste reviewers time