Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: runtime deprecate instantiating without new #54869

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RedYetiDev
Copy link
Member

Followup #54842

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

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module. labels Sep 9, 2024
@RedYetiDev RedYetiDev added semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Sep 9, 2024
Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.05%. Comparing base (c3a7b29) to head (1ada6a0).
Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
lib/repl.js 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54869      +/-   ##
==========================================
+ Coverage   87.91%   88.05%   +0.14%     
==========================================
  Files         651      651              
  Lines      183354   183413      +59     
  Branches    35719    35829     +110     
==========================================
+ Hits       161190   161506     +316     
+ Misses      15446    15158     -288     
- Partials     6718     6749      +31     
Files with missing lines Coverage Δ
lib/internal/util.js 96.59% <100.00%> (-0.16%) ⬇️
lib/repl.js 94.80% <75.00%> (-0.06%) ⬇️

... and 74 files with indirect coverage changes

Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with updates to existing codebase. Example:

@aduh95 aduh95 added the needs-citgm PRs that need a CITGM CI run. label Sep 10, 2024
@RedYetiDev
Copy link
Member Author

CC @nodejs/repl

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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
Copy link
Member Author

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

@jasnell
Copy link
Member

jasnell commented Sep 17, 2024

I prefer to wait

@RedYetiDev
Copy link
Member Author

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
Copy link
Member Author

RedYetiDev commented Sep 19, 2024

@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
Copy link
Member

I agree with James on this

@anonrig
Copy link
Member

anonrig commented Sep 19, 2024

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. repl Issues and PRs related to the REPL subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants