Fork me on GitHub
#cljs-dev
<
2016-09-30
>
tomjkidd00:09:21

In bash uname could positively identify MINGW and Darwin at least

tomjkidd00:09:37

My bash isn't very good, so there may be a cleaner way to write it...

tomjkidd00:09:45

line 5 should be '/'

tomjkidd00:09:03

(figured that out testing on OSX)

tomjkidd00:09:44

Changed it in the snippet...

anmonteiro11:09:25

@dnolen just confirming that the refer-clojure syntax should be: (refer-clojure :exclude '[map mapv]) instead of (refer-clojure :exclude [map mapv]), right?

anmonteiro11:09:29

note the

anmonteiro11:09:03

also we can’t have cljs.js/require anymore if require is a special form 🙂

dnolen11:09:15

@anmonteiro yes all those things need to require quoting

anmonteiro11:09:46

awesome, quoting definitely works. problem would be otherwise

dnolen11:09:59

quoting is necessary

dnolen11:09:09

@anmonteiro hrm I wonder if then adding all these special forms is a bad idea

dnolen11:09:22

instead one special form and + macros

dnolen11:09:31

something like (ns* …) or something like that

anmonteiro11:09:47

@dnolen I kinda like that idea better

dnolen12:09:08

we can pick a different name later - but let’s just do one special form + macro sugar

tomjkidd12:09:39

@dnolen @anmonteiro Would it be valuable to create a patch for supporting mingw?

dnolen12:09:31

@tomjkidd as long as it doesn’t create problems for anyone else - sure

tomjkidd12:09:06

@dnolen I am new to Jira, but in order to follow the patch directions, it looks like it needs to be there. Any suggestions?

dnolen12:09:51

@tomjkidd what is your question?

dnolen12:09:01

(I’m not following what you are asking)

tomjkidd12:09:25

I just created an account for the Jira page, I think I need to create an issue, but I don't know how to.

dnolen12:09:01

Login and click the create issue button (it’s on the top right side of the screen)

dnolen12:09:54

create an Enhancement ticket, set the priority to Minor, filling out the rest of the relevant information

dnolen12:09:09

@tomjkidd did you submit your CA?

tomjkidd12:09:04

Yeah, I did that a few days ago

anmonteiro12:09:19

@dnolen just noticed we’re not running all the tests when executing ./script/test

anmonteiro12:09:28

opened a ticket for that, working on a fix now

dnolen12:09:46

@anmonteiro yeah I noticed that too, not sure what the deal is - all of them under bootstrapped strangely

anmonteiro12:09:49

I’m thinking about removing the duplicate test_runner.cljs

anmonteiro12:09:55

it’s just a lot of trouble

dnolen12:09:58

there’s a dupe?

anmonteiro12:09:03

that’s the problem

anmonteiro12:09:19

got all of them running now, but the predicates_test are failing

dnolen12:09:42

looking ...

anmonteiro12:09:02

FAIL in (test-preds) (b0@builds/out-adv/core-advanced-test.js:1114:77)
(#object[ig "function ig(b){return null!=b?b.o&8388608||}"] #inst "2016-09-30T12:56:25.080-00:00")
expected: (= ((nth preds i) v) (nth row i))
  actual: (not (= true false))

FAIL in (test-int-preds) (b0@builds/out-adv/core-advanced-test.js:1114:77)
(#object[ng "function ng(b){return mg(b)||b instanceof pa||b instanceof Ha}"] 0)
expected: (= ((nth preds i) v) (nth row i))
  actual: (not (= true false))

FAIL in (test-int-preds) (b0@builds/out-adv/core-advanced-test.js:1114:77)
(#object[rg "function rg(b){if(mg(b))return!(0>b)||0===b;if(b instanceof pa){var c=Db(b.ta());return c?c:b.ab()}return b instanceof Ha?(c=Db(b.ta()))?c:b.ab():!1}"] 0)
expected: (= ((nth preds i) v) (nth row i))
  actual: (not (= true false))

FAIL in (test-int-preds) (b0@builds/out-adv/core-advanced-test.js:1114:77)
(#object[ng "function ng(b){return mg(b)||b instanceof pa||b instanceof Ha}"] 1)
expected: (= ((nth preds i) v) (nth row i))
  actual: (not (= true false))

FAIL in (test-int-preds) (b0@builds/out-adv/core-advanced-test.js:1114:77)
(#object[og "function og(b){return mg(b)?0<b:b instanceof pa?Db(b.ta())&&Db(b.ab()):b instanceof Ha?Db(b.ta())&&Db(b.ab()):!1}"] 1)
expected: (= ((nth preds i) v) (nth row i))
  actual: (not (= true false))

anmonteiro12:09:05

some of those ^

anmonteiro12:09:41

probably the tests are wrong?

anmonteiro12:09:57

I’ll leave them out for now and look into that in a subsequent patch

dnolen13:09:04

@anmonteiro yeah all those numeric tests are busted

dnolen13:09:09

they assume you can detect integers

anmonteiro13:09:53

so should we just delete them?

dnolen13:09:24

actually ...

dnolen13:09:28

we should probably just fix them

dnolen13:09:33

so the semantics are documented

dnolen13:09:41

but separate patch is fine

anmonteiro13:09:59

@dnolen OK every test namespace running now with the patch in http://dev.clojure.org/jira/browse/CLJS-1798

anmonteiro13:09:27

1 or 2 also missing from the self-parity one

anmonteiro13:09:13

when you did this moving around the test-runner got duped

dnolen13:09:46

heh took a year to notice

anmonteiro13:09:01

no idea what was happening though

anmonteiro13:09:05

one overriding the other?

dnolen13:09:56

just classpath randomness I think

dnolen13:09:08

cljs.predicates-test fixed & expanded

dnolen13:09:14

also docstrings changed to reflect what these predicates actually do

tomjkidd13:09:27

Alright, I moved http://dev.clojure.org/jira/browse/CLJS-1797 into resolved and have tested that build works on both Windows 10 and OSX.

tomjkidd13:09:33

Is there anything else I need to do?

dnolen13:09:13

@tomjkidd assign it to me, it will get reviewed when I have time

tomjkidd13:09:04

@dnolen done, and thanks

anmonteiro14:09:23

testing locally to see if it passes

anmonteiro14:09:42

oh it doesn't

dnolen14:09:08

hrm interesting

anmonteiro14:09:29

@dnolen problem is (Integer.ZERO)

anmonteiro14:09:15

#error {:message "ERROR in file src/test/cljs/cljs/predicates_test.cljs", :data {:tag :cljs/analysis-error}, :cause #object[TypeError TypeError: goog.math.Integer.ZERO.call is not a function]}

anmonteiro14:09:32

trying Integer.ZERO

anmonteiro14:09:38

without the invocation

anmonteiro14:09:04

well actually it also doesn’t pass in ./script/test

anmonteiro14:09:34

so probably can’t be invoked

dnolen14:09:25

try master

anmonteiro14:09:34

FAIL in (test-preds) (b0@builds/out-adv/core-advanced-test.js:1114:77)
(#object[ig "function ig(b){return null!=b?b.o&8388608||}"] #inst "2016-09-30T14:20:08.624-00:00")
expected: (= ((nth preds i) v) (nth row i))
  actual: (not (= true false))

anmonteiro14:09:28

not very descriptive though

anmonteiro14:09:59

oh it’s not related

anmonteiro14:09:08

it’s in the pred table above

anmonteiro14:09:10

@dnolen it seems that, at least in my machine, (seqable? (js/Date.)) is failing those pred tests?

anmonteiro14:09:23

but I can’t repro it in script/noderepljs

dnolen14:09:40

that test doesn’t make any sense, no?

dnolen14:09:27

is there some extension to js/Date in the tests?

anmonteiro14:09:21

makes no sense to me as well

dnolen14:09:35

that’s probably the problem yes

dnolen14:09:42

goog.typeOf on Date returns “object” I’m sure

anmonteiro14:09:12

cljs.user=> (goog/typeOf (js/Date.))
"object"

anmonteiro14:09:14

that’s right

dnolen14:09:39

the date test line seems questionable anyhow

anmonteiro14:09:59

@dnolen then self-parity tests also pass with the predicates-test

anmonteiro14:09:10

so we can just uncomment that too

jasoncourcoux16:09:05

@dnolen Thanks for your response on 1195...think I'm finally with you!

anmonteiro16:09:39

@dnolen I think I’ve got everything working except the :toplevel validation thing

anmonteiro16:09:57

just trying to figure out where/how to hook up that part?

anmonteiro16:09:46

correct me if I’m wrong, but sticking it into cljs.analyzer/analyze wouldn’t work

anmonteiro16:09:02

because we might call analyze from a nested call, right?

dnolen16:09:58

@lauri http://dev.clojure.org/jira/browse/CLJS-1786 please respond to my comment when you get a chance

lauri16:09:40

@dnolen Sorry, there is no reason. What could be the correct approach for setting a true value for "print-namespace-maps" value in repl while the default value would stay false otherwise?

dnolen16:09:04

@lauri err on matching Clojure’s behavior

dnolen16:09:41

then we don’t have to discuss 🙂

dnolen16:09:17

@anmonteiro I don’t understand, the default env should have :top true

dnolen16:09:24

and just remove this in the recursive cases

anmonteiro16:09:40

I was afraid sticking that into empty-env would break something

dnolen16:09:11

@anmonteiro I don’t see how

anmonteiro16:09:18

probably would only break tests, now that I think about it

anmonteiro16:09:22

which I can fix accordingly

anmonteiro16:09:50

preference for :top / :toplevel ?

dnolen16:09:34

:top-level is fine

dnolen16:09:49

@anmonteiro a few questions about CLJS-1782 if you have a moment

dnolen16:09:10

the monkey patching of the require/use-macros what does this do again?

anmonteiro16:09:35

the problem is all about Bootstrap

anmonteiro16:09:45

and you might have a file that :require-macros itself

anmonteiro16:09:51

just like in Om Next now

anmonteiro16:09:07

we wanna make that work for the 3 cases: Clojure, JVM CLJS & self-hosted

anmonteiro16:09:26

but if you follow the :require-macros in the bootstrap case, you end up looping forever

dnolen16:09:34

right but remind me why this doesn’t work in bootstrapped?

dnolen16:09:46

why is there a loop?

dnolen16:09:59

because the macro file will require the macro file again

dnolen16:09:08

oh so you just remove it

anmonteiro16:09:09

because there’s no “other” macro file

dnolen16:09:35

2nd question - what is the arity stuff?

anmonteiro16:09:32

so the problem then is that we have a single file which we can’t hide under reader conditionals

anmonteiro16:09:55

so we’re analyzing the $macros part and there’s a macro call

anmonteiro16:09:09

obviously the arity is wrong because it’s not meant to be called from that compilation stage

anmonteiro16:09:15

so we just swallow that warning

dnolen16:09:34

hrm still not following

dnolen16:09:41

you mean when compiling the ns as a macros file?

dnolen16:09:11

but how are those macros coming into play?

dnolen16:09:14

you’re not using them

anmonteiro16:09:33

we might, the example in om.next is the invariant one

anmonteiro16:09:02

we use it in om.next, but it’s not meant to be used from om.next$macros

anmonteiro16:09:14

and the user knows that

anmonteiro16:09:33

but we can’t hide it under a reader conditional

dnolen16:09:46

but I don’t see why you would use invariant macro in the macro stage?

anmonteiro16:09:10

exactly, we’d never use that in the macro stage

anmonteiro16:09:16

but we only have one file

dnolen16:09:27

so then how is that ever getting analyzed

anmonteiro16:09:44

because we’re not analyzing a different macros file

anmonteiro17:09:02

we’re analyzing the same file that has all the function definitions as well

dnolen17:09:03

because we don’t have a reader conditional to get out of this

anmonteiro17:09:12

the 3rd part in the patch is the same problem

anmonteiro17:09:22

but for var resolution

dnolen17:09:40

right I get that part

dnolen17:09:04

thanks, looks good

anmonteiro17:09:08

@dnolen should I make any comments more explicit in the patch?

anmonteiro17:09:16

explicit / descriptive, IDK

dnolen17:09:17

applying and running the tests

dnolen17:09:34

it does all make sense - it’s just meta-circular 🙂

anmonteiro17:09:15

yeah I can explain it now that I’ve worked on the problem for a while

anmonteiro17:09:24

but it took me a while to wrap my head around all that

juhoteperi17:09:30

I was hoping to work on it today but beer&sauna now

juhoteperi17:09:47

Hopefully I can finish it this weekend

anmonteiro17:09:31

@dnolen if this is at the top-level:

{:a (invoke-some-fn args)}
should analyzing the invocation be top-level?

anmonteiro17:09:10

I’d say “yes” because a map doesn’t introduce scope?

dnolen17:09:15

@anmonteiro all recursive cases are not :top-level

anmonteiro17:09:48

easier to reason about, but more assoc env :top-level false I have to write 🙂

dnolen17:09:49

@juhoteperi playing around with it, tests are passing at least

anmonteiro17:09:15

FWIW there aren’t a lot of tests with JS modules

dnolen17:09:48

right I just mean no other breakage 🙂

anmonteiro17:09:09

yeah, it’s awesome, the circle-color project compiles too, as we saw the other day

dnolen17:09:31

Maria’s circle project works fine for me no optimizations

anmonteiro17:09:35

it compiles with 2 warnings with optimizations for me

anmonteiro17:09:49

but I think I specified externs

anmonteiro17:09:09

the result in the browser wasn’t what I expected though

anmonteiro17:09:23

I think I saw a quarter of a circle instead of the full circle

dnolen17:09:09

ok yeah I see the same thing, and the input field doesn’t work

dnolen18:09:47

@maria did your circle-color repo ever work with advanced optimizations? (I don’t remember)

dnolen18:09:39

even though you still need externs being able to use 3rd party libs as proper requires is pretty nice https://github.com/swannodette/circle-color/blob/master/src/circle_color/core.cljs

anmonteiro18:09:10

FWIW I got errors when trying to do advanced compilation with the deps in that repo

anmonteiro18:09:37

so I’d assume no

dnolen18:09:53

@anmonteiro right I get warnings, but no hard errors

anmonteiro18:09:08

hrm I think I had a hard error at some point

anmonteiro18:09:25

something about React being assigned twice or something

anmonteiro18:09:34

can’t remember in full

dnolen18:09:20

@anmonteiro it’s just a warning over here

dnolen18:09:46

WARNING - constant module$resources$public$js$libs$react assigned a value more than once.

anmonteiro18:09:27

ERROR: JSC_CONSTANT_REASSIGNED_VALUE_ERROR. constant module$resources$public$js$libs$react assigned a value more than once.

darwin18:09:34

dnolen: I’m back to my yesterday’s proposal, I tried to fix the issue “goog.require after repl/bootstrap“ @bendlas ran into: https://github.com/clojure/clojurescript/compare/master...darwin:async-bootstrap

darwin18:09:56

I’m perfectly fine to just solve it on my side in dirac if you don’t want something like this to be merged

dnolen18:09:59

@anmonteiro yeah not seeing that

anmonteiro18:09:15

this is with CLJS 1.7.145

darwin18:09:21

just thought it would be better to be solved for everyone calling bootstrap

dnolen18:09:29

@anmonteiro oh, heh sorry I’m trying this with master

dnolen18:09:43

@anmonteiro but ok, you’re verifying the old behavior

anmonteiro18:09:57

yeah, with @juhoteperi ’s patch I see the same as you!

dnolen18:09:05

@darwin except no one else has considered this a problem

anmonteiro18:09:09

glad I’m not going insane

darwin18:09:39

yes, it is very subtle

dnolen18:09:05

bootstrap is annoying bit of functionality to have written - but it’s not meant to be end-all-be-all

bendlas18:09:10

@dnolen: it's easy to dismiss, because when running alongside figwheel, it starts with a blank screen, but starts working on the first code update

bendlas18:09:19

I think i've even seen that in a presentation

dnolen18:09:32

and in fact we have to change it occasionally when goog.base changes

dnolen18:09:45

@bendlas it’s purely anecdotal, but I’ve never encountered this myself

dnolen18:09:22

in anycase I’m not against seeing it fixed but @darwin I don’t understand your patch at all

dnolen18:09:34

please explain the minimal thing that needs to happen and don’t do any more than that

dnolen18:09:46

dynamic vars are not desirable

dnolen18:09:59

just passing options to bootstrap seems more sensible if you want config

darwin18:09:03

the minimal thing = boostrap must be called after document finishes loading, because there may be some code coming after the bootstrap call which expects goog.require to work synchronously, like @bendlas discovered

darwin18:09:38

I’m afraid this is too low-level to avoid dynamic vars without messing with user’s environment, I’m open for better ideas

dnolen18:09:53

@darwin I don’t understand how that can possible be true

dnolen18:09:08

make bootstrap take a map of options

dnolen18:09:18

split out a bootstrap* that does the real work

dnolen18:09:46

if bootstrap gets :wait-until-load true then setTimeout to invoke bootstrap*

darwin18:09:23

if course, that can be optional, but people can call bootstrap! directly in current impl

darwin18:09:34

I wanted this change to be transparent to current users

dnolen18:09:36

@darwin why does this matter?

dnolen18:09:58

as I already said, do not include all these other concerns

dnolen18:09:01

just solve the minimal thing

dnolen18:09:22

if you don’t want to do that - then sure, solve it downstream instead

darwin18:09:23

well, from UX perspective I wanted to have there a possibility to pass a callback, so I can warn people when they potentially eval dirac REPL js without being properly bootstrapped, that is the only thing which is added as extra

darwin18:09:39

but as I said, I’m happy to solve it downstream, if you don’t want to touch this

darwin18:09:55

without the callback I won’t use it

dnolen18:09:15

yeah not interested in the approach taken in your patch

dnolen18:09:22

to many concerns that aren’t relevant for ClojureScript

darwin18:09:49

ok, fair, will isolate the code into a separate ns, so people could grab my code if they run into issues

darwin18:09:52

no problem here

bendlas18:09:44

looking at that browser-repl bootstrap, the load queue strikes me as pretty sketchy: clojure and clojurescript do recursive-descent loading: each require is loaded, before the code in each namespace is executed. The browser-repl - provided writeScriptTag looks like it would load the full namespaces before starting with its dependencies ...

bendlas19:09:51

on a second thought, scratch that, load - order is provided by goog.closure's dependency analysis.

dnolen19:09:14

expecting loading to work precisely as Clojure is unrealistic

bendlas19:09:56

ha, but this lead me to another possible fix: The problem, really is triggered by the content of the main .js file in dev mode: it require's the :main namespace synchronously, via document.write

bendlas19:09:47

this could already utilize goog.require, after goog/base.js has already been required via document.write

bendlas19:09:27

no, also not quite ...

darwin19:09:28

don’t fix your code yet, I need you to test my patch in dirac, (10mins or so) 🙂

darwin19:09:57

I don’t have a repro case here, and I’m not going to test actively for this (rare) issue - too lazy to write a test for it

darwin19:09:31

the only thing I care about ATM is that it will fix your particular issue and won’t break my existing code 🙂

bendlas19:09:55

darwin I've fixed it even before reporting

favila19:09:03

why does that code use document.write and not appendChild() script elements? I've always wondered that

bendlas19:09:37

favila: document.write for script tags, makes them execute synchronously

favila19:09:52

why is that necessary?

bendlas19:09:09

favila: it's like extracting them and calling eval on the contents

bendlas19:09:23

otherwise you get the effect of a .setTimeout

bendlas19:09:20

i.e. code executing after document.write will see its effects, code executing after .appendChild, will not

dnolen19:09:43

we’re not going to mess with any of this code 🙂

dnolen19:09:08

show me a bug and then we can talk

dnolen19:09:57

pretty sure I struggled over this for quite some time plus tested every major browser

dnolen19:09:12

note the presence of IE hacks

darwin19:09:48

with async nature of this you could have had even more fun trollface

dnolen19:09:00

oh I already did

dnolen19:09:06

that’s what the load queue is for

darwin19:09:02

I’m pretty fine with this solution and people could grab[1] if they wanted to solve it for themselves: [1] https://github.com/binaryage/dirac/blob/e5f42d3343f54990f347165e247e85ab163b300c/src/runtime/dirac/runtime/bootstrap.cljs

anmonteiro19:09:57

@dnolen just attached a patch to the “require without NS” http://dev.clojure.org/jira/browse/CLJS-1346

anmonteiro19:09:24

let me know if anything is missing or not the way we discussed

dnolen19:09:44

did you give it a try with advanced on some files?

anmonteiro19:09:13

yeah I suppose they were concatenated to the advanced build

anmonteiro19:09:30

I had some in the classpath when running script/test

anmonteiro19:09:46

and yeah, it’s kinda of a big one

anmonteiro19:09:52

lots of test lines though

dnolen19:09:02

still this is awesome

anmonteiro19:09:23

awesome will be next improvements such as extensible tagged literals 🙂

anmonteiro19:09:25

can’t wait

anmonteiro19:09:47

it’s really satisfying to work on these “big” patches

anmonteiro19:09:10

I actually get to mess around with all the compiler phases

dnolen19:09:05

@anmonteiro a question maybe you know the answer to - since you looked at 1762 - I’m assuming these modules get indexed as foreign-deps?

anmonteiro19:09:35

hrm actually I don’t know

anmonteiro19:09:56

why would they?

anmonteiro19:09:14

they are GClosure compatible, so not foreign I’d say?

dnolen19:09:50

@anmonteiro they are supplied via the :foreign-libs compiler option

dnolen19:09:01

(I’m not saying you’re not right)

dnolen19:09:56

it makes some sense because they aren’t Closure compatible, but they can be transformed to be

dnolen19:09:16

and “compatible” is a loose term here

dnolen19:09:35

since they don’t necessarily follow the coding standards and may require externs

anmonteiro19:09:04

@dnolen sorry which patch are you talking about?

anmonteiro19:09:17

I probably didn’t even understand the first question

anmonteiro20:09:14

so yeah, I was conflating everything in my head 🙂

anmonteiro20:09:03

@dnolen so to actually answer your question, I think they are indexed as foreign libs

dnolen20:09:39

I was hoping so but seeing something which makes me thinks they aren’t …

dnolen20:09:43

but I’m gonna backburner this for now

dnolen20:09:47

trying your patch

anmonteiro20:09:49

i.e. we end up associng to the ijs

dnolen20:09:56

(yeah I saw that)

anmonteiro20:09:57

so I’d say it maintains foreign status

dnolen20:09:26

@anmonteiro ok there are some weird things with 1346

dnolen20:09:38

I made an uberjar with patch applied

dnolen20:09:48

then if I make a project with just src/foo.cljs

dnolen20:09:12

where foo.cljs is just a file that requires clojure.spec a prints a simple conform

dnolen20:09:32

then I build the project with :none - but the :output-to main.js doesn’t have anything I expect

dnolen20:09:42

no dependency graph or anything like that

anmonteiro20:09:02

does foo.cljs have a namespace?

dnolen20:09:15

no because I’m testing a file with no namespace

anmonteiro20:09:41

k it’s just so I can repro locally

dnolen20:09:23

@anmonteiro also I think we should probably amend the patch so that these are proper subnamespaces

dnolen20:09:32

cljs.user.AAAAA…

anmonteiro20:09:57

yeah that makes sense

dnolen20:09:36

(but let’s not get distracted by that for now)

anmonteiro20:09:35

k so I have a foo.cljs file without a namespace

anmonteiro20:09:47

and I’m seeing this in :output-to:

goog.addDependency("base.js", ['goog'], []);
goog.addDependency("../cljs/core.js", ['cljs.core'], ['goog.string', 'goog.object', 'goog.math.Integer', 'goog.string.StringBuffer', 'goog.array', 'goog.math.Long']);
goog.addDependency("../clojure/string.js", ['clojure.string'], ['goog.string', 'cljs.core', 'goog.string.StringBuffer']);
goog.addDependency("../cljs/user$$gen_ns$$_foo0BEEC7B.js", ['cljs.user$$gen_ns$$_foo0BEEC7B'], ['cljs.core', 'clojure.string']);

anmonteiro20:09:22

@dnolen my foo.cljs requires clojure.string, and the dep is there

dnolen20:09:55

oh hrm strange

dnolen20:09:00

I also see it now

dnolen20:09:19

sorry might be Atom editor issues (I use that for quick tests with uberjar)

dnolen20:09:46

@anmonteiro ok maybe a legit bug, I can’t start a Node.js REPL without hitting an error

dnolen20:09:59

I suspect this might be because we put an ns form or something inside of a do

anmonteiro20:09:28

Namespace declarations must be at the top-level?

anmonteiro20:09:31

seeing that too

anmonteiro20:09:50

@dnolen ohhh this is probably in cljs.repl

anmonteiro20:09:24

I didn’t try a REPL after hooking up the :top-level stuff

dnolen20:09:37

many hopes have been dashed when firing up a REPL 🙂

anmonteiro20:09:13

looking into that now

dnolen20:09:33

oh it’s REPL helpers load

dnolen20:09:38

like doc etc.

anmonteiro20:09:54

it doesn’t seem to be only that

anmonteiro20:09:07

when you type (ns foo.core) you’ll also get an error

dnolen20:09:05

@anmonteiro oh maybe because the REPL always wrap the eval form

anmonteiro20:09:20

well that’s exactly the problem 🙂

anmonteiro20:09:31

I just noticed that too

anmonteiro20:09:35

it always calls identity

anmonteiro20:09:45

if no wrapper is given

anmonteiro20:09:27

@dnolen we can have a dynamic var with-no-top-level-allowed or something

anmonteiro20:09:53

that’ll probably go away whenever we fix the REPL requires anyway

anmonteiro20:09:10

kinda ugly though

dnolen20:09:14

or maybe the whole top thing is a just a bad idea

dnolen20:09:43

and we should instead (disallowing-ns* …) macro helper

dnolen20:09:55

like (disallowing-recur …)

anmonteiro20:09:56

just like disallowing-recur

dnolen20:09:15

yeah let’s try that

anmonteiro20:09:53

@dnolen will that solve the REPL problem though?

dnolen20:09:04

why wouldn’t it?

dnolen20:09:32

I actually think that we should relax our restrictions a bit here

dnolen20:09:51

disallowing-ns* should maybe just be confined to fn bodies for now

anmonteiro20:09:55

if we disallow ns* in a function body it won’t work for REPLs too

dnolen20:09:26

oh let me look at the wrap-js thing

anmonteiro20:09:49

I mean function call, not body

anmonteiro20:09:07

because we always wrap the ns call hrm

dnolen20:09:53

aha no this will work for repls

dnolen20:09:04

I’m suggesting disallowing inside fn bodies

dnolen20:09:09

REPL wrap thing doesn’t do that

anmonteiro20:09:30

yeah I confused function body with the args to the function

anmonteiro20:09:34

which is what the REPL does

dnolen20:09:56

I think this is good enough to handle most mistakes

dnolen20:09:09

the other cases are handled by parse-ns behavior

anmonteiro20:09:28

@dnolen any reason there is disallowing-recur in cljs.analyzer.macros and cljs.analyzer?

dnolen20:09:03

we needed it before anyway

anmonteiro20:09:09

it’s under :clj though

dnolen20:09:10

since we didn’t use self-require

dnolen20:09:30

@anmonteiro there was definitely a reason

anmonteiro20:09:38

well I’m not contesting that 🙂

dnolen20:09:39

(but I don’t know if it’s valid anymore or not)

anmonteiro20:09:47

just curious

dnolen20:09:13

@anmonteiro right that’s the reason

dnolen20:09:20

we didn’t self-require cljs.analyzer for macros

dnolen20:09:41

since I don’t think it occured to me that might work or cause more issues while I was trying to bootstrap

anmonteiro20:09:55

oh I see it now

anmonteiro20:09:05

I was thinking the other way around

anmonteiro20:09:21

(i.e. that the .macros ns was for JVM)

dnolen20:09:04

oh yeah, no for cljs

anmonteiro20:09:35

@dnolen if we disallow ns* inside ’def I think it covers the fn body case too?

anmonteiro20:09:43

or rather, there and in fn*

dnolen20:09:33

@anmonteiro yes that should be ok

anmonteiro20:09:06

(also tested entering the REPL this time)

dnolen20:09:58

giving it a spin

dnolen21:09:35

k all tests passing for me here, I can make an advanced build, dev build

dnolen21:09:57

there’s some finessing we can do but I say we ship and enhance with subtickets

anmonteiro21:09:42

@dnolen exciting! happy to have a look at the subtickets, just link them here

bendlas22:09:03

@darwin: here is a test project for the issue and to verify the fix: https://github.com/bendlas/cljs.test-bootstrap-loading

bendlas22:09:24

I haven't built dirac yet

bendlas22:09:47

I'll adapt the test project to demonstrate for plain browser repl first

bendlas22:09:37

dnolen: I've put a demonstration of the issue with plain cljs on a second branch: demonstrate-plain-cljs