GeezerVison

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.

@@            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     

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

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.

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.

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.

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?

Ping @nodejs/tsc per the plan for v23

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

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.

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

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?

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?

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