@bendlas nice job! thanks for this minimal repro, I will probably incorporate it into dirac test suite when I get to working on dirac again

@dnolen No, unfortunately it didn’t work with :advanced and :simple. I also always got the “constant assigned a value more than once” error.

@dnolen can’t see how it conflicts. as far as i see tests for multimethods remain in core_test.cljs

@dnolen created and attached a patch to a ticket for generating proper subnamespaces http://dev.clojure.org/jira/browse/CLJS-1802

forgot to include that suggestion of yours in yesterday’s patch

@dnolen fix is rebased on master + some refactor http://dev.clojure.org/jira/browse/CLJS-1791?focusedCommentId=43998

@dnolen hah, experimenting with removing the REPL special functions and it “just works”. only need to add support for recognizing the :ns* AST op

so good

captured the REPL stuff in http://dev.clojure.org/jira/browse/CLJS-1803

if anyone wants to try and break the REPL requires with current master & that patch I appreciate it

There is an interesting change that appears to only occur in downstream bootstrap I’d like to track down. If you (require '[cljs.test :refer [is]]) and then (macroexpand '(is (or true))), near the beginning of the expansion an extra form is in the let

(try
   (cljs.core/let [values__39__auto__ (cljs.core/list true)
                   result__40__auto__ (cljs.core/apply or values__39__auto__)]

@mfikes I think that’s causing a planck test to fail, right?

I noticed that a few weeks ago

(if it’s the same thing)

@anmonteiro Yes. I’ll try to bisect ClojureScript commits to see where it changed.

@mfikes I think it was this one: https://github.com/clojure/clojurescript/commit/d994f883d34a946963c3410714c522e25a633944

but I didn’t have time to investigate it further

@mfikes and I think the problem downstream was caused by: https://github.com/mfikes/planck/blob/master/planck-cljs/src/planck/repl.cljs#L422

(if you’re wondering why I know these things, I was playing with Planck’s source for CLJS-1782: https://github.com/clojure/clojurescript/commit/04751f337620279b0228856e4d224ae3d41abe72)

hope the above helps

Cool! I'll dig :)

let me know what you find out

@mfikes btw the new “require outside ns” is working great in a few tests in bootstrap

as in, it obviates the need to define REPL specials in self-hosted too 🙂

Sweet. (As an aside, I think it needs a little documentation somewhere—perhaps in the new ClojureScript site somewhere.) I was going to play with it and the first thing I wanted to do was to understand its limitations, where it is applicable, etc.

Maybe even a blog post to start off with

@mfikes I think the easiest way to start playing with it is fire up a REPL and load-file something like:

(require ‘[clojure.test :refer [deftest run-tests is]])

(deftest foo (is false))

then you can (run-tests) because everything is required to the user ns

Can a require occur later in a file?

no

Cool. (That’s probably the salient bit that needs docs.)

a file can either have a ns declaration or some requires at the top

yeah

it definitely needs docs

you can’t expect it to work as in Clojure

(The rules are probably very simple, I’d guess.)

I see, it is as if requires can be used as sugar in lieu of ns. Makes sense.

@anmonteiro Absolutely amazing compiler work recently (as usual) by the way. Wish I could hang more with you—perhaps will be able to in the future.

thanks, I appreciate it

@mfikes I’m really excited about the macro loop patch for self-host

Yes, it seems like you found a way to violate my simple mental model 🙂 I’m curious to play with it and learn what magic you pulled off.

@mfikes I wonder if there’s a way for cljs.js/ns-side-effects to go into the analyzer

that would make requiring macros work with the new require forms

it currently doesn’t in self-host because we only process side-effects in the :clj branch

Interesting. All I can say for now is that the require-macros REPL special works in Planck simply because it converts it to an ns form (pretty much in the exact same way is the regular REPL) and evaluates it. I suppose the new require (special?) form has more subtleties. I need to ramp up on it.

@mfikes not really, not a lot of subtleties there. the new require is almost like the REPL require (in that it :merges to the compiler state)

actually it might not need moving to the analyzer.

it might just be an overlook from my patch. Checking now

@mfikes how you do e.g. load cljs.test in Planck?

is it included in the bundle somehow?

Yes, cljs.test is pre-compiled (perhaps that is part of the problem)

oh this is unrelated to your issue. I’m looking into ns-side-effects for the new requires

OK

I think it’s definitely an overlook from my part

how do you include it in the bundle?

don’t you have to require it somewhere?

or just force extract it from the cljs jar?

The compiled JavaScript, the analysis metadata (Transit), etc. is simply bundled so it can be loaded directly in lieu of loading the source

gotcha

@mfikes can you help me with:

Loading result:  {:error #error {:message ERROR in file target/main.out/clojure/template.clj, :data {:tag :cljs/analysis-error}, :cause #object[TypeError TypeError: Cannot set property 'apply_template' of undefined]}}
ERROR: #error {:message ERROR in file target/main.out/clojure/template.clj, :data {:tag :cljs/analysis-error}, :cause #object[TypeError TypeError: Cannot set property 'apply_template' of undefined]}

bootstrap should be able to load clojure.template no?

I think it does in the CLJS codebase

I have successfully loaded that as a macro namespace, getting apply-template and do-template defined and usable

But, to be honest, I recall that namespace being tricky

oh actually I think it loads

the problem is evaling

anyway, it’s beside the point

which is that ns-side-effects works great with the new requires for self hosted

just need to submit the patch

cljs.test now fails with (is (or …)) under bootstrap because cljs.test/function? thinks or is a function because (with the latest code, or is resolved properly in the compiler state) and :fn-var is true. I’m wondering about the meaning of :fn-var

Here is a minimal set of forms you can try in script/noderepljs to see what’s going on:

(require 'cljs.js)

(def st (cljs.js/empty-state))

(cljs.js/eval-str st
  "(ns cljs.user (:require-macros foo.core))"
  nil
  {:eval cljs.js/js-eval
   :load (fn [_ cb]
           (cb {:lang :clj :source "(ns foo.core) (defmacro add [a b] `(+ ~a ~b))"}))}
  (fn [_]
    (get-in <@U07E9BBBN> [:cljs.analyzer/namespaces 'foo.core$macros :defs 'add])))

The above yields

{:protocol-inline nil, :meta {:file foo.core, :line 1, :column 25, :end-line 1, :end-column 28, :macro true, :arglists (quote ([a b]))}, :name foo.core$macros/add, :variadic false, :file nil, :end-column 28, :method-params ([&form &env a b]), :protocol-impl nil, :arglists-meta (nil nil), :column 15, :line 1, :macro true, :end-line 1, :max-fixed-arity 4, :fn-var true, :arglists (quote ([a b]))}

@mfikes seems like the problem is in cljs.test/function?

its docstring:

"Returns true if argument is a function or a symbol that resolves to
  a function (not a macro)."

Well, it could indeed be patched up

“not a macro"

probably needs to check :macro in the AST

it could also be that :fn-var being set (perhaps incorrectly) for macros

Evidently, :fn-var is false in this case in non-bootstrap… but there are other issues at play (we are now resolving to something in :defs instead of :macros, for example). So yes, patching up cljs.test/function? could do it, but I’m really wondering more about :fn-var and what it should be in this case

Yeah, I understand

I could see an argument that “a macro is a function that is def’d"

Hey, at least we know the root cause… just need to think of the right solution 🙂

yep, half of the problem is solved 🙂

@mfikes the problem I’m having with clojure.template: Cannot set property 'apply_template' of undefined

it seems that I have to create the clojure object / namespace somehow?

does this ring a bell?

I would expect the existing machinery to do that

@mfikes I got around it by doing (set! js/clojure.template$macros #js {})

before requiring it, which is really weird

but it worked, no idea how

Yeah, that corroborates the thinking

A thought: clojure as a prefix is causing a different code path to be followed

it seems systemic to my code though

getting the same error for cljs.test$macros

I can’t recall. Isn’t it Closure that helps with that. I’d have to dig.

yeah I’ll figure it out I suppose, thanks anyway

I wonder if it has anything to do with the Node vm contexts

Found an apparent bug: when inline code is provided as vector to the composition of cljs.build.api/build and cljs.build.api/inputs methods under :target :nodejs the provided inline code is not output.

(cljs.build.api/build (cljs.build.api/inputs '[(ns hello.core) (+ 1 2)]) {}) ;; this outputs code

(cljs.build.api/build (cljs.build.api/inputs '[(ns hello.core) (+ 1 2)]) {:target :nodejs}) ;; this does not output inline code


;; When you don't use cljs.build.api/intputs everything works correctly
(cljs.build.api/build '[(ns hello.core) (+ 1 2)] {}) ;; this outputs code

(cljs.build.api/build '[(ns hello.core) (+ 1 2)] {:target :nodejs}) ;; this outputs code

not surprising since this is probably a little used feature, but I started using it and this bug showed up for nodejs users

@maria, thanks! good to know, I might look into that soon

@jetzajac thanks

@bendlas ok, now that I think about this some more I don’t see a problem with just waiting until document load to bootstrap - I don’t know why it needs to be configurable

feel free to open a ticket for that

@anmonteiro do we have docstrings for all the new macros? We can rip out the obsolete doc support for those things in REPLs as well. Might as well fold this into CLJS-1803

@dnolen those (old) docs work at the REPL

should we move them to the new macros instead?

@anmonteiro yep

@bhauman ok, file an issue

@dnolen will do

@anmonteiro oh right, yeah ok - then not really a priority

@mfikes btw my errors are solved by not messing with the Node.js global env

if I use the Node.js vm module (`require(‘vm’)`), everything works

@dnolen just to make sure, let CLJS-1803 unchanged for now and maybe port docs over sometime later?

@dnolen oh btw if you find that the browser REPL is broken the fix in CLJS-1802 addresses that

@anmonteiro yes leaving CLJS-1803 alone is fine, thanks for the info about CLJS-1802

👍

Btw when the patching of the new require is stable I can test against replumb's 1000 assertions as well!

For self-host...

@dnolen: btw messing around with the REPL code I noticed some files in the repl and source_map dirs don't have a copyright preamble

Also doesn't seem to be in any .js files

Not sure what to do there, so deferring that to you

yeah I should add those

@anmonteiro thanks added a bunch, let me know if you see one I missed

@dnolen: yep those were the ones I noticed. Now just wondering if the externs and the imul.js should have it too

@anmonteiro hrm probably

done