search

nrepl / weasel

ClojureScript browser REPL using WebSockets
The Unlicense
324 stars 35 forks source link

Weasel should wait to connect #51

Closed swannodette closed 9 years ago

swannodette commented 9 years ago

The current behavior that Weasel relies on is undesirable for many other REPLs. Other REPLs would like to abort if -setup fails. I think an error in -setup should be considered a hard failure. However I don't want to change this logic until Weasel has made this change. I know that Ambly would prefer to fail. @bhauman does Figwheel keep going even if -setup has obviously failed?

bhauman commented 9 years ago

I wait until connect. I could add a really long timeout. I want to give people more than enough time to get things up and running. I don't want require that folks have their browsers open first.

The nature of browser development requires handling browser refreshes.

Also figwheel is operating in and env where cljs.core is already loaded so setup is only waiting for connect.

On Mon, Feb 23, 2015 at 9:30 AM, David Nolen notifications@github.com wrote:

The current behavior that Weasel relies on is undesirable for many other REPLs. Other REPLs would like to abort if -setup fails. I think an error in -setup should be considered a hard failure. However I don't want to change this logic until Weasel has made this change. I know that Ambly would prefer to fail. @bhauman https://github.com/bhauman does Figwheel keep going even if -setup has obviously failed?

— Reply to this email directly or view it on GitHub https://github.com/tomjakubowski/weasel/issues/51.

swannodette commented 9 years ago

@bhauman OK that clarifies things. I believe then that Weasel is the only REPL with this behavior, which means even less incentive to continue supporting it.

tomjakubowski commented 9 years ago

I support changing Weasel to wait on setup for a client to connect.

I believe @the-kenny raised some objections to changing to this model, although I think he specifically was concerned about being able to refresh a connected browser page without bringing down the REPL. I am concerned though that browser refreshes during a REPL session could easily cause the state of the REPL's environment to disagree with the connected browser page's state, like vars being defined in the REPL (because of some prior REPL interactions) but not in the browser (after a refresh) for example.

the-kenny commented 9 years ago

What about 'degrading' the state to 'waiting for browser to connect' when refreshing a browser? That could either be a full reset, or we could (at least) keep the current namespace the same. I think that's a reasonable compromise.

the-kenny commented 9 years ago

@tomjakubowski any update on this?

tomjakubowski commented 9 years ago

@the-kenny I've pushed two changes:

1) websocket-setup-env waits for a client to connect before returning 2) Attempting to evaluate some form will block a running REPL if a client is not connected to it.

I've done some basic tests and it seems to work fine but let me know if you encounter any problems! I'll note that I'm on clojurescript 2850 and piggieback 0.1.5 still.

the-kenny commented 9 years ago

Hey! The waiting code seems to work pretty fine, but much of the functionality of weasel is broken with piggieback 0.2.0. I'll try with 0.1.5 in a moment.

the-kenny commented 9 years ago

Nope. No cigar.

[org.clojure/clojurescript "0.0-3178"] [weasel "0.7.0-SNAPSHOT"] [com.cemerick/piggieback "0.1.5"]

1. Unhandled clojure.lang.ExceptionInfo
   No such namespace: cljsjs.react, could not locate cljsjs/react.cljs
   at line 1
   file:/home/moritz/.m2/repository/org/omcljs/om/0.8.8/om-0.8.8.jar!/om/dom.cljs
   {:tag :cljs/analysis-error,
    :file
    "file:/home/moritz/.m2/repository/org/omcljs/om/0.8.8/om-0.8.8.jar!/om/dom.cljs",
    :line 1,
    :column 1}

                      core.clj: 4403  clojure.core/ex-info
                  analyzer.clj:  379  cljs.analyzer/error
                  analyzer.clj:  376  cljs.analyzer/error
                  analyzer.clj: 1245  cljs.analyzer/analyze-deps
                  analyzer.clj: 1496  cljs.analyzer/eval11477/fn
                  MultiFn.java:  249  clojure.lang.MultiFn/invoke
                  analyzer.clj: 1839  cljs.analyzer/analyze-seq
                  analyzer.clj: 1931  cljs.analyzer/analyze/fn
                  analyzer.clj: 1924  cljs.analyzer/analyze
                  analyzer.clj: 2160  cljs.analyzer/analyze-file/fn
                  analyzer.clj: 2155  cljs.analyzer/analyze-file
                  analyzer.clj: 1242  cljs.analyzer/analyze-deps
                  analyzer.clj: 1496  cljs.analyzer/eval11477/fn
                  MultiFn.java:  249  clojure.lang.MultiFn/invoke
                  analyzer.clj: 1839  cljs.analyzer/analyze-seq
                  analyzer.clj: 1931  cljs.analyzer/analyze/fn
                  analyzer.clj: 1924  cljs.analyzer/analyze
                  analyzer.clj: 2160  cljs.analyzer/analyze-file/fn
                  analyzer.clj: 2155  cljs.analyzer/analyze-file
                  analyzer.clj: 1242  cljs.analyzer/analyze-deps
                  analyzer.clj: 1496  cljs.analyzer/eval11477/fn
                  MultiFn.java:  249  clojure.lang.MultiFn/invoke
                  analyzer.clj: 1839  cljs.analyzer/analyze-seq
                  analyzer.clj: 1931  cljs.analyzer/analyze/fn
                  analyzer.clj: 1924  cljs.analyzer/analyze
                  analyzer.clj: 2160  cljs.analyzer/analyze-file/fn
                  analyzer.clj: 2155  cljs.analyzer/analyze-file
                  analyzer.clj: 1242  cljs.analyzer/analyze-deps
                  analyzer.clj: 1496  cljs.analyzer/eval11477/fn
                  MultiFn.java:  249  clojure.lang.MultiFn/invoke
                  analyzer.clj: 1839  cljs.analyzer/analyze-seq
                  analyzer.clj: 1931  cljs.analyzer/analyze/fn
                  analyzer.clj: 1924  cljs.analyzer/analyze
                      repl.clj:  417  cljs.repl/evaluate-form
                      repl.clj:  414  cljs.repl/evaluate-form
                      repl.clj:  412  cljs.repl/evaluate-form
                      repl.clj:  479  cljs.repl/load-stream
                      repl.clj:  511  cljs.repl/load-file
                      repl.clj:  604  cljs.repl/fn/self
                piggieback.clj:  107  cemerick.piggieback/cljs-eval/fn
                      AFn.java:  152  clojure.lang.AFn/applyToHelper
                      AFn.java:  144  clojure.lang.AFn/applyTo
                      core.clj:  624  clojure.core/apply
                      core.clj: 1862  clojure.core/with-bindings*
                   RestFn.java:  425  clojure.lang.RestFn/invoke
                piggieback.clj:   97  cemerick.piggieback/cljs-eval
                      AFn.java:  165  clojure.lang.AFn/applyToHelper
                      AFn.java:  144  clojure.lang.AFn/applyTo
                      core.clj:  626  clojure.core/apply
                piggieback.clj:  180  cemerick.piggieback/cljs-repl/fn
                   RestFn.java:  436  clojure.lang.RestFn/invoke
                      Var.java:  388  clojure.lang.Var/invoke
                          REPL:    1  cljs.user/eval17947
                 Compiler.java: 6703  clojure.lang.Compiler/eval
                 Compiler.java: 6666  clojure.lang.Compiler/eval
                      core.clj: 2927  clojure.core/eval
                      main.clj:  239  clojure.main/repl/read-eval-print/fn
                      main.clj:  239  clojure.main/repl/read-eval-print
                      main.clj:  257  clojure.main/repl/fn
                      main.clj:  257  clojure.main/repl
                   RestFn.java: 1523  clojure.lang.RestFn/invoke
        interruptible_eval.clj:   67  clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn
                      AFn.java:  152  clojure.lang.AFn/applyToHelper
                      AFn.java:  144  clojure.lang.AFn/applyTo
                      core.clj:  624  clojure.core/apply
                      core.clj: 1862  clojure.core/with-bindings*
                   RestFn.java:  425  clojure.lang.RestFn/invoke
        interruptible_eval.clj:   51  clojure.tools.nrepl.middleware.interruptible-eval/evaluate
        interruptible_eval.clj:  183  clojure.tools.nrepl.middleware.interruptible-eval/interruptible-eval/fn/fn
        interruptible_eval.clj:  152  clojure.tools.nrepl.middleware.interruptible-eval/run-next/fn
                      AFn.java:   22  clojure.lang.AFn/run
       ThreadPoolExecutor.java: 1142  java.util.concurrent.ThreadPoolExecutor/runWorker
       ThreadPoolExecutor.java:  617  java.util.concurrent.ThreadPoolExecutor$Worker/run
                   Thread.java:  745  java.lang.Thread/run
the-kenny commented 9 years ago

Note that this is also reproducible in weasel-example by adding [org.omcljs/om "0.8.8"] to :dependencies and adding [om.core :as om] to :requires in weasel-example.example.

tomjakubowski commented 9 years ago

Blehhhh. I always forget to test with dependencies. Not sure if it's worth trying to fix this on 0.1.5, or if it should go in with the other changes for piggieback 0.2.0.

Is there a small reproduction available for that error?

the-kenny commented 9 years ago

Available here: https://github.com/the-kenny/weasel/tree/broken-om-dependency

the-kenny commented 9 years ago

Also note that the weasel-example project throws an error on repl-start/load because *print-fn* isn't set. That seems to mess up with ClojureScript loading causing weird errors like println not being found.

the-kenny commented 9 years ago

Ok, another update: I just upgraded some stuff in weasel's project.cljs, including piggieback. 0.2.0 seems to work just fine, including Om/React. I'll keep you updated...

the-kenny commented 9 years ago

Correction: It seems to work mostly. One notable exception is that commands run from cider doesn't seem to return. For example, C-c C-k loads the file just fine but it never prints the result in the minibuffer. The responses seem to get caught somewhere in the Nrepl middleware chain. (I suspect cider, but that needs more investigation).

So far, this is almost acceptable. I'll finally start working on my projects now and see if it's stable.

cichli commented 9 years ago

@the-kenny cider-nrepl has piggieback 0.2.0 support on master now, I think @bbatsov needs to push a new snapshot to clojars though.

the-kenny commented 9 years ago

@cichli Yup, that's what I'm using right now. Seems to work quite fine so far, with the exception of the missing responses.

cemerick commented 9 years ago

I believe the "missing responses" are due to cider expecting a value from the load-file operation. This is possible for Clojure, but not for ClojureScript AFAICT. Simply clearing the minibuffer some seconds after the "Loading $FILE" message is shown is the only thing we can do (assuming no errors).

Using weasel master with piggieback 0.2.0 works well here (modulo dropping the :repl-env keyword as noted elsewhere). I am getting some (spurious?) dependency fetch errors in the console, but I can't correlate them with actions of mine in any way yet.

erikcw commented 9 years ago

Another data point. I'm running:

weasel 0.6
clojurescript 0.0-3196
piggieback 0.2

I'm getting the following exception in the console:

Thu Apr 23 14:26:59 PDT 2015 [worker-1] ERROR - handle websocket frame org.httpkit.server.Frame$TextFrame@596b2708
java.lang.IllegalArgumentException: Compiler environment must be a map or atom containing a map, not
        at weasel.repl.websocket$eval6273$fn__6274.invoke(websocket.clj:60)
        at clojure.lang.MultiFn.invoke(MultiFn.java:231)
        at weasel.repl.websocket$websocket_setup_env$fn__6280.invoke(websocket.clj:67)
        at org.httpkit.server.AsyncChannel.messageReceived(AsyncChannel.java:166)
        at org.httpkit.server.WSHandler.run(RingHandler.java:140)
        at org.httpkit.server.LinkingRunnable.run(RingHandler.java:120)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
        at java.lang.Thread.run(Thread.java:745)

Weasel is invoked using

(defn browser-repl []
     (piggieback/cljs-repl (weasel/repl-env :ip "0.0.0.0" :port 9001)))
cemerick commented 9 years ago

@erikcw You need to use weasel master with the latest ClojureScript and piggieback.

tomjakubowski commented 9 years ago

I apologize for not getting at least a SNAPSHOT out, I've been extremely busy the last month or so (new job!). But I will try to get something out very soon.

— Sent from Mailbox

On Thu, Apr 23, 2015 at 7:09 PM, Chas Emerick notifications@github.com wrote:

@erikcw You need to use weasel master with the latest ClojureScript and piggieback.

Reply to this email directly or view it on GitHub: https://github.com/tomjakubowski/weasel/issues/51#issuecomment-95774690

gberenfield commented 9 years ago

bump saw @erikcw's same error. Works fine with weasel 0.7.0-SNAPSHOT and piggieback 0.2.1.

tomjakubowski commented 9 years ago

I've finally deployed a 0.7.0-SNAPSHOT to clojars. (does a month count as "very soon"?). I apologize for the long delay.

tomjakubowski commented 9 years ago

Closing because this is in a SNAPSHOT, however it does still need to go into a proper release.