8000 repl: convert to ES2015 class by avivkeller · Pull Request #54838 · nodejs/node · GitHub
  • [go: up one dir, main page]
    Skip to content

    repl: convert to ES2015 class #54838

    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.

    Sign up for GitHub

    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

    Jump to bottom
    Closed
    avivkeller wants to merge 1 commit into from
    Closed

    repl: convert to ES2015 class #54838

    avivkeller wants to merge 1 commit into from

    Conversation

    @avivkeller
    Copy link
    Member
    @avivkeller avivkeller commented Sep 7, 2024

    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-bot nodejs-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
    Member Author
    avivkeller commented Sep 7, 2024

    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
    Member Author
    avivkeller commented Sep 7, 2024

    CC @nodejs/repl

    @codecov
    Copy link
    codecov bot commented Sep 7, 2024 •
    edited
    Loading

    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 lines Patch % Lines
    lib/repl.js 94.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 lines Coverage Δ
    lib/repl.js 94.06% <94.02%> (-0.06%) ⬇️

    ... and 33 files with indirect coverage changes

    jasnell
    jasnell requested changes Sep 7, 2024
    View reviewed changes
    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'm not against this change but I think we need to take it through a deprecation cycle first given the breaking change.

    @avivkeller avivkeller marked this pull request as draft September 7, 2024 22:25
    @aduh95
    Copy link
    Contributor
    aduh95 commented Sep 8, 2024

    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

    @avivkeller avivkeller changed the title repl: convert to ES5 class repl: convert to ES2015 class Sep 8, 2024
    @avivkeller avivkeller added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 8, 2024
    @avivkeller
    Copy link
    Member Author
    avivkeller commented Sep 8, 2024 •
    edited
    Loading

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

    @marco-ippolito
    Copy link
    Member
    marco-ippolito commented Sep 9, 2024 •
    edited
    Loading

    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
    Copy link
    Member
    atlowChemi commented Sep 14, 2024 •
    edited
    Loading

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

    node/lib/test/reporters.js

    Lines 34 to 42 in a65105e

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

    node/lib/test/reporters.js

    Lines 52 to 60 in a65105e

    lcov: {
    __proto__: null,
    configurable: true,
    enumerable: true,
    value: function value() {
    lcov ??= require('internal/test_runner/reporter/lcov');
    return ReflectConstruct(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

    @avivkeller avivkeller added the blocked PRs that are blocked by other issues or PRs. label Sep 14, 2024
    @avivkeller
    Copy link
    Member Author
    avivkeller commented Sep 14, 2024

    The issue with that is:

    const reporters = require("node:test/reporters");
(new reporters.spec()) instanceof reporters.spec // false

    Sidenote: This is blocked by #54869

    @avivkeller avivkeller closed this Sep 23, 2024
    @avivkeller
    Copy link
    Member Author
    avivkeller commented Sep 23, 2024

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

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

    Reviewers

    @jasnell jasnell jasnell requested changes

    @addaleax addaleax addaleax approved these changes

    Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

    Assignees

    No one assigned

    Labels

    blocked PRs that are blocked by other issues or PRs. needs-ci PRs that need a full 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.

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    7 participants

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