Skip to content

repl: convert to ES2015 class - #54838

Closed
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:repl-refactor
Closed

repl: convert to ES2015 class#54838
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:repl-refactor

Conversation

@avivkeller

Copy link
Copy Markdown
Member

This PR modifies the REPL to use an ES5 class.

It will (hopefully) be one of many in series to refactor the REPL's code, as IMO it is currently hard to read as is. My plan is to first convert the REPL to ECMAScript classes, and then move away from using the the domain module. IMO keeping the internal codebase up-to-date with deprecations and additions is a great way to maintain longevity.

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Sep 7, 2024
@avivkeller

Copy link
Copy Markdown
MemberAuthor

Right now, despite this change, most of the logic for the REPL remains in the constructor. I plan to move those into class methods (but not all at once - as that broke things).

@avivkeller

Copy link
Copy Markdown
MemberAuthor

CC @nodejs/repl

@codecov

codecovBot commented Sep 7, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 94.02439% with 49 lines in your changes missing coverage. Please review.

Project coverage is 87.62%. Comparing base (5a3c605) to head (67a09cf).
Report is 41 commits behind head on main.

Files with missing linesPatch %Lines
lib/repl.js94.02%47 Missing and 2 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #54838 +/- ##
========================================
Coverage 87.61% 87.62% ========================================
Files 650 651 +1 Lines 182945 183326 +381 Branches 35394 35445 +51 ========================================
+ Hits 160282 160632 +350 - Misses 15918 15953 +35 + Partials 6745 6741 -4 
Files with missing linesCoverage Δ
lib/repl.js94.06% <94.02%> (-0.06%)⬇️

... and 33 files with indirect coverage changes

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not against this change but I think we need to take it through a deprecation cycle first given the breaking change.

Comment threadlib/repl.js Outdated
@avivkeller
avivkeller marked this pull request as draft September 7, 2024 22:25
@aduh95

Copy link
Copy Markdown
Contributor

Typo in the commit title s/ES5/ES2015/: "ECMAScript [5] does not use classes such as those in C++, Smalltalk, or Java." (source: https://262.ecma-international.org/5.1/#sec-4.2.1). The class keyword is listed as "Future Reserved Words" in ES5, see https://262.ecma-international.org/5.1/#sec-7.6.1.2

@avivkelleravivkeller changed the title repl: convert to ES5 classrepl: convert to ES2015 classSep 8, 2024
@avivkelleravivkeller added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 8, 2024
@avivkeller

avivkeller commented Sep 8, 2024

Copy link
Copy Markdown
MemberAuthor

semver-major, as this is a breaking change. See #54842 for the deprecation, which should land in a release line before this.

@marco-ippolito

marco-ippolito commented Sep 9, 2024

Copy link
Copy Markdown
Member

semver-major, as this is a breaking change. See #54842 for the deprecation, which should land in a release line before this.

doc deprecation needs to be followed by a runtime deprecation (emit a warning) which is semver major. The runtime deprecation needs to land before refactoring and removing the constructor without new which is a EOL deprecation.

@atlowChemi

atlowChemi commented Sep 14, 2024

Copy link
Copy Markdown
Member

@redyetidev We might want to consider landing this (possibly temporarily) with a solution similar to what some of the test-runner reporters implemented to solve this semver-major issue:

spec: {
__proto__: null,
configurable: true,
enumerable: true,
value: functionvalue(){
spec??=require('internal/test_runner/reporter/spec');
returnReflectConstruct(spec,arguments);
},
},

lcov: {
__proto__: null,
configurable: true,
enumerable: true,
value: functionvalue(){
lcov??=require('internal/test_runner/reporter/lcov');
returnReflectConstruct(lcov,arguments);
},
},

This would potentially allow landing this PR without needing to wait for the entire deprecation cycle to complete, and handle the deprecation separately

@avivkelleravivkeller added the blocked PRs that are blocked by other issues or PRs. label Sep 14, 2024
@avivkeller

Copy link
Copy Markdown
MemberAuthor

The issue with that is:

constreporters=require("node:test/reporters");(newreporters.spec())instanceofreporters.spec// false

Sidenote: This is blocked by #54869

@avivkeller

Copy link
Copy Markdown
MemberAuthor

Closing until the runtime deprecation is done (which won't be for a while)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blockedPRs that are blocked by other issues or PRs.needs-ciPRs that need a full CI run.replIssues and PRs related to the REPL subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@avivkeller@aduh95@marco-ippolito@atlowChemi@jasnell@addaleax@nodejs-github-bot