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

refactor(repl): use an inspector session #7763

Merged

Conversation

caspervonb
Copy link
Contributor

@caspervonb caspervonb commented Sep 30, 2020

This ports the REPL over to Rust and makes use of an inspector session to run a REPL on top of any isolate which lets make full use of rustylines various things like validators and completors without having to introduce a bunch of hard to test internal ops and glue code.

An accidental but good side effect of this is that the multiple line input we previously had is now an editable multi-line input prompt that is correctly stored in the history as a single entry.

We'll also be able to enable top level await via replMode in Runtime.evaluate (inspector protocol).

@caspervonb caspervonb force-pushed the refactor-repl-use-an-inspector-session branch from 11b8f10 to 043ee12 Compare September 30, 2020 05:59
cli/repl.rs Outdated Show resolved Hide resolved
cli/repl.rs Outdated Show resolved Hide resolved
cli/repl.rs Outdated Show resolved Hide resolved
cli/repl.rs Outdated Show resolved Hide resolved
@@ -1157,6 +1157,7 @@ fn repl_test_function() {
}

#[test]
#[ignore]
Copy link
Contributor Author

@caspervonb caspervonb Oct 1, 2020

Choose a reason for hiding this comment

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

Temporarily ignored, editing (e.g multiline) only works with a tty.

@caspervonb caspervonb marked this pull request as ready for review October 1, 2020 03:09
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Awesome work @caspervonb just a few minor nitpicks, but other than that I think it's landable.

pub struct Repl {
editor: Editor<()>,
history_file: PathBuf,
#[derive(Completer, Helper, Highlighter, Hinter)]
Copy link
Member

Choose a reason for hiding this comment

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

Does using derive here actually provide completions/hints/highlights? Or is it just to have default impls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to have default no-op implementations because the Helper trait requires them all to be implemented.

cli/repl.rs Show resolved Hide resolved
cli/repl.rs Outdated Show resolved Hide resolved
Comment on lines +143 to +154
let is_closing = session
.post_message(
"Runtime.evaluate".to_string(),
Some(json!({
"expression": "(globalThis.closed)",
"contextId": context_id,
})),
)
.await?
.get("result")
.unwrap()
.get("value")
Copy link
Member

Choose a reason for hiding this comment

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

👍 nice

Comment on lines -333 to -335
if (repl) {
replLoop();
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice to see this go

"contextId": context_id,
// TODO(caspervonb) set repl mode to true to enable const redeclarations and top
// level await
"replMode": false,
Copy link
Member

Choose a reason for hiding this comment

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

It'd be great if we could switch this on before Friday's release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll flip it and add tests as soon as this lands.

@ry
Copy link
Member

ry commented Oct 1, 2020

This used to work, but appears to be broken in this branch:

~/src/deno> cargo run
   Compiling deno v1.4.2 (/Users/rld/src/deno/cli)
    Finished dev [unoptimized + debuginfo] target(s) in 1m 11s
     Running `target/debug/deno`
Deno 1.4.2
exit using ctrl+d or close()
> function add(a, b) {
return a + b;
}
[Function: add]
> add(1, 2)
Uncaught ReferenceError: add is not defined
    at <anonymous>:2:1
>

@caspervonb
Copy link
Contributor Author

This used to work, but appears to be broken in this branch:

Oops, yeah bit too eager on the wrapping, need to wind it back to just starts_with("{" like #7591 did.

@caspervonb
Copy link
Contributor Author

Toned it down a bit to match #7591 exactly.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, massive work @caspervonb

@bartlomieju bartlomieju merged commit 4c779b5 into denoland:master Oct 1, 2020
ry added a commit to ry/deno that referenced this pull request Oct 2, 2020
Soremwar pushed a commit to Soremwar/deno that referenced this pull request Oct 6, 2020
This ports the REPL over to Rust and makes use of an inspector session to run a REPL on top of any isolate which lets make full use of rustylines various things like validators and completors without having to introduce a bunch of hard to test internal ops and glue code.

An accidental but good side effect of this is that the multiple line input we previously had is now an editable multi-line input prompt that is correctly stored in the history as a single entry.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants