viebel / klipse

Klipse is a JavaScript plugin for embedding interactive code snippets in tech blogs.
http://blog.klipse.tech/
GNU General Public License v3.0
3.11k stars 145 forks source link

Bind `cljs.js` dynamic vars before `eval` #275

Open moxaj opened 7 years ago

moxaj commented 7 years ago

I have a proof of concept project which I'm testing with various self-hosted envs, and it does not work with klipse. I believe some minor modifications could be made to the way expressions are currently compiled and evaluated, which might benefit klipse as well (so it's not entirely for my own selfish reasons :))

The cause of this issue is that klipse does not directly set or bind cljs.js/*load-fn* and cljs.js/*eval-fn*, but simply passes them to cljs.js/eval-str. Therefore, client code which uses cljs.js is restricted (require expressions cannot be eval-d for instance).

The solution would be to simply bind or set these vars before making the eval-str call.

Also, the current implementation uses async channels to hold the result. I believe these could safely be replaced with refs (lumo and planck use volatiles). Besides being simpler, it would also enable the use of binding (which is arguably better than set!).

If you have no objections, I can put together a PR for you to review.

viebel commented 7 years ago

Sounds good @moxaj ! I'd be happy to receive a PR.

What do you mean by "require expressions cannot be eval-d for instance"?

In Klipse, require works fine. See for instance http://app.klipse.tech/?cljs_in=(require%20%27%5Bclojure.set%20%3Aas%20set%5D)%0A(set%2Fintersection%20%23%7B1%202%7D%20%23%7B2%203%7D)

moxaj commented 7 years ago

Try to evaluate something like this:

(ns foo.bar
  (:require [cljs.js :as cljs]
            [cljs.env :as env]))

(defn eval [expr]
  (let [result (volatile! nil)]
    (cljs/eval env/*compiler*
               expr
               {:ns      (.-name *ns*)
                :context :expr}
               (fn [{:keys [value error]}]
                 (if error
                   (throw (js/Error. (str error)))
                   (vreset! result value))))
    @result))

(eval `(require '~'cljs.string)) ;; the quote-unquote-quote is a workaround for a bug in the cljs compiler

It will fail with Error: No *load-fn* set. Not saying this is a common use case, just a 'nice to have'.