Fork me on GitHub
#cljs-dev
<
2018-03-24
>
juhoteperi08:03:40

@mfikes @dnolen This patch will make Cljs work with current Closure snapshot and current release, and no need to for reflection or other ugly tricks: https://dev.clojure.org/jira/browse/CLJS-2699

juhoteperi08:03:05

At some point, we can look into supporting ES7/8/TypeScript module processing

juhoteperi08:03:38

This closure-library svn checkout on bootstrap script seems deprecated: https://github.com/clojure/clojurescript/blob/master/script/bootstrap#L62-L77

juhoteperi08:03:29

If bootstrap is rewritten (or something) to use tools.deps it would be easy to use Closure snapshot?

dnolen10:03:07

@juhoteperi Windows is a blocker here

dnolen10:03:21

Bootstrap has to work there

dnolen10:03:14

@juhoteperi huh 2699 does that solve the scoped npm deps issue?

juhoteperi10:03:35

hmm, what's issue with scoped npm deps?

juhoteperi10:03:15

I doubt it. Is just about getting Cljs to work with both current Closure-compiler release and the snapshot.

dnolen10:03:25

I think it might - one second

dnolen10:03:14

yeah doesn’t

juhoteperi10:03:18

Hmm will calling those compiler passes through those Compiler methods set the correct moduleLoader?

dnolen10:03:28

so we’ll still need my fix

dnolen10:03:48

but I think this means I can make a simpler test

dnolen11:03:41

@mfikes I reverted one of your changes, I don’t know why you need to read .cljsc_opts.edn in compile

dnolen11:03:45

you really don’t want to do that

dnolen12:03:05

I know this was to handle analyze paths or something for REPL but it’s just not the right way to handle it

dnolen12:03:32

because you will supply options like :advanced etc. from previous build

mfikes12:03:09

I’m trying t recall that change… was it mine?

mfikes12:03:46

Oh, it must have been a while back.

dnolen12:03:46

so probably was made early while we were still exploring how this was all going to work

dnolen12:03:51

whittled down the criticals again

dnolen12:03:03

good time to test master

mfikes12:03:27

@dnolen I'm going to set up new Canary subproject that builds and tests ClojureScript against the latest Closure compiler and also the latest Closure library. If you think https://dev.clojure.org/jira/browse/CLJS-2697 https://dev.clojure.org/jira/browse/CLJS-2698 are safe to have on master, I'll use the switches they provide. (If you are not comfortable with that yet, I can simply make Travis in that subproject imitate the behavior in those patches.)

mfikes12:03:35

Cool! I see with Juho's patch things pass. I'll set things up so we know if anything regresses going forward.

mfikes13:03:43

I'm going to look into https://dev.clojure.org/jira/browse/CLJS-2700. My hope is that it is just a bad test and that, after assessing I can add a filter for those two REPL environments.

mfikes13:03:33

Yeah, it looks like a bad test. Working to remove it.

mfikes14:03:11

👍 No smoke from any of my non-trivial projects when using master

dnolen18:03:10

@juhoteperi can confirm that your Closure patch plus my patch at least lets scoped requires work

mfikes18:03:21

This caught my eye, a new loader namespace of some sorts in Closure Library: https://github.com/google/closure-library/tree/master/closure/goog/loader

mfikes18:03:57

(I'm digging into why bootstrapping is failing in self host tests with the latest Closure Library, and some stuff may have changed. goog.dependencies_ is undefined and might be related to this new loader stuff, but not sure yet.)

mfikes18:03:47

Perhaps a trailing underscore in a goog variable name means "private"?

mfikes18:03:32

Cool. We'll have some figuring out to do at some point.

mfikes18:03:17

^ Captured as a future enhancement / thing https://dev.clojure.org/jira/browse/CLJS-2702

mfikes19:03:46

Canary is now set up to run ClojureScript through its suite using the latest Closure stuff.

👏 4
mfikes19:03:17

A couple things are disabled, namely

lein with-profile +closure-snapshot test
(with the latest Closure Compiler) and
script/test-self-partity
(with the latest Closure Library). I'm making sure we have JIRAs so we ultimately address the needed code changes to accommodate Closure changes.

Garrett Hopper19:03:22

@dnolen It seems that https://github.com/clojure/clojurescript/commit/84004ff7d90d48a730b2e1e8ebc2c421830462f7 broke piggieback support for node cljs repls, due to different nREPL threads. (https://github.com/clojure/tools.nrepl/blob/fcac1e13d6f80eb0db28773247a769c1dc9659b1/src/main/clojure/clojure/tools/nrepl/middleware/interruptible_eval.clj#L131) It works just fine until (cljs.repl.node/thread-name) returns "nREPL-worker-1" instead of "nREPL-worker-0". I'm not familiar with nREPL's threads, but I assume there's no known reason this should break node repls. It seems like it might be an issue with stdout getting rebound, because {"type":"result","repl":"nREPL-worker-1","status":"success","value":"true"} ends up getting printed to the console. (It wasn't before this commit.) Let me know if there's any more information I can provide.

Garrett Hopper19:03:50

@cemerick Would it be possible to constrain piggieback to a single nREPL thread?

dnolen20:03:54

I didn’t think nREPL shared environments across threads \cc @bhauman

mfikes21:03:04

@juhoteperi You might be interested in this one. It might be trivial or innocuous. I'm not yet familiar with that part of the codebase: https://dev.clojure.org/jira/browse/CLJS-2703

john22:03:24

@ghopper could you set up a repro?

john22:03:45

I don't normally use node

mfikes22:03:27

Did another pass through the doc list https://gist.github.com/mfikes/bdbe214f03abac88ae384adb1ac26490 and I think everything is pretty much up to date now.

john22:03:56

@mfikes I've got your zippy.clj test running against the websocket socket repl implementation and it's going strong at 1.6 million incs

john22:03:24

That's on linux/chrome

john22:03:17

are there any tests out there for the browser repl stuff?

mfikes22:03:47

I don't recall any headless tests, probably because you need a browser. The only thing I'm aware of is if you run script/test-cli browser it will fire up your browser during the tests.

john22:03:51

It's a hard thing to test I guess. This zippy is definitely a good one

john22:03:15

I could probably build some out from that

mfikes22:03:05

Yeah, even if we have some "manual" tests in the codebase, that's better than nothing.

mfikes22:03:59

@john One easy stress test you can try in your browser REPL code is (range 1000000). I just tried that with Ambly and it completed in a few minutes (sending the range back across TCP).

dnolen22:03:24

@ghopper this looks like just interruptible eval - I don’t see why you can’t just discard that middleware - it doesn’t really work for ClojureScript anyway

mfikes22:03:32

Cool. Looks like https://dev.clojure.org/jira/browse/CLJS-2632 is no longer reproducible 🙂

dnolen22:03:33

you can’t stop eval in general in target JS envs

dnolen22:03:18

so this middleware doesn’t look like it’s really doing anything meaningful

dnolen22:03:59

sigh, nREPL is really a whole mess of things that don’t make sense for ClojureScript

dnolen22:03:38

@ghopper you should probably report this issue and have piggieback remove that middleware

dnolen22:03:07

I’m not really that interested in making JS envs reusable by more than one thread

mfikes23:03:10

Canary looks good with master https://github.com/cljs-oss/canary

👌 4
mfikes23:03:22

Are there currently any "blocker" tickets? (It seems like stuff is mostly under control.)

dnolen23:03:09

no I lowered the priority on two things - they just don’t seem critical and we’ve delayed long enough

dnolen23:03:40

the nREPL thing is actually likely to cause a lot of trouble - but I don’t really see a good answer to that - other than piggieback needs to be updated

mfikes23:03:13

Is that isolated to Node?

dnolen23:03:16

interruptible eval just isn’t a thing

dnolen23:03:32

@mfikes it will affect browser REPL too

dnolen23:03:14

but interruptible eval is just a broken concept wrt. ClojureScript

dnolen23:03:17

it’s not a thing

mfikes23:03:35

Yeah, maybe it is a trivial fix for Piggieback

darwin23:03:31

during rewrite I would vote for making it an optional feature, not implemented for everything, just for case in future there would be possibility, we could optionally implement it

mfikes23:03:31

Is the browser REPL used by large numbers of developers? Just curious. Something like think won't affect Figwheel users right?

john23:03:37

I thought I needed it for the ws thing but it works without it

darwin23:03:44

for example dirac could possibly implement it: https://github.com/binaryage/dirac/issues/41

dnolen23:03:25

we do not care about doing stuff for Chrome

dnolen23:03:34

not even worth talking about

dnolen23:03:58

interruptible is not a thing

dnolen23:03:13

and it’s never going to be

dnolen23:03:19

because JavaScript is single-threaded

darwin23:03:46

maybe webworkes?

mfikes23:03:48

I think for any chance, you need support by all the JS engines. I think Lumo is the only one to have pulled it off in the case of V8.

john23:03:01

The different evals would not share a common environment

john23:03:08

in different workers

dnolen23:03:49

anyways, my opinion is including that middleware was an oversight

dnolen23:03:51

and now it doesn’t work

dnolen23:03:36

hrm based on Piggieback docs it doesn’t look like this going to be simple

dnolen23:03:55

because Piggieback needs to - for no reason I can fathom - be built on top of it

dnolen23:03:20

once again, nREPL seems like a really long … ugh

mfikes23:03:45

Hah. It is the gift that will keep on giving forever 🙂

mfikes23:03:19

maybe pREPL someday

dnolen23:03:48

well I’m not going to fix this now - but I will spend some cycles on how to work around this tomorrow

dnolen23:03:09

actually, we could just hack around this …

dnolen23:03:16

and check for nrepl thread name for now

dnolen23:03:31

@ghopper I added a hack for nREPL to master - please try it when you get a chance

dnolen23:03:42

there may be I/O redirect issues - so it might fix the immediate problem but maybe printing won’t work correctly you should check for that

mfikes23:03:48

I'm trying it too. I saw the interruptible eval thing. (apply + (range)) lets you Ctrl-C, but then you get a cljs.user=> prompt that won't eval anything after that. Hrm.

dnolen23:03:09

yes because interruptible eval isn’t a thing 🙂

dnolen23:03:16

in Clojure you can kill the thread that tried to eval

mfikes23:03:18

Right. Why is it even there

dnolen23:03:20

but this is impossible

dnolen23:03:24

for JavaScript

dnolen23:03:37

because someone was lazy?

mfikes23:03:45

Well, for me, the purpose is that it proves that I'm actually using Piggieback.

dnolen23:03:00

Piggieback is pure dead weight

dnolen23:03:34

the only reason to work around is because 50% of users use Emacs and nREPL

dnolen23:03:42

yet somehow that 50% doesn’t work on Piggieback

dnolen23:03:46

it’s mind boggling

dnolen23:03:03

whenever people say the “community” should have more control I think of Piggieback

mfikes23:03:58

OK, cool. I think I reprod it without your hack

dnolen23:03:10

correction, fifty percent

mfikes23:03:11

I just let it sit there for a while.

mfikes23:03:44

the nREPL-worker-2 is I'm assuming core to the failure

dnolen23:03:08

yes the hack maps nREPL threads to one name

mfikes23:03:31

OK. I'll check your hack to see if it fixes things. First I want to see how easy it is to repro without it.

mfikes23:03:16

I'm assuming you have to wait 30 seconds

mfikes23:03:00

OK, it seems trivial to repro. Start it up, eval a form, wait 30 seconds. Deterministic failure.

dnolen23:03:28

so hopefully remapping to one name fixes that

dnolen23:03:42

if the threads inherit *out* then I/O redirection should just work

mfikes23:03:46

Yep, setting up master now

mfikes23:03:09

Once done, I'll write up precisely how to repro in the ticket.

dnolen23:03:34

ok nice thanks, I literally haven’t used nREPL in 3 years

mfikes23:03:44

I forgot how to as well.

mfikes23:03:08

I think in Cursive I keep picking that "plain REPL" option.

dnolen23:03:34

losing macroexpansion is a paltry price to pay

mfikes23:03:41

We don't even have a JIRA for this do we?

dnolen23:03:48

no we don’t

mfikes23:03:50

I'll put my repro instructions in a gist

mfikes23:03:59

So far it seems that your hack is working

dnolen23:03:05

and print works?

mfikes23:03:07

Oh, spoke to soon

mfikes23:03:19

This time with nREPL-worker-1

mfikes23:03:27

Anyway, writing the repro instructions now...

dnolen23:03:40

one second

mfikes23:03:46

Her is what I got

Exception in thread "nREPL-worker-1" java.lang.NullPointerException
	at cljs.repl.node$node_eval.invokeStatic(node.clj:67)
	at cljs.repl.node$node_eval.invoke(node.clj:61)
	at cljs.repl.node.NodeEnv._evaluate(node.clj:235)
	at user.Delegatingcljs_repl_node_NodeEnv._evaluate(form-init2154258829537880589.clj:122)
	at cljs.repl$evaluate_form.invokeStatic(repl.cljc:553)
	at cljs.repl$evaluate_form.invoke(repl.cljc:484)
	at cljs.repl$repl_STAR_.invokeStatic(repl.cljc:930)
	at cljs.repl$repl_STAR_.invoke(repl.cljc:839)
	at cemerick.piggieback$run_cljs_repl.invokeStatic(piggieback.clj:169)
	at cemerick.piggieback$run_cljs_repl.invoke(piggieback.clj:155)
	at clojure.lang.AFn.applyToHelper(AFn.java:171)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.core$apply.invokeStatic(core.clj:650)
	at clojure.core$apply.invoke(core.clj:641)
	at cemerick.piggieback$evaluate.invokeStatic(piggieback.clj:259)
	at cemerick.piggieback$evaluate.invoke(piggieback.clj:255)

dnolen23:03:25

nrepl -> nREPL

mfikes23:03:54

Gonna try the new master now, but here are the repro instructions using just lein in your terminal

dnolen23:03:02

is it me or does Leiningen not work with Java 1.9?

mfikes23:03:19

Let me check that. I'm set to 1.8 right now

mfikes23:03:45

There was some classpath crap they fixed I thought

dnolen23:03:07

well FWIW my hack seems to work

dnolen23:03:18

printing included

mfikes23:03:22

Yes, it is working for me.

dnolen23:03:45

No-one can say I never did anything for nREPL 🙂

mfikes23:03:48

I wonder how you can tell if you are on a different underlying thread when printing.