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

WIP repl inside deno, initial phase #947

Closed
wants to merge 13 commits into from
Closed

Conversation

cedric05
Copy link
Contributor

@cedric05 cedric05 commented Oct 9, 2018

current issues

  • evaluate real reason behind setTimeout not working
  • better global handler (currently deno is exiting whenever exception is happening)
  • repl should print of previous line execution (until we do console.log)

@cedric05 cedric05 changed the title repl loop inside deno, initial phase WIP repl loop inside deno, initial phase Oct 9, 2018
@cedric05 cedric05 changed the title WIP repl loop inside deno, initial phase WIP repl inside deno, initial phase Oct 9, 2018
@cedric05 cedric05 mentioned this pull request Oct 9, 2018
@hayd
Copy link
Contributor

hayd commented Oct 9, 2018

Perhaps repl_loop should be in a new file repl.rs?

rustyline looks pretty great 👍 . One thing worth noting is there's not (yet) tokio support kkawakam/rustyline#126 I'm not sure if that's a problem..

@cedric05
Copy link
Contributor Author

cedric05 commented Oct 9, 2018

i still node able to import deno package/functions into repl loop(after adding to global.ts its working, i think that is not correct way). i will work on repl with ts instead of js and will update here.
if anyone has any leads i'm happy to follow

@hayd yes, i will create a new file.

src/main.rs Outdated
@@ -68,6 +74,7 @@ fn main() {

log::set_logger(&LOGGER).unwrap();
let args = env::args().collect();
let args2: Vec<String> = env::args().collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

an alternative here is to do:

let start_repl = args.len() == 1

(You'll need to set args: Vec<String> above.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

Copy link
Contributor

Choose a reason for hiding this comment

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

I take this back, in future you're going to have to parse args. e.g. -D.

@cedric05
Copy link
Contributor Author

console is getting exited, whenever there is some exception, will work on that as well.

@hayd
Copy link
Contributor

hayd commented Oct 10, 2018

Also setTimeout doesn't seem to work, so I think there is an issue with the event loop too?

Edit: It seems to exit inside the libdeno::execute unsafe, but I haven't uncovered why.

@cedric05
Copy link
Contributor Author

setTimeout is not working but deno.mkdir is working for me. i will try test suite and see what else if failing.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Oct 10, 2018

@cedric05 in event_loop implementation, there are some special rx.recv_timeout that deals with timeout (along with async response). Also, I could not find code that checks ntasks in repl_loop as event_loop would do

@kevinkassimo
Copy link
Contributor

My intuition is that we might want to run repl_loop on another thread (such that we can spawn an event loop to its side that does not block by repl). Also, I feel we can use the trick of setting a huge timeout that ensures ntasks for event loop is always more than 1 to prevent exiting.

@kevinkassimo
Copy link
Contributor

Also, current global error handler for libdeno would cause the program to exit whenever an exception happens.

deno/js/main.ts

Lines 23 to 36 in 8ada287

function onGlobalError(
message: string,
source: string,
lineno: number,
colno: number,
error: any // tslint:disable-line:no-any
) {
if (error instanceof Error) {
console.log(error.stack);
} else {
console.log(`Thrown: ${String(error)}`);
}
os.exit(1);
}

I could help look into REPL this weekend if possible

@cedric05
Copy link
Contributor Author

yes @kevinkassimo that global error handler really helped, i will keep posting my current status. please take over what all you can pick.

@cedric05
Copy link
Contributor Author

i'm looking at compilation of ts to js on the fly for repl now.

@hayd
Copy link
Contributor

hayd commented Oct 10, 2018

It seems like with the global error change it works fine with errors! (Interestingly throw behaves a little differently / source is undefined in onGlobalError.)

The event_loop call is blocking until all setTimeouts are finished, so it's not running at all inside the loop. If you add event_loop to after the execute (inside the loop) then it works... waiting for all setTimeouts to complete - which is probably not desired but shows it works!

One thing that doesn't work yet is multi-line input. kkawakam/rustyline#79
Edit: although node doesn't support fully interactive multiline either 😢 .

Run event_loop after each input line
@hayd
Copy link
Contributor

hayd commented Oct 11, 2018

Some more of the way there hayd@e5cb43c

asciicast

Edit: Modulo timeouts: each line waits until all the timeouts are finished... which personally I prefer anyway - ha! Or compiling...

@hayd
Copy link
Contributor

hayd commented Oct 12, 2018

What's a good way to approach the ts compilation? From a ts (rust) String to a js String.

@cedric05
Copy link
Contributor Author

cedric05 commented Oct 12, 2018

my thought process is to expose some api main.ts and do some thing like
https://github.com/denoland/deno/blob/master/src/main.rs#L75
isolate.execute('compile_to_js.ts', !format("compile({})", stdin))
@hayd
i'm still working on that.
which branch are you working on? i will pull your changes as well

@hayd
Copy link
Contributor

hayd commented Oct 12, 2018

@cedric05 I only really got this far: hayd@e5cb43c (one commit after a rebase of your work).

@hayd
Copy link
Contributor

hayd commented Oct 12, 2018

Perhaps the alternative is to create a flatbuffers table/result and do the compile and eval in js... ?
All the other fbs messages go the other way, so that may be a bad approach?

@bartlomieju
Copy link
Member

bartlomieju commented Oct 12, 2018

AFAICT we need to be able to call compiler.run multiple times to get TS compilation working for REPL. Right now it's not possible without creating new deno instance. Maybe we should add second entry point (repl.ts) just for REPL?

@cedric05
Copy link
Contributor Author

trying to understand https://github.com/denoland/deno/blob/master/js/compiler_test.ts#L292, we will have to do the same for repl IMO

@kevinkassimo
Copy link
Contributor

@kitsonk for compiler questions

@kitsonk
Copy link
Contributor

kitsonk commented Oct 12, 2018

Talking with @ry yesterday, the objective for the first REPL would be direct JavaScript support, not supporting TypeScript. I think the REPL for TypeScript will require a bit of change to the compiler or morel likely additional APIs and a specific runtime context that can be built up over time for evaluating types, which trust me, is something that we should build on once the initial REPL is there, versus trying to deliver it in one go.

@hayd
Copy link
Contributor

hayd commented Oct 12, 2018

Executing javascript is pretty much there. Though as mentioned above doesn't print the returned value (since that's not returned by libdeno::execute)...

At the moment this is relying on libdeno::execute BUT in order to do the compile IIUC this is going to have to happen in ts? So I wonder if we're structuring this wrong:

Currently:
rustyline (await input) -> line -> libdeno::execute in rust -> (repeat)

Maybe it needs to be:
ts sends fb::Repl -> rustyline (await input) -> fb::ReplRes(line) -> compile + execute in ts -> (repeat)

It seems like this is necessary in order to do ts at some point it needs evaluate in ts?
Aside: I think this would also fix the event_loop being blocking issue.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 12, 2018

@hayd I would still try to tackle the TS as a seperate PR, but looking at how ts-node provides REPL functionality for TypeScript on top of Node.js could help inspire what we need to do.

@hayd
Copy link
Contributor

hayd commented Oct 12, 2018

@kitsonk I guess I am asking if initially the (javascript) eval should happen in the ts-side (rather than rust) if we're going to have to move that way in future...

Node exports https://nodejs.org/api/repl.html and https://nodejs.org/api/vm.html which ts-node relies on. https://github.com/TypeStrong/ts-node/blob/master/src/bin.ts#L163-L190 So I think it makes sense to do the same in deno (even if we're initially only eval-ing and not compiling)...

cedric05 and others added 2 commits October 14, 2018 18:20
Run event_loop after each input line
@hayd
Copy link
Contributor

hayd commented Oct 15, 2018

So, here's a version that evals javascript: hayd@e1a9b03

The loop is:

export async function repl_loop() {
  while(true){
    let line = await readline('>> ')  // this calls rust for the entered text
    try {
      let result = eval.call(window, line);;
      if (result) { console.log(result) };
    } catch (err) {
      console.log(err);
    }
  }

set timeouts work! and it's non-blocking.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 15, 2018

Just thinking out loud, should we have a different global scope for the REPL, one in particular that doesn't require the import of the deno namespace? There are valid reasons to leave deno as a module in regular userland, but having to import * as deno from "deno" on most REPL commands seems a bit silly.

@cedric05
Copy link
Contributor Author

@hayd pushed your changes, and added deno to global namespace.
any idea why tests couldn't pull third_party code ?

@cedric05
Copy link
Contributor Author

@ry @kitsonk need some insights

@cedric05 cedric05 closed this Oct 15, 2018
@cedric05 cedric05 reopened this Oct 15, 2018
@cedric05
Copy link
Contributor Author

windows build keeps failing, i need to setup my windows machine.
i have one more out of context doubt? do deno really need xcode for developing in mac? , i don't have enough disk space for xcode.

@cedric05
Copy link
Contributor Author

is this expected??

Expected: Error: bad
    at foo (file://[WILDCARD]tests/error_001.ts:2:9)
    at bar (file://[WILDCARD]tests/error_001.ts:6:3)
    at eval (file://[WILDCARD]tests/error_001.ts:9:1)
    at DenoCompiler.eval [as _globalEval] (<anonymous>)
    at DenoCompiler._gatherDependencies ([WILDCARD]/js/compiler.ts:[WILDCARD])
    at DenoCompiler.run ([WILDCARD]/js/compiler.ts:[WILDCARD])
    at denoMain ([WILDCARD]/js/main.ts:[WILDCARD])
    at deno_main.js:1:1
Actual:   Error: bad
    at foo (file:///Users/travis/build/denoland/deno/tests/error_001.ts:2:9)
    at bar (file:///Users/travis/build/denoland/deno/tests/error_001.ts:6:3)
    at eval (file:///Users/travis/build/denoland/deno/tests/error_001.ts:9:1)
    at DenoCompiler.eval [as _globalEval] (<anonymous>)
    at DenoCompiler._gatherDependencies (deno/js/compiler.ts:201:10)
    at DenoCompiler.run (deno/js/compiler.ts:535:12)
    at denoMain (deno/js/main.ts:93:14)
    at deno_main.js:1:1
Source: /Users/travis/build/denoland/deno/tests/error_001.ts

@hayd
Copy link
Contributor

hayd commented Oct 15, 2018

You might be better to checkout my commit/branch and force push that here. I wonder if there was an issue in your merge conflict resolution.

I didn't try running the tests yet. Let me look at adding a repl test.

Now it's in the ts side we can accept multi-line (though only edit the last line) and later compile. is there anything else we're missing?

@hayd hayd mentioned this pull request Oct 16, 2018
@cedric05 cedric05 closed this Oct 16, 2018
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.

5 participants