arichiardi / replumb

ClojureScript plumbing for your self-hosted REPLs.
http://clojurescript.io
Eclipse Public License 1.0
210 stars 16 forks source link

replumb/read-eval-call doesn't work with custom macros #185

Closed viebel closed 8 years ago

viebel commented 8 years ago

When I define a macro inside a namespace and use it in the same namespace, read-eval-call returns an error on first execution and it returns the correct result on subsequent executions.

Two remarks:

(defn no-op [file-url src-cb]
  (src-cb ""))

(def repl-opts-noop (merge (replumb/options :browser
                                             []
                                             no-op)
                            {:warning-as-error false
                             :context :statement
                             :verbose false}))

(replumb/read-eval-call repl-opts-noop identity "(ns my.hello$macros) (defmacro hello [x] `(inc ~x)) (my.hello/hello 13)")
; => {:success? false, :form (ns my.hello2$macros), :warning nil, :error #error {:message "ERROR", :data {:tag :cljs/analysis-error}, :cause #object[TypeError TypeError: Cannot read property 'hello' of undefined]}}
(replumb/read-eval-call repl-opts-noop identity "(ns my.hello$macros) (defmacro hello [x] `(inc ~x)) (my.hello/hello 13)")
; => {:success? true, :form (ns my.hello2$macros), :warning nil, :value "14"}
arichiardi commented 8 years ago

Uhm, one thing I am worried about is the io (again) here. By using no-op you are maybe interfering with some underlying mechanism that I will confirm you after some investigation ok?

Thanks for reporting!

viebel commented 8 years ago

same bug with fetch-file! as provided here io.cljs

arichiardi commented 8 years ago

Yes I have to check this out

arichiardi commented 8 years ago

And then again we have this in our test (I did not remember, we should probably refer to check tests before opening issues):

https://github.com/Lambda-X/replumb/blob/master/test/cljs/replumb/macro_test.cljs#L9

With options:

https://github.com/Lambda-X/replumb/blob/master/test/browser/replumb/test_env.cljs#L7

viebel commented 8 years ago

I'm using the same options and it doesn't work. Could you please double check that the tests pass in the browser?

arichiardi commented 8 years ago

Yes I will, btw all the builds are here, but we execute NodeJS only tests for io related stuff. We should use Korma for browser testing, it was planned but then all the contributors got caught up with other things. PRs as usual would be more than welcome in this case. I can guide you if you want to contribute, just let me know. Thanks!

arichiardi commented 8 years ago

As you can see we have:

;; ======================================================================
;; Testing with Phantom:
...
Testing replumb.macro-test
(foo.core/hello (+ 2 3))
(hello (+ 2 3))

So they pass with PhantomJS.

viebel commented 8 years ago

in your tests, you use read-eval-call-test that receives an array of expressions to execute; while in my case, I am passing all the expressions together in a single statement

The issue repos also with eval-str

(cljs/eval-str (cljs/empty-state) "(ns my.hello231$macros) (defmacro hello [x] `(inc ~x)) (my.hello231/hello 13)" "" {:eval cljs/js-eval} identity)
; => {:error #error {:message "ERROR", :data {:tag :cljs/analysis-error}, :cause #object[TypeError TypeError: Cannot read property 'hello' of undefined]}}
arichiardi commented 8 years ago

It might be a bug with :context :statement then...

arichiardi commented 8 years ago

@viebel do you still have this problem?

viebel commented 8 years ago

@arichiardi Yes I still have the problem.

But I think this is by design: I heard David Nolen and Mike Fikes telling a couple of times:

Macros should be defined in a previous compilation stage than their execution

It explains why it returns an error on first execution and it returns the correct result on subsequent executions.