Fork me on GitHub
#cljs-dev
<
2018-08-10
>
dnolen18:08:47

let me know if there’s some tickets I should be looking at

mfikes18:08:56

I can’t think of any that seem overly critical

dnolen19:08:11

how is there a race, I thought browser REPL prevented simultaneous load?

mfikes19:08:33

What I observed (simply by adding a few util/debug-prns), is that this causes goog.require to be evaluated https://github.com/clojure/clojurescript/blob/9923fadc659dc1694c88371d46791da8881c026b/src/main/clojure/cljs/compiler.cljc#L1250 right before this causes the code that looks like

cljs.user.global$module$my_lib = goog.global["mylib"];
to be emitted https://github.com/clojure/clojurescript/blob/9923fadc659dc1694c88371d46791da8881c026b/src/main/clojure/cljs/compiler.cljc#L1273 And in the browser the goog.require simply writes out a script tag or somesuch (need to re-look at it), so the assignment is done before the lib is actually loaded

mfikes19:08:51

It in fact may not really be a “race”, as it may always lose, and be in the wrong order

dnolen19:08:03

right so let’s clarify how this is supposed to work

mfikes19:08:59

(The same failure occurs with Figwheel, FWIW.)

dnolen19:08:53

the require should not be triggered

dnolen19:08:21

we compute the dependencies in order, my feeling is that goog.require shouldn’t actually do anything

mfikes19:08:14

Oh, when digging into it, I added some stuff to console log directly from the foreign lib (`mylib.js` in the ticket), and also at the point where cljs.user.global$module$my_lib = goog.global["mylib"]; is executed, and that’s where I could see the assignment occuring prior to the execution of mylib.js In short goog.global["mylib"] is null when the assignment is performed.

mfikes19:08:59

My takeaway was that, even though requires / loading occur in a well defined order, we have this assignment sitting in the middle of all of it

dnolen20:08:07

but you won’t evaluate file until all it’s deps are already loaded, no?

dnolen20:08:38

unless that isn’t true - but I thought it was

mfikes20:08:43

I’m re-looking at the sequence more closely to better understand it

mfikes20:08:15

All I’m really confident in saying is that we emit code that reads from goog.global["mylib"] prior to it being set.

dnolen20:08:45

right and I’m just saying I don’t know how 🙂

dnolen20:08:51

goog.requires are fake

dnolen20:08:20

they should never do anything during file evaluation

dnolen20:08:30

because all deps are already loaded

dnolen20:08:09

this is exactly what dep.js and cljs_deps.js is for

dnolen20:08:18

we know the dep graph ahead of time

mfikes20:08:30

Here is a small example (which doesn’t explain why, but it is a step towards the problem):

$ clj -m cljs.main -co co.edn -re node -r
cljs.user=> (require 'my-lib)
my-lib code executing
emit global export
nil

mfikes20:08:52

^ That is in node, but if I do the same in the browser, the two lines logged are reversed

dnolen20:08:16

right I understand all that

dnolen20:08:39

the important point is what you’re observing doesn’t reconcile with how Google Closure goog.require should work - unless we overlooked something

dnolen20:08:42

or broke something

mfikes20:08:21

In the REPL, goog.require isn’t fake, right? It causes code to be loaded in the end. Our monkey-patched version ultimately calls js/goog.require__ which ultimately evaluates

goog.writeScripts_(path);

dnolen20:08:31

it’s still fake actually

dnolen20:08:56

see cljs.repl/load-namespaces

dnolen20:08:23

and cljs.repl/load-sources

dnolen20:08:50

first we add the dep graph

dnolen20:08:36

but that might be the bug?

dnolen20:08:43

not the dep graph part but -load

dnolen20:08:59

we load instead of just emitting goog.require for the thing we want to load

dnolen20:08:53

oops not true

dnolen20:08:53

yes this should work

dnolen20:08:03

load-sources doesn’t try to load - it just updates the dep graph

dnolen20:08:51

hrm yeah I don’t get it

dnolen20:08:06

(require ...) will compile to goog.require ...

dnolen20:08:19

which will get evaluated after the dep graph gets updated

mfikes20:08:35

Yeah, this sequence

cljs.user=> (.addDependency js/goog "../mylib.js" #js ["my_lib"] #js [] #js {"foreign-lib" true})
nil
cljs.user=> (js/goog.require "my_lib")
nil
cljs.user=> js/mylib
#js {:abc 3}

mfikes21:08:49

This is consistent with it being async (done in a fresh browser REPL):

cljs.user=> (.addDependency js/goog "../mylib.js" #js ["my_lib"] #js [] #js {"foreign-lib" true})
nil
cljs.user=> (.-mylib js/goog.global)
nil
cljs.user=> (do (js/goog.require "my_lib") (.-mylib js/goog.global))
nil
cljs.user=> (.-mylib js/goog.global)
#js {:abc 3}

dnolen21:08:44

@mfikes try with :repl-verbose

dnolen21:08:00

I think you’ll see that the dep graph comes before

mfikes21:08:06

Yeah, I’ve tried that.

dnolen21:08:11

(do (js/goog.require "my_lib") (.-mylib js/goog.global))

dnolen21:08:18

this won’t work, but we don’t really about that

mfikes21:08:21

And yep, the dep graph is done before the goog.require call

dnolen21:08:33

(require 'my-lib) should work

dnolen21:08:36

but it’s not the same

mfikes21:08:48

(do (js/goog.require "my_lib") (.-mylib js/goog.global)) is the same as the failure mode… hold on…

dnolen21:08:07

right but I mean we don’t care about (do (js/goog.require "my_lib") (.-mylib js/goog.global)) case

mfikes21:08:11

That bit that reads from goog global is the same as the global exports setup code

dnolen21:08:12

since that should never actually happen

dnolen21:08:23

that would be in the compiled code

dnolen21:08:35

and we already loaded all the deps before we look at that

mfikes21:08:53

Yeah, things work properly in the compiled case

mfikes21:08:11

It only fails in the REPL

dnolen21:08:26

right but I changed REPL to be like the compile case 🙂

dnolen21:08:35

to remove lots of bugs

mfikes21:08:51

That was the queue logic right?

dnolen21:08:16

no, this whole thing ^ is new

dnolen21:08:33

this makes it so that REPL never load files

dnolen21:08:41

we just compile and load the dep graph

dnolen21:08:49

then goog.require(...)

dnolen21:08:55

will automatically load deps in the right order

dnolen21:08:18

I mean the goog.require(...) entry point

dnolen21:08:31

(require ...) in the REPL creates an entry point require

dnolen21:08:02

it’s like the old pattern of a final script tag with one goog.require(...) in it

dnolen21:08:17

or what we generate in the :main case

mfikes21:08:21

As best I can ascertain these three JavaScript statements get executed

goog.addDependency("../mylib.js", ['my_lib'], [], {'foreign-lib': true});
goog.require('my_lib');
cljs.user.global$module$my_lib = goog.global["mylib"];
with the side effect of the 2nd statement emperically being asynchronous

mfikes21:08:42

So goog.global["mylib"] is null when read

mfikes21:08:52

I had seen language like the following in Closure library https://github.com/google/closure-library/blob/master/closure/goog/base.js#L779-L780 which made me believe that goog.require can essentially be async

mfikes21:08:59

The (do (js/goog.require "my_lib") (.-mylib js/goog.global)) example is meant to imitate the 2nd and 3rd lines (at least the salient aspect)

dnolen21:08:00

@mfikes heh ok I see the problem now

dnolen21:08:08

:repl-verbose output makes it clear

mfikes21:08:08

Ahh good 🙂

dnolen21:08:32

so if you were requiring my-lib transitively you wouldn’t have this problem

dnolen21:08:46

because that export assignment would be in some other lib

dnolen21:08:02

but when you require it directly it’s like a ns form for cljs.user

dnolen21:08:14

so yes that won’t work

mfikes21:08:17

Yes, the transitive aspect probably explains why my last comment in https://dev.clojure.org/jira/browse/CLJS-2854 works

mfikes21:08:01

Or, maybe my last comment in that ticket is something else, more like the compiled case. Hrm.

mfikes21:08:51

All the patch in CLJS-2854 tries to do is defer

cljs.user.global$module$my_lib = goog.global["mylib"];
when you are in a REPL. (And the patch feels very hackish.)

mfikes21:08:38

An alternative way to solve it would be to think of the global export initialization code

cljs.user.global$module$my_lib = goog.global["mylib"];
as a tiny library which depends on the foreign library, and then tell Closure to load that tiny library instead of the foreign library.

mfikes21:08:23

(Or somehow shove that line in at the bottom of the foreign library 🙂 )

dnolen21:08:39

@mfikes did you try wrapping the global export statement in a setTimeout 0 in the REPL case?

dnolen21:08:56

wondering if that’s enough to make this work over adding a bunch of stuff

dnolen21:08:11

the thing I don’t like about 2854, is pushing that responsibility onto REPL implementers

mfikes21:08:12

Oh, yeah, that had crossed my mind, but I didn’t think of using 0. Hmm. Maybe sufficient for 1 tick later. 🙂

mfikes21:08:46

Definitely. I had already pinged Bruce so he is aware of that negative aspect of 2854. But there are other Bruce’s out there. 😞

mfikes21:08:14

I’ll give a setTimeout a try. It might suffice for the REPL use case.

dnolen21:08:34

fingers crossed

mfikes22:08:28

Dang. 0 might be too fast (it failed). I tried 1000, 100, 10 and all of those worked, so it is like rolling the dice. I also tried setImmediate, but Safari evidently doesn’t have that.

bhauman22:08:30

how about 5, it sounds like a good number

mfikes22:08:57

Hah, your typing speed is faster than mine. 🙂

mfikes22:08:26

(Assuming that this matters when you do a require, and then type some other form that depsnds on that require, all at the REPL. :)

mfikes22:08:36

Trying 5 🙂

bhauman22:08:14

I think this method is a good solution in the near term even though it is non-deterministic.

bhauman22:08:46

and actually a larger delay makes sense in this context because it's a REPL interaction

bhauman22:08:09

and is already latent beyond 100

bhauman22:08:41

ah but this won't only be applied in the REPL case will it?...

mfikes22:08:11

Well, the trick will be to check to see if (:repl-env env) is non-`nil` and only do it in that case

bhauman22:08:24

that would work

bhauman22:08:01

and then the hack is only applied in a fairly rare circumstance

mfikes22:08:04

I’ll put together a tentative patch that tries 100 for now. We can adjust (or some up with something more sophisticated)

mfikes22:08:29

No rush (this feature just turned 1 year old and nobody has complained yet)

bhauman22:08:31

I bet you'll never hear about it or experience it again

bhauman22:08:43

after the patch

mfikes22:08:59

Right, we will all forget about it and move on with life 🙂

bhauman22:08:12

but it deserves a code comment for sure!!!

bhauman22:08:51

imagine trying to figure what thats about in 5 years

bhauman22:08:01

or in my case a month

bhauman22:08:23

@mfikes @dnolen just wanted to let you know that -b dev:admin now works

mfikes22:08:36

Ahh, nice.

bhauman22:08:37

merging metadata as well

mfikes22:08:20

By the way, I love that I can test CLJS-2854 downstream with Figwheel by replacing

clj -m cljs.main -co co.edn -r
with
clj -m figwheel.main -co co.edn -r

darwin22:08:48

what about instead of one-off setTimeout, there would be emitted code which would do polling of goog.global["mylib"] availability and stop polling when assigned, it could try it immediatelly, then after every 50ms or so

mfikes22:08:25

That would work too

mfikes22:08:41

@darwin I don’t mind if you’d like to take my patch and turn it into that

darwin22:08:31

I’m sorry I not going to take this one 🙂