Update to new REPL APIs

[mfikes]

Changes are being made to cljs.repl that will require updates to derivative REPLs.

David Nolen indicated that he wants to allow time to allow REPLs to update before the code in the latest ClojureScript tree is landed as a release.

I suspect one consequence is that the (in-ns ...) form produces an arity exception if tried with the latest ClojureScript tree.

(Note: It is not at all clear to me whether anything needs to be done in Weasel. It could be that all of the needed changes are lower down in the stack.)

[tomjakubowski]

Thanks for the heads up! I'm currently traveling but will see if I can squeeze any necessary fixes in in the coming week (and patches are always welcome :wink:)

[mfikes]

Hey Tom, I'll attempt to put together some Weasel revisions for this.

[mfikes]

I did some exploratory hacking: fc4c2230e6042e83a9cf7e072531fe47ed4d72a3

In that commit, there is a lot of commented code in websocket-setup-env that was essentially copied from the new Node.js REPL impl.

Interesting is the interaction between the REPL and the target execution environment. Copied below is some IRC dialog with David Nolen regarding this:

[12:33:53]  <mfikes>    dnolen_: I've been looking at updating weasel. The -setup impl for the nodejs REPL launches node and has it compile & load cljs.core, making use of node-eval. For weasel, -setup runs without yet having access to the target environment, and can only -evaluate after target has connected. But perhaps this is a non-issue as the "target" will already have cljs.core running when it connects.
[12:37:57]  <dnolen_>   mfikes: -setup needs block until it eval
[12:38:29]  <dnolen_>   mfikes: happy to think through why this is a bad idea, but a REPL that's started that can't eval doesn't make much sense to me
[12:38:47]  <dnolen_>   s/until it eval/until it can eval
[12:38:49]  <mfikes>    dnolen_: OK. That was an alternative conclusion I came to as well.
[12:39:40]  <dnolen_>   mfikes: without this everything just gets more difficult - we can't universally load `cljs.repl/doc` or whatever other helpers we need.
[12:40:10]  <dnolen_>   mfikes: so it's a pain for REPL implementers but it makes the experience consistent everywhere
[12:40:10]  <dnolen_>   which IMO is more valuable
[12:51:07]  <mfikes>    dnolen_: I'll try the blocking approach. I wonder about the consequences of compiling and loading cljs.core when the client target environment connects. In node's case, it seems like an empty/fresh JS environment is being "filled with CLJS", while with weasel the client is already "filled". (I'm still trying to get the big picture straight in my head.)
[12:53:52]  <dnolen_>   mfikes: sort of, Weasel analyzes the src directory - but really it should just load cached analysis off disk same as Node.js REPL
[12:54:13]  <dnolen_>   mfikes: in the Node.js case all the compilation bits don't do anything if everything has already been compiled
[12:54:46]  <dnolen_>   mfikes: so in the new world you should point Weasel at :output-dir not src
[12:55:05]  <mfikes>    dnolen_ Yep. I was thinking that in weasel, the (cljs.repl/analyze-source (:src this)) would go away.
[12:55:57]  <dnolen_>   mfikes: fwiw Node.js REPL also technically needs to block, currently 300ms Thread/sleep
[12:56:08]  <dnolen_>   temporary hack
[12:57:05]  <mfikes>    dnolen_: I'll give it a shot. I would imagine that when the client environment connects, its existing cljs.core will be replaced/overwritten when -setup runs, if I'm understanding correctly.
[12:57:34]  <dnolen_>   mfikes: no, because :output-dir is not stale
[12:57:53]  <mfikes>    dnolen_: Ahh. OK.
[12:58:21]  <dnolen_>   mfikes: in essence the new REPL model is closer to the truth of the compiled state
[12:58:39]  <dnolen_>   mfikes: you can launch a REPL from any :output-dir and it will exactly match what your build looks like

[mfikes]

The updates to the REPL apis are now available in ClojureScript 0.0-2644.

[mfikes]

Note that if you try using the updates in my experimental fork, you will encounter an error about *print-fn*. I thought this was just something that needs to be addressed in the fork, but now think it may be a ClojureScript defect CLJS-956.

[the-kenny]

@mfikes do you know if this also fixes the issue with in-ns from https://github.com/tomjakubowski/weasel/issues/26 ?

[mfikes]

@the-kenny No and yes. I suspect that #26 could easily become obsolete with the new REPL APIs, simply because there is some new explicit support for in-ns and other REPL "special forms", and some changes are needed to support in-ns.

Prior to 0.0-2644, I wasn't experiencing #26 (I am using Cursive, so perhaps there is enough difference there).

But, regardless, with the new REPL APIs, I get a fundamental ArityException when invoking in-ns. In my experimental fork to try to understand the needed Weasel changes, I haven't sorted out what the root cause of this is. Perhaps it is in piggyback, and will also affect Austin... don't know yet.

[the-kenny]

26 isn't really about in-ns directly: Calling it from the REPL itself worked fine. The problem was in the nrepl-protocol.

[mfikes]

Here is the ArityException I get for in-ns:

(in-ns 'foo.bar)
ArityException Wrong number of args (2) passed to: repl/fn--4253/self--4259  clojure.lang.AFn.throwArity (AFn.java:429)

[mfikes]

There is an interesting design issue to sort out: The Node.js REPL bootstraps by loading cljs.core into an initially empty Node.js environment, and all loading is accomplished by directly loading JavaScript source from the filesystem via an override to CLOSURE_IMPORT_SCRIPT.

It is not clear to me how this should end up working for Weasel, which has a Weasel client connect to the Weasel server, where the client already has cljs.core loaded along with at least enough JavaScript to support the (ws-repl/connect "ws://localhost:9001") call. (This concept was discussed with David Nolan a bit in the IRC log copied into the previous comment.)

It seems like two drastically different approaches could be taken:

1. Make radical changes to have some minimal bootstrap JavaScript start listening on a WebSocket, and have the Weasel REPL load in things just like the Node.js REPL does.
2. Continue to have the client call ws-repl/connect, and make assumptions in the Weasel server that the client already has certain libs pre-loaded.

The first approach would appear to result in closer tracking between the compiled state and what is actually running in the JavaScript environment, but it seems like it presents a lot of other challenges.

In any case, another challenge is that, unlike in the Node.js REPL, Weasel doesn't know the details of the target JavaScript execution environment, perhaps making it challenging to write a proper override to CLOSURE_IMPORT_SCRIPT. (One thing to consider: when targeting mobile JavaScript engines, the target environment likely doesn't have filesystem access to the compilation output directory, and CLOSURE_IMPORT_SCRIPT would have to load files perhaps via the Weasel TCP connection.)

My experimental fork currently has code in it that mimic's the Node.js REPL in some respects, but it hasn't addressed CLOSURE_IMPORT_SCRIPT and arguably currently behaves a lot like the currently-released Weasel implementation.

[mfikes]

Much of what I described in the previous comment is probably answered by studying the way the new browser REPL operates with the latest ClojureScript updates.

[mfikes]

It appears that Weasel minimally needs to be updated to support the new 2-arg arity of -setup in IJavaScriptEnv that was introduced in ClojureScript 0.0-2665. It is not clear if anything more must be done at this point; with this change, things work. I'm presuming that the ArityException associated with in-ns is lower down in the stack. I'll put together a PR addressing this.

[martinklepsch]

Probably relevant to this issue: https://github.com/cemerick/piggieback/issues/38

[mfikes]

Yes, @martinklepsch I can confirm that with the changes in cemerick/piggieback#38 my (in-ns ...) forms work properly.

[the-kenny]

Do just the (in-ns ...) forms work properly, or do the keyboard shortcuts in Emacs (C-c M-n, (cider-repl-set-ns NS) work fine too?

[mfikes]

@the-kenny I am using Cursive, with the entire stack involving simple-brepl to start Weasel. In Cursive, both manually typing (in-ns 'foo) and using Tools > REPL > Switch REPL NS to current file menu work.

[mfikes]

This ticket can be closed IMHO, especially in light of work that will probably be done for #42

[tomjakubowski]

In particular we should ensure that we're meeting the expectations noted here: https://github.com/clojure/clojurescript/wiki/Custom-REPLs.

[swannodette]

wrt to c9f6b08, this is good enough for browser based contexts which is the primary use case for Weasel far as I know. If Weasel is being used for other targets then yes you may need to configure goog.require monkey patching. Seems pretty low priority and honestly better handled by making a new REPL for dramatically different targets a la Ambly.