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: integrate experimental repl #52965

Closed
wants to merge 1 commit into from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented May 12, 2024

Summary

This PR introduces an overhauled REPL for Node.js, building upon the groundwork laid by @devsnek and their team on nodejs/repl. While significant changes have been made to the team's prototype REPL, credit for the concept belongs to them for their initial codebase.

Modifications

Technical

  • Class: This REPL uses an ECMAScript class instead of the REPLServer function prototype.
  • Execution Context: Unlike the current REPL, which uses a mix of an inspector session and a virtual context, and the prototype REPL, which uses child_process and an inspector session, this REPL uses only a virtual context. (This allows for faster execution and less memory usage)

General

  • Syntax Highlighting: Real-time syntax highlighting is now a feature of this REPL using a custom implementation.
  • Runtime Variables: This REPL introduces an _X variable (where X is any line number), differing from the typical _ and _error variables.

Usage

To try out this experimental REPL, users must enable it with the --experimental-repl CLI flag; otherwise, the stable REPL remains unaffected.

Why?

While I love the current REPL, an experimental redesign has been in the works for a while, and in my opinion, it was completable, and while some decent changes and adjustments, it could find a home in Node.js.

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 12, 2024
@RedYetiDev RedYetiDev added repl Issues and PRs related to the REPL subsystem. experimental Issues and PRs related to experimental features. review wanted PRs that need reviews. labels May 12, 2024
@RedYetiDev
Copy link
Member Author

I took advice based on my previous PRs, those related to this and unrelated, and I think this has promise. I hope y'all will take a look :-)

@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 12, 2024

By the way, the highlighting patterns were inspired by that of https://github.com/speed-highlight/core/blob/249ad8fe9bf3eb951eb263b5b3c35887bbbbb2eb/src/languages/js.js

doc/api/cli.md Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented May 13, 2024

On top of TLA, better error message when using import statement is also lacking – but maybe support for import statements should be a goal. I kinda doubt that integrating the code in the node repo makes it more likely that those features will get implemented. If the code is "highly experimental", is there value in exposing it before making it more stable? – and is there anyone working on making it more stable?

There are some very weird cases (e.g. running delete Array.prototype[Symbol.iterator] would actually remove globalThis.Array 🤨 You have to run Function("a", "delete a.prototype[Symbol.iterator]")(Array) to delete the correct property.

The Node.js REPL has remained unchanged for years

That's hardly true.. even if it was, that wouldn't be good point, doing a change for the sake of doing changes is not convincing. You don't need to belittle other people's work, why not focus on the actual technical advantages?

  • (This allows for faster execution and less memory usage)

And also allows for e.g. deleting Function.prototype.call without crashing the process!

By the way, the highlighting patterns were inspired by that of https://github.com/speed-highlight/core/blob/249ad8fe9bf3eb951eb263b5b3c35887bbbbb2eb/src/languages/js.js

Are we sure the license allows for that?

@benjamingr
Copy link
Member

@nodejs/repl

@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 13, 2024

You don't need to belittle other people's work, why not focus on the actual technical advantages?

I didn't mean to belittle anyone's work, I'm sorry if it seems that way, the REPL currently is a marvel, and I do love it, I just figured that an experimental change couldn't hurt. I should not have worded my PR like that, as I see it sounds belittling.

As for technical advantages:

  • This version does not rely on the inspector protocol
  • This version includes syntax highlighting
  • The preview in this version will preview the current expression, rather than the entire execution
  • And more

Are we sure the license allows for that?

They RegEx patterns were inspired, and while they are similar, they have been modified for this use-case. I can contact the author to make sure if you'd like.

If the code is "highly experimental", is there value in exposing it before making it more stable? – and is there anyone working on making it more stable?

In my opinion, its not "highly" experimental, despite what I said, but given the edge cases, I didn't want to assume any form of slight stability.

There are some very weird cases

Yes, and for the Array one, I think I may know whats causing it and I'll take a look later today. (I think it has to do with the preview implementation)

And also allows for e.g. deleting Function.prototype.call without crashing the process!

Just to clarify, is this a bad thing?

@cola119
Copy link
Member

cola119 commented May 14, 2024

While I understand the technical advantages of the experimental REPL you mentioned, I'm not convinced that integrating it into the core is the only way to improve the current REPL. We can also incorporate improvements through gradual patched to the core. Additionally, I'm concerned about the maintenance costs of the new REPL. It was actively developed until 2019, and with few contributors, future contributions may become difficult, potentially leading to it becoming unmaintained.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 14, 2024

While I understand the technical advantages of the experimental REPL you mentioned, I'm not convinced that integrating it into the core is the only way to improve the current REPL. We can also incorporate improvements through gradual patched to the core.

I suppose, but IMO one of my favorite changes can't be implemented gradually, as it is a re-write of how the REPL works fundamentally. In the current REPL, an inspector protocol is used for processing some expressions, and a VM context for others. In this REPL, a VM context is only used (which is more efficient and easier on the debugger).

Additionally, I'm concerned about the maintenance costs of the new REPL. It was actively developed until 2019, and with few contributors, future contributions may become difficult, potentially leading to it becoming unmaintained.

Yes, I saw it was last edited several years ago. Rest assured that, while I used that REPL as inspiration, many changes have been made to it to bring it to a point where I'm happy with it, and I'm happy to keep maintaining it. This REPL isn't the same one as it was back in 2019, and while they share features, this one has its own 'spin' on things. I'm confident it won't be unmaintained, like I said, I'm happy to chip in with it, and I'm happy to educate the REPL team on how it works :-)

(I'm not a CODEOWNER, and I don't want to seem like I'm 'taking over' things, I want to seem like a helpful volunteer)

lib/internal/repl/experimental/index.js Outdated Show resolved Hide resolved
lib/internal/repl/experimental/index.js Outdated Show resolved Hide resolved
@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 15, 2024

Later today,

I'll mark the contributors of the original concept as co-authors, they deserve a huge round-of-applause

Co-Authored-By: snek <me@gus.host>
@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 19, 2024

I've added @devsnek as a Co-Author, as they were the only contributor to the repo with more than 5 commits.

@RedYetiDev RedYetiDev changed the title repl: enable experimental repl repl: integrate experimental repl May 22, 2024
@RedYetiDev
Copy link
Member Author

And also allows for e.g. deleting Function.prototype.call without crashing the process!

@aduh95 just clarifying, but is that a bad thing? I haven't testing that case yet, but I'm not sure what's significant about that?

@aduh95
Copy link
Contributor

aduh95 commented May 31, 2024

To clarify, I see “not crashing” as a “good thing”. The fact that the experimental REPL is more resilient than the current one is IMO an argument in its favor.

@RedYetiDev
Copy link
Member Author

To clarify, I see “not crashing” as a “good thing”. The fact that the experimental REPL is more resilient than the current one is IMO an argument in its favor.

Great! I just want the clarification, thanks for your help :-).

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I believe we should improve the current repl feature by feature. It is already possible to use the other repl as outlined in the documentation and including it in the binary is not improving that much but it increases the maintenance burden.
I am therefore against adding this without very good reason.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jun 5, 2024

I believe we should improve the current repl feature by feature. It is already possible to use the other repl as outlined in the documentation and including it in the binary is not improving that much but it increases the maintenance burden.

This isn't just the other REPL; it draws inspiration from it but introduces modifications. Additionally, integrating this REPL incrementally isn't feasible as it operates without the debugger (which is the evaluator of the original REPL, along with VM). Unlike its predecessor, this REPL fundamentally alters the approach to script evaluation to use VM, leveraging a more resilient method.

@RedYetiDev
Copy link
Member Author

As recommended, I'm gonna open pulls for these changes one at a time, making reviewing and landing much easier on everyone involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants