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 #52593

Closed
wants to merge 3 commits into from
Closed

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Apr 19, 2024

Summary

This PR introduces an overhauled REPL for Node.js, inspired by 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, rather than the REPLServer function prototype
  • Execution Context Contrary to 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.

General

  • Syntax Highlighting Using the a custom implementation, syntax highlighting (in real-time) is now a feature of this REPL
  • (Coming Soon) Annotations Function annotations are under development
  • Runtime Variables Differing from the typical _ and _error variables, this REPL has an _X variable (where X is any line number)

Upcoming Features

These features are under development but are not included in this integration (as they are incomplete)

  • Anotations An annotations system, similar to that of the prototype REPL is under development

Usage

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

Limitations

While this iteration shows promise, it comes with certain limitations due to its experimental nature:

  1. Annotations is a work in progress. Although there's a prototype implementation, it heavily relies on 'requires'. I aim for a more dynamic approach, avoiding the generated one.
  2. Top-level await is unsupported

Why?

The Node.js REPL has remained unchanged for years, and it's time for a makeover.

Caution

This REPL is highly experimental, and its use may lead to unintended side effects.

@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 Apr 19, 2024
@richardlau richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 19, 2024
@RedYetiDev RedYetiDev force-pushed the patch-11 branch 2 times, most recently from b3dcdfc to 66b08ec Compare April 19, 2024 15:32
@RedYetiDev
Copy link
Member Author

Note that a few 'little' bugs have been patched locally, but I don't want to force push for a little bit (so I can fix and test even more :-) )

@aduh95
Copy link
Contributor

aduh95 commented Apr 19, 2024

Primordials are not used in active runtime places, as they are speed bottlenecks.

REPL is a place where reliability is more important than speed, so definitely a part of the codebase where we'd want to avoid prototype pollution as much as possible. A solution that have been suggested was to run user code on a different realm, so the UI and user code can't interfere with one another. I think this is going to be a necessary condition before we can replace the current REPL and/or call it stable.

Additionally, the lack of primordials will help with the (possible) removal of them entirely.

(Off topic, but removal of primordials is not realistic nor desirable any time soon IMO. Anyways, let's not discuss that here)

@RedYetiDev
Copy link
Member Author

REPL is a place where reliability is more important than speed, so definitely a part of the codebase where we'd want to avoid prototype pollution as much as possible. A solution that have been suggested was to run user code on a different realm, so the UI and user code can't interfere with one another. I think this is going to be a necessary condition before we can replace the current REPL and/or call it stable.

Currently, I have this REPL running in it's own vm context.

(Off topic, but removal of primordials is not realistic nor desirable any time soon IMO. Anyways, let's not discuss that here)

Oh! I must've heard wrong information, I'll add primordials

@aduh95
Copy link
Contributor

aduh95 commented Apr 19, 2024

Oh! I must've heard wrong information, I'll add primordials

More accurately, you heard a different opinion :) It doesn't mean that my opinion is the correct one / the other opinion you heard is "wrong", deciding if primordials are worth it is subjective, so it should not be surprising there's no consensus on how/if they should be used. In the end, you are free to do as you please as long as the linter is happy (and the code is not on an error path / does not introduce perf regression).

@RedYetiDev RedYetiDev force-pushed the patch-11 branch 2 times, most recently from a0ee2b7 to c09f639 Compare April 19, 2024 18:40
@VoltrexKeyva VoltrexKeyva added the repl Issues and PRs related to the REPL subsystem. label Apr 19, 2024
@RedYetiDev
Copy link
Member Author

Note

In my latest commit, I changed the test-cli-options-doc.js file. There was a bug in the file, that prevented the tests from passing (and a few errors in my documentation, which I also resolved).
The check was --no<option>, when it should have been `--no<option>`

@RedYetiDev
Copy link
Member Author

ERROR: Coverage for lines (94.96%) does not meet global threshold (95%)

Well... I have my work cut out for me

Copy link

@Noumanluckey8057 Noumanluckey8057 left a comment

Choose a reason for hiding this comment

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

Ok

@RedYetiDev RedYetiDev force-pushed the patch-11 branch 6 times, most recently from 0a6d558 to f5cf21a Compare April 20, 2024 22:18
@RedYetiDev
Copy link
Member Author

All tests passed!

CCing @nodejs/repl for review

@RedYetiDev RedYetiDev added the experimental Issues and PRs related to experimental features. label Apr 21, 2024
@RedYetiDev
Copy link
Member Author

Requesting a CI test so verify all is in working order

@RedYetiDev RedYetiDev added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 21, 2024
doc/api/repl.md Outdated Show resolved Hide resolved
doc/api/repl.md Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev requested a review from aduh95 April 28, 2024 14:16
@RedYetiDev
Copy link
Member Author

@aduh95 WDYT can be done about the dependencies?

@aduh95
Copy link
Contributor

aduh95 commented May 3, 2024

@aduh95 WDYT can be done about the dependencies?

Like I said, it needs to be moved to deps/, and the update process would need to be automated (which should happen on a separate PR that would need to land first). I reckon it does require more work, if you are not interested in doing that work, you could remove syntax highlighting from this PR, it can always be added later.

@RedYetiDev
Copy link
Member Author

Sure thing! I'll move the dependencies today :-)

@RedYetiDev RedYetiDev added the dependencies Pull requests that update a dependency file. label May 3, 2024
@RedYetiDev
Copy link
Member Author

Once local linting and building is complete, I'll update the PR. Adding dependencies label preemptively.

@RedYetiDev RedYetiDev added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels May 3, 2024
@RedYetiDev
Copy link
Member Author

Added labels based on what the would've been if this PR opened with these changes.

@aduh95
Copy link
Contributor

aduh95 commented May 3, 2024

Sorry if you missed it, but like I said in my other comment, the censoring should happen in a separate PR so we can discuss whether we’re OK with taking a new dependency.

@RedYetiDev
Copy link
Member Author

Sorry if you missed it, but like I said in my other comment, the censoring should happen in a separate PR so we can discuss whether we’re OK with taking a new dependency.

Got it.

@RedYetiDev RedYetiDev added blocked PRs that are blocked by other issues or PRs. and removed build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. dependencies Pull requests that update a dependency file. labels May 3, 2024
@RedYetiDev
Copy link
Member Author

I'll open a PR shortly, which will block this, so I added the 'blocked' label.

@RedYetiDev RedYetiDev removed the blocked PRs that are blocked by other issues or PRs. label May 9, 2024
@RedYetiDev
Copy link
Member Author

@aduh95 I've implemented a basic syntax highlighter (I haven't decided on colors, right now it's all inspect.colors.special (unless inspect.styles.X exists)

@RedYetiDev
Copy link
Member Author

The ReGexes used are inspired from a variety of sources, so I need to condense them a bit

@RedYetiDev
Copy link
Member Author

[re-opened due to merge conflicts]

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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants