node
node copied to clipboard
nodejs
repl: integrate experimental repl
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_processand 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_errorvariables, this REPL has an_Xvariable (whereXis 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:
- 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.
- 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.
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 :-) )
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)
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
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).
[!NOTE] In my latest commit, I changed the
test-cli-options-doc.jsfile. 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>`
ERROR: Coverage for lines (94.96%) does not meet global threshold (95%)
Well... I have my work cut out for me
@aduh95 So far, I think the failed CI tests are unrelated to this change, WDYT?
The latest changes to the Node.js repo were merged into this PR. This will not affect this PR but will prevent a merge conflict.
I will update the code with the latest lint changes.
Any more things I need to update, @aduh95? I'm still not sure what to do about dependencies if they aren't going there.
By the way, currently the Markdown linter doesn't like the use of await in a JS code block, is there a way to disable it without adding a comment into the actual code block?
By the way, currently the Markdown linter doesn't like the use of
awaitin a JS code block, is there a way to disable it without adding a comment into the actual code block?
If you search for use of await in the doc/api folder, you can find example of how to use that keyword in a way that keeps the linter happy. Disabling the linter is (almost?) never the correct solution.
@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.
Once local linting and building is complete, I'll update the PR. Adding dependencies label preemptively.
Added labels based on what the would've been if this PR opened with these changes.
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.
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.
I'll open a PR shortly, which will block this, so I added the 'blocked' label.
@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)
The ReGexes used are inspired from a variety of sources, so I need to condense them a bit