ClojureScript

Aside: if this is accepted, a fair bit of documentation on the github wiki (and maybe dev.clojure.org wiki?) will need to be updated (something I'm happy to do FWIW).

I just discovered that cljs.repl.reflect depends directly upon cljs.repl.server. I'll have to unroll that in order to be able to remove the latter...

Thanks for working on this Chas. This has been in need of improvement for a long time. Can't wait to take this for a spin.

I've never used cljs.repl.reflect, so I had to take a close look at what it supports, thus the delay.

First, background: cljs.repl.reflect provides backing support for two operations:

• getting the documentation for a cljs "var" (i.e. any top-level function or other definition), and
• obtaining the macroexpansion of a Clojure macro as provided by the ClojureScript analyzer

These operations are exposed within the ClojureScript REPL via clojure.reflect/doc and clojure.reflect/macroexpand, respectively.

I see a couple of issues here. First, aside from the problems in their current implementations (e.g. clojure.reflect/doc does not re-sugar arglists prior to printing them), both of these operations (doc and macroexpand) can simply be provided by macros, eliminating any dependence on a particular REPL environment/implementation. e.g., this does well for `doc`:

(defmacro doc
  [fq-sym]
  (let [meta (get-meta fq-sym)]
    `(do
       (println "-------------------------")
       (println ~(:name meta))
       (println ~(pr-str (map (partial mapv :name) (read-string (:method-params meta)))))
       (println " " ~(:doc meta)))))

A similar macro for macroexpand is trivial to produce.

Second, doc and macroexpand just don't belong in clojure.reflect. AFAICT, clojure.* namespaces in ClojureScript should be maximally isomorphic to their Clojure namesakes. This means that doc should be in clojure.repl; placing macroexpand is a little trickier, but perhaps a default-referred macro in cljs.core would make sense? (clojure.core/macroexpand is obviously already taken.)

So, my tl;dr plan (which I'll make concrete in a patch after circulating this comment on various mailing lists to get feedback) is:

1. reimplement doc and macroexpand as macros in more appropriate namespaces
2. rm the current cljs.repl.reflect and clojure.reflect namespaces

This should clear the way for the improved browser-REPL to land.

Disregard the above plan; later discussion here clarified the objectives of the browser REPL reflect bits, and contains a basic outline of the plan of attack for achieving them alongside the browser-REPL reimplementation.

New patch attached that retains reflection capabilities working with the new browser-repl backend. As discussed in the previously-linked ML discussion, the user-facing reflection namespace is now cljs.repl.browser.reflect. Otherwise, all usage instructions discussed previously apply as before for phantomjs, head-ful browsers, and other external processes via exec-env.

Two things:

• I am seeing a bunch of gclosure compilation warnings on master. They're ugly, but not seeming to affect functionality. The CLJS changes I've made are minimal, so I'll just chalk them up to transient fickleness of master for now...
• Since reflection requests are entirely asynchronous (i.e. responses are processed via a callback on an xhr-connection), there's no way to either (a) print them nicely in a REPL context (the prompt is often going to be interspersed), and (b) those results will never end up in e.g. *1. This is how the prior reflection utilities were, and I assume that that is intentional/desirable/low priority at the moment; in any case, I'd consider improvement of the reflection facilities (outside of the minor cleanup I've done) to be out of scope for this particular effort.

I've taken my first pass over the patch, it looks nice!

A couple of concerns, the instructions for the REPL sample now in the repo no longer work. For people who don't want or need to setup another server this seems unfortunate, but it also complicates basic browser REPL testing for compiler devs.

So I think we need two things before moving forward with this:

• Updated instructions for browser REPL sample
• Preserve the old behavior (serve the directory) at port 9000 or some user specified port. The new behavior should have to be explicitly set.

Yeah, definitely happy to refresh all the related docs, etc.

But, part of the point of the work was to eliminate the server/port contention/management that the prior impl implied. That the new repl environment carries around a URL string (@ :entry-url) that can be dropped into a (connect ...) call in your app makes it so you never have to think about ports, session identifiers, etc.

Stepping back:

• Basic browser-repl testing should be much easier, especially if you use exec-env. Its default is to use phantomjs, but using e.g. Chrome is trivial, as described in at the top of the ticket.
• That the browser-repl can serve non-REPL-related static content is clever, but I can't imagine that anyone is using it as their only or primary server...or if they are, they should probably stop (in neither browser-repl impl is the HTTP server up to snuff for anything other than servicing browser-REPLs). Implicitly recommending this in a sample app seems not-good, whatever the details of the browser-repl's implementation.

I can update the sample and documentation to match the the prior experience, but surely the sample's current particulars shouldn't wag the dog of the browser-REPL implementation.

Updating the sample & documentation is definitely a must. We should touch base with the lein cljsbuild folks to make sure this isn't going to cause massive breakage.

It's definitely going to cause breakage: the lifecycle of the supporting server is changing, which (positively) affects the semantics of calling repl-env. That breakage should make the browser-REPL more reliable and easier to use by all parties, including tool authors.

Anyway, yeah, any feedback from all corners is always welcome.