Add repl #998

hayd · 2018-10-16T06:01:30Z

Rebase and work upon #947. cc @cedric05

Add tests in form of repl_test.py.
Executes js.
No compiling from ts.

Note: atm the scope of eval is window + window.deno. (I haven't been able to get the tests passing any of the suggested alternatives) @kitsonk

kitsonk

Some feedback.

kitsonk · 2018-10-16T08:52:46Z

js/repl.ts

+  // const replScope = Object.create(window);
+  // Object.defineProperty(replScope, "deno", { value: deno, enumerable: true });
+  // const replScope = Object.defineProperty("deno", Object.create(window), { value: deno, enumerable: true });
+  // const replScope = {"deno": deno, ...window};


This could be:

const replScope = { deno, ...window };

That is the most concise, but it will only deal with enumerable own properties on window, where as Object.create would set the prototype of a new object to window. I would think it is fine for now, but we may want to note, comment, that we are aware of the limitation (enumerable own properties).

sidenote: I just noticed that many of globals have strange enumerability in Node.js and Chrome. For example in Chrome, window.console is not enumerable but in Node.js it is enumerable.

This (along with eval.call) didn't seem to update variables

var a = 1
a

I suspect there's a permutation that works but I haven't found it!

I'll try again.

I'm still unable to get it.

js/repl.ts

kitsonk · 2018-10-16T08:56:08Z

js/repl.ts

+  assert(baseRes != null);
+  assert(msg.Any.ReplRes === baseRes!.innerType());
+  const inner = new msg.ReplRes();
+  assert(baseRes!.inner(inner) != null);


We should always comment when we use the not null assertion operator.

Hmmm this is copy/pasted. Which is to say, I am not sure what to comment here.

Suggested change

assert(baseRes!.inner(inner) != null);

// TypeScript does not track asserts, baseRes asserted above, therefore not null here

assert(baseRes!.inner(inner) != null);

kitsonk · 2018-10-16T09:10:11Z

js/repl.ts

+    if (line === ".exit") { break }
+    try {
+      let result = eval.call(window, line);
+      // let result = eval.call(replScope, line);


We should use this line, but it should be a const. There is no reassignment within the block. Again, the tslint would error on this.

js/repl.ts

tools/repl_test.py

hayd · 2018-10-17T17:03:01Z

I've removed my attempt at having a single rustyline Editor instance. It seems fast even when the history file starts with 5k lines.

I am not sure where to put history file. I think the scope change could be made later.

ry · 2018-10-17T17:06:43Z

I apologize for the lack of reviews here. I'm glad you guys are working on this. But I want to figure out if we're going to move the event loop into JS (#1003) before landing any REPL code.

src/repl.rs

bartlomieju · 2018-10-21T12:37:11Z

EDIT: @hayd I managed to get REPLs stored on IsolateState, and refactored it to struct. History loading is done from name of the repl so we can handle multiple of them as you suggested. Feel free to cherry-pick those commits.

You can check it out here: https://github.com/bartlomieju/deno/commits/repl

hayd · 2018-10-22T05:56:06Z

@bartlomieju 👍 I've merged it to this branch and rebased, but github is still playing catch up/down.

bartlomieju · 2018-10-23T10:14:29Z

@hayd awesome 👍 I think we can start figuring out how to compile TS to JS. @kitsonk can you give us a hand here and point into right direction?

hayd · 2018-10-24T16:33:22Z

@ry this is mostly ts side now, originally it was calling out isolate's event_loop but now it's not. So it shouldn't be affected by event loop changes.

Perhaps it should be:

🦕>

ry · 2018-10-24T17:42:56Z

@hayd I want to get #1081 in before looking into this one.
And no emoji in the prompt. "> " is sufficient.

hayd · 2018-10-28T03:56:08Z

rebased. Build fail due to new third_party commit not in denoland repo.

Strangely #1087 doesn't appear to be an issue any more in the repl (or perhaps I've forgotten how to trigger it).

Squashed from cbef6b6.

piscisaureus · 2018-11-03T20:15:05Z

@hayd - you can work around the issue with this patch: piscisaureus@repl_send

cc @ry

hayd · 2018-11-03T23:18:09Z

The build/tests are now green on appveyor + travis.

ry

This work is excellent! I'm really excited. I want to try to land this for 0.1.11.

I have a few superficial comments...

src/repl.rs

src/deno_dir.rs

src/isolate.rs

ry · 2018-11-04T14:25:14Z

src/repl.rs

+  }
+}
+
+pub struct DenoRepl {


Rename to Repl or Linereader (?)

Linereader.readline 🤢 . hmmm

But you're right, the loop (eval / print) is not in rust. So it's not a repl.

linereader.read sounds fine

ry · 2018-11-04T14:27:02Z

tools/repl_test.py

+
+if __name__ == "__main__":
+    deno_exe = os.path.join(build_path(), "deno" + executable_suffix)
+    repl_tests(deno_exe)


src/isolate.rs

src/msg.fbs

bartlomieju · 2018-11-04T15:55:44Z

@hayd do you need help with requested changes?

hayd · 2018-11-04T16:00:19Z

@bartlomieju yes please, if you could add to resources table, change name to history_file etc. would be very helpful, thanks 👍

ry · 2018-11-05T04:56:05Z

@hayd @bartlomieju I've made the changes to move the REPL into the resources table myself and pushed to this branch... because it's a little tricky. (We're going to refactor the resources table soon - to hopefully be less haphazard.)

I've also changed "readline()" to be a synchronous function. Mostly because its simpler to do... but also it might be more correct? Do you really want output printed to the console while you're interacting with the REPL? I suppose it depends on the situation - but for debugging - generally not.

hayd · 2018-11-05T05:14:41Z

Awesome! Thanks @ry.

The async behavior is from node (and maybe other languages too), but I agree it's kinda strange.... at least, it's strange if you're logging output.

hayd · 2018-11-05T06:57:24Z

@ry I've addressed the other feedback I think (aside from it still being called Repl).

I suspect we'll need to do async (at some point). A cool thing you can try is running the repl with debug:

$ ./out/debug/deno -D
DEBUG RS - starting background reactor
DEBUG RS - msg_from_js Start sync true
DEBUG JS - cwd /Users/hayd/OSS/deno
DEBUG JS - args []
DEBUG RS - op_repl_start deno_history.txt
DEBUG RS - Loading REPL history: "/Users/hayd/.deno/deno_history.txt"
DEBUG RS - msg_from_js ReplStart sync true
DEBUG RS - op_repl_readline 3 >
> DEBUG RS - key: Char('a')
DEBUG RS - Emacs command: SelfInsert(1, 'a')
DEBUG RS - Changeset::insert(0, 'a')
> aDEBUG RS - key: Char('b')
DEBUG RS - Emacs command: SelfInsert(1, 'b')
DEBUG RS - Changeset::insert(1, 'b')
> abDEBUG RS - key: Char('c')
DEBUG RS - Emacs command: SelfInsert(1, 'c')
DEBUG RS - Changeset::insert(2, 'c')
> abc

Every key stroke logged, yet the repl remains functional... which is nice.

ry · 2018-11-05T14:37:01Z

I suspect we'll need to do async (at some point).

Yah, I think so too...

ry

LGTM!

- Running repl from js side. - Add tests for repl behavior. - Handle ctrl-C and ctrl-D.

- Performance and stability improvements on all platforms. - Add repl (denoland#998) - Add deno.Buffer (denoland#1121) - Support cargo check (denoland#1128) - Upgrade Rust crates and Flatbuffers. (denoland#1145, denoland#1127) - Add helper to turn deno.Reader into async iterator (denoland#1130) - Add ability to load JSON as modules (denoland#1065) - Add deno.resources() (denoland#1119) - Add application/x-typescript mime type support (denoland#1111)

- Performance and stability improvements on all platforms. - Add repl (#998) - Add deno.Buffer (#1121) - Support cargo check (#1128) - Upgrade Rust crates and Flatbuffers. (#1145, #1127) - Add helper to turn deno.Reader into async iterator (#1130) - Add ability to load JSON as modules (#1065) - Add deno.resources() (#1119) - Add application/x-typescript mime type support (#1111)

kitsonk reviewed Oct 16, 2018

View reviewed changes

hayd force-pushed the repl branch from 8c35525 to 5254181 Compare October 16, 2018 17:00

bartlomieju reviewed Oct 19, 2018

View reviewed changes

src/repl.rs Outdated Show resolved Hide resolved

src/repl.rs Outdated Show resolved Hide resolved

hayd force-pushed the repl branch from c10b9a3 to 8594073 Compare October 20, 2018 02:24

hayd force-pushed the repl branch from 0ca7336 to ca9757c Compare October 22, 2018 04:36

hayd mentioned this pull request Oct 22, 2018

WIP Permission prompt #1018

Closed

3 tasks

hayd force-pushed the repl branch from 43ac896 to ef8ff71 Compare October 28, 2018 03:54

hayd force-pushed the repl branch 6 times, most recently from be8922c to c560981 Compare November 3, 2018 07:13

hayd force-pushed the repl branch from 042bf77 to 6d4ba09 Compare November 3, 2018 23:03

ry reviewed Nov 4, 2018

View reviewed changes

src/isolate.rs Outdated Show resolved Hide resolved

ry reviewed Nov 4, 2018

View reviewed changes

src/msg.fbs Show resolved Hide resolved

ry reviewed Nov 4, 2018

View reviewed changes

src/msg.fbs Outdated Show resolved Hide resolved

hayd force-pushed the repl branch from 019f6da to 9f7c07b Compare November 5, 2018 07:16

ry approved these changes Nov 5, 2018

View reviewed changes

ry force-pushed the repl branch from 0f66300 to 3f6a7e0 Compare November 5, 2018 16:07

Add repl

1aa3696

- Running repl from js side. - Add tests for repl behavior. - Handle ctrl-C and ctrl-D.

ry force-pushed the repl branch from 3f6a7e0 to 1aa3696 Compare November 5, 2018 16:28

ry merged commit 27ecfc1 into denoland:master Nov 5, 2018

hayd deleted the repl branch November 5, 2018 18:08

ry mentioned this pull request Nov 5, 2018

v0.1.11 #1157

Merged

kitsonk mentioned this pull request Nov 5, 2018

TypeScript support in REPL #1158

Closed

hayd mentioned this pull request Nov 6, 2018

setTimeout does not work in REPL #1161

Closed

kt3k mentioned this pull request Oct 23, 2021

chore(node/_tools/setup): Improve cli messages denoland/std#1446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add repl #998

Add repl #998

hayd commented Oct 16, 2018 •

edited

Loading

kitsonk left a comment

kitsonk Oct 16, 2018

hayd Oct 16, 2018

hayd Oct 16, 2018

hayd Oct 16, 2018

kitsonk Oct 16, 2018

hayd Oct 16, 2018 •

edited

Loading

kitsonk Oct 22, 2018

kitsonk Oct 16, 2018

hayd commented Oct 17, 2018

ry commented Oct 17, 2018

bartlomieju commented Oct 21, 2018 •

edited

Loading

hayd commented Oct 22, 2018

bartlomieju commented Oct 23, 2018

hayd commented Oct 24, 2018

ry commented Oct 24, 2018

hayd commented Oct 28, 2018 •

edited

Loading

piscisaureus commented Nov 3, 2018 •

edited

Loading

hayd commented Nov 3, 2018

ry left a comment

ry Nov 4, 2018

hayd Nov 4, 2018

hayd Nov 4, 2018 •

edited

Loading

ry Nov 4, 2018

ry Nov 4, 2018

bartlomieju commented Nov 4, 2018

hayd commented Nov 4, 2018

ry commented Nov 5, 2018 •

edited

Loading

hayd commented Nov 5, 2018

hayd commented Nov 5, 2018 •

edited

Loading

ry commented Nov 5, 2018

ry left a comment

	assert(baseRes!.inner(inner) != null);
	// TypeScript does not track asserts, baseRes asserted above, therefore not null here
	assert(baseRes!.inner(inner) != null);

Add repl #998

Add repl #998

Conversation

hayd commented Oct 16, 2018 • edited Loading

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayd Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayd commented Oct 17, 2018

ry commented Oct 17, 2018

bartlomieju commented Oct 21, 2018 • edited Loading

hayd commented Oct 22, 2018

bartlomieju commented Oct 23, 2018

hayd commented Oct 24, 2018

ry commented Oct 24, 2018

hayd commented Oct 28, 2018 • edited Loading

piscisaureus commented Nov 3, 2018 • edited Loading

hayd commented Nov 3, 2018

ry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayd Nov 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Nov 4, 2018

hayd commented Nov 4, 2018

ry commented Nov 5, 2018 • edited Loading

hayd commented Nov 5, 2018

hayd commented Nov 5, 2018 • edited Loading

ry commented Nov 5, 2018

ry left a comment

Choose a reason for hiding this comment

hayd commented Oct 16, 2018 •

edited

Loading

hayd Oct 16, 2018 •

edited

Loading

bartlomieju commented Oct 21, 2018 •

edited

Loading

hayd commented Oct 28, 2018 •

edited

Loading

piscisaureus commented Nov 3, 2018 •

edited

Loading

hayd Nov 4, 2018 •

edited

Loading

ry commented Nov 5, 2018 •

edited

Loading

hayd commented Nov 5, 2018 •

edited

Loading