Fork me on GitHub
#cljs-dev
<
2016-10-01
>
darwin00:10:48

@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

maria07:10:17

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

jetzajac08:10:04

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

anmonteiro11:10:18

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

anmonteiro11:10:42

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

anmonteiro11:10:21

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

anmonteiro12:10:35

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

mfikes13:10:22

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__)]
(The second form doesn’t appear in non-bootstrap.) And the apply of or derails owing to it being a macro.

anmonteiro13:10:03

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

anmonteiro13:10:27

I noticed that a few weeks ago

anmonteiro13:10:31

(if it’s the same thing)

mfikes13:10:47

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

anmonteiro13:10:32

but I didn’t have time to investigate it further

anmonteiro13:10:58

(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)

anmonteiro13:10:36

hope the above helps

mfikes13:10:53

Cool! I'll dig :)

anmonteiro13:10:14

let me know what you find out

anmonteiro13:10:49

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

anmonteiro13:10:11

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

mfikes13:10:11

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.

mfikes13:10:30

Maybe even a blog post to start off with

anmonteiro13:10:30

@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))

anmonteiro13:10:07

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

mfikes13:10:10

Can a require occur later in a file?

mfikes13:10:38

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

anmonteiro13:10:41

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

anmonteiro13:10:51

it definitely needs docs

anmonteiro13:10:59

you can’t expect it to work as in Clojure

mfikes13:10:01

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

mfikes13:10:59

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

mfikes13:10:03

@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.

anmonteiro13:10:53

thanks, I appreciate it

anmonteiro14:10:50

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

mfikes14:10:40

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.

anmonteiro14:10:28

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

anmonteiro14:10:47

that would make requiring macros work with the new require forms

anmonteiro14:10:04

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

mfikes14:10:33

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.

anmonteiro14:10:01

@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)

anmonteiro14:10:47

actually it might not need moving to the analyzer.

anmonteiro14:10:55

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

anmonteiro14:10:59

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

anmonteiro14:10:12

is it included in the bundle somehow?

mfikes14:10:27

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

anmonteiro14:10:57

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

anmonteiro14:10:07

I think it’s definitely an overlook from my part

anmonteiro14:10:19

how do you include it in the bundle?

anmonteiro14:10:25

don’t you have to require it somewhere?

anmonteiro14:10:34

or just force extract it from the cljs jar?

mfikes14:10:19

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

anmonteiro15:10:48

@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]}

anmonteiro15:10:18

bootstrap should be able to load clojure.template no?

anmonteiro15:10:25

I think it does in the CLJS codebase

mfikes15:10:08

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

mfikes15:10:29

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

anmonteiro15:10:41

oh actually I think it loads

anmonteiro15:10:50

the problem is evaling

anmonteiro15:10:08

anyway, it’s beside the point

anmonteiro15:10:20

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

anmonteiro15:10:25

just need to submit the patch

mfikes15:10:12

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

mfikes15:10:55

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])))

mfikes15:10:03

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]))}

anmonteiro16:10:13

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

anmonteiro16:10:21

its docstring:

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

mfikes16:10:22

Well, it could indeed be patched up

anmonteiro16:10:29

“not a macro"

anmonteiro16:10:47

probably needs to check :macro in the AST

anmonteiro16:10:34

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

mfikes16:10:25

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

anmonteiro16:10:55

Yeah, I understand

mfikes16:10:01

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

mfikes16:10:34

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

anmonteiro16:10:15

yep, half of the problem is solved 🙂

anmonteiro16:10:08

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

anmonteiro16:10:24

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

anmonteiro16:10:28

does this ring a bell?

mfikes16:10:50

I would expect the existing machinery to do that

anmonteiro16:10:44

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

anmonteiro16:10:52

before requiring it, which is really weird

anmonteiro16:10:00

but it worked, no idea how

mfikes16:10:18

Yeah, that corroborates the thinking

mfikes16:10:54

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

anmonteiro16:10:09

it seems systemic to my code though

anmonteiro16:10:15

getting the same error for cljs.test$macros

mfikes16:10:25

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

anmonteiro16:10:00

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

anmonteiro16:10:11

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

bhauman17:10:25

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

bhauman17:10:19

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

dnolen17:10:53

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

dnolen17:10:06

@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

dnolen17:10:25

feel free to open a ticket for that

dnolen17:10:18

@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

anmonteiro17:10:28

@dnolen those (old) docs work at the REPL

anmonteiro17:10:33

should we move them to the new macros instead?

dnolen17:10:28

@bhauman ok, file an issue

dnolen17:10:51

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

anmonteiro17:10:18

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

anmonteiro17:10:34

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

anmonteiro17:10:42

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

anmonteiro17:10:05

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

dnolen17:10:34

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

richiardiandrea18:10:30

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

richiardiandrea18:10:35

For self-host...

anmonteiro19:10:09

@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

anmonteiro19:10:28

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

anmonteiro19:10:48

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

dnolen19:10:26

yeah I should add those

dnolen19:10:58

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

anmonteiro19:10:15

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