Fork me on GitHub
#cljs-dev
<
2017-07-03
>
dnolen00:07:52

merged the module stuff to master since I couldn鈥檛 find a way to break it here - feel free to do your worst and report back 馃檪

dnolen00:07:33

random thought - @anmonteiro, @mfikes and I did do some work around registering specs across incremental compiles

dnolen00:07:59

I don鈥檛 see why it would be a problem but we do try to do a require

dnolen00:07:07

hrm but w/o a lock

dnolen00:07:58

that might be it

dnolen00:07:16

in analyzer

anmonteiro00:07:36

the stacktrace didn鈥檛 show that

dnolen00:07:41

(line no. is master)

anmonteiro00:07:46

could it be that the exception is swallowed somehow?

dnolen00:07:52

well that鈥檚 just it

dnolen00:07:57

we do swallow it

anmonteiro00:07:09

yeah I see the try catch

anmonteiro00:07:15

so it would show up somewhere else?

dnolen00:07:27

I don鈥檛 know but it doesn鈥檛 seem like this is safe

dnolen00:07:32

so might be worth fixing that up

anmonteiro00:07:50

cljs.spec.alpha does depend on gen

anmonteiro00:07:59

I wonder if that could be it

anmonteiro00:07:21

let me build a version of CLJS with a lock around that require and see if I can repro

anmonteiro04:07:27

@dnolen > you may see an error about goog.require but this can be ignored

anmonteiro04:07:31

what is this about?

anmonteiro04:07:37

or rather, why does it happen?

anmonteiro05:07:09

also noticed that under :verbose or :compiler-stats we don鈥檛 output diagnostic information so created & attached a patch to https://dev.clojure.org/jira/browse/CLJS-2154

anmonteiro05:07:12

one more q: does loader/set-loaded! need to be called at the end of the namespace?

anmonteiro05:07:05

^ it looks like under advanced it doesn鈥檛 really matter (except the order in which the code executes will be slightly different - e.g. for toplevel code)

anmonteiro05:07:44

but with :none we need to call set-loaded! at the end of the file

anmonteiro05:07:01

I assume because of goog.require calls?

anmonteiro05:07:23

or like, code will stop being evaluated once you call set-loaded!?

thheller08:07:29

@dnolen FWIW I made a quick comparison between cljs.loader and shadow.loader, the only 鈥渟ignificant鈥 difference is that shadow-cljs covers the set-loaded! call as well

thheller08:07:56

any particular reason you don鈥檛?

dnolen11:07:00

@thheller no reason other than I wasn鈥檛 really interested in writing injection code for it

thheller11:07:49

seems like an unnecessary complication for the user though?

dnolen11:07:47

I agree insofar that I consider it a very minor one

dnolen11:07:35

that said I鈥檓 not against it - but it should probably be done via some generic simple feature we don鈥檛 currently have to append to the end of a source file like postamble

thheller11:07:33

yeah I have :prepend :prepend-js :append-js :append per :module and the loader is done entirely via that

dnolen11:07:53

but this is also pretty trivial work

dnolen11:07:59

someone else can have the honor 馃檪

dnolen12:07:28

@anmonteiro I haven鈥檛 had time to look into the error that comes up in the guide, only happens under browser REPL as far as I could - probably something simple

dnolen13:07:45

@rauh interesting about closing in Clojure apply

dnolen13:07:59

reviewing the Quick Start I realized the browser REPL section is much simpler now due to :browser-repl true compiler option (which just converts to a preload)

dnolen13:07:53

I鈥檓 thinking we should do something similar for enabling console print

dnolen13:07:51

really what I鈥檓 wondering is does anybody actually have a problem with this just always being set to true?

dnolen13:07:19

I mean pretty much everyone is targeting browser or Node.js, where setting it to true isn鈥檛 an issue

dnolen13:07:13

we could probably cover Nashorn / Rhino users via :target so this being set to true isn鈥檛 a problem

dnolen13:07:29

but in the near terms users can just set this default option to false to disable it.

rauh14:07:09

So: Re apply optimization: I'm thinking about doing a specialized implementation for IndexedSeq, which is by FAR the most common data type to be passed in.... For me. If anybody could run this little snippet in a real world CLJS app, that would be greatly appreciated. Then report back the types:

(def seen-types (atom {}))
(set! js/print_appy_usage #(js/console.log @seen-types))
(let [oapp apply
      new-apply (js/Object.assign
                  (fn []
                    (let [a (js-arguments)
                          xs (aget a (dec (alength a)))]
                      (swap! seen-types update-in
                             [(alength a)
                              (cond
                                (nil? xs) "nil"
                                (array? xs) "array"
                                :else (.-cljs$lang$ctorStr (.-constructor xs)))]
                             (fnil inc 0))
                      (.apply oapp nil (js-arguments))))
                  apply)]
  (set! apply new-apply))
Just paste the snippet, load the app and then either inspect seen-types or call print_apply_usage() in the console.

rauh14:07:44

For me, it's:

{3 {"cljs.core/IndexedSeq" 88, "cljs.core/List" 5},
               2 {"array" 127,
                  "cljs.core/IndexedSeq" 1286,
                  "cljs.core/PersistentVector" 213,
                  "cljs.core/LazySeq" 416,
                  "cljs.core/Cons" 1},
               4 {"cljs.core/LazySeq" 300}}

dnolen14:07:51

@rauh that seems to match my intuition - IndexedSeq is common in apply scenarios due to wrapping of arrays

rauh15:07:52

@petterik Thanks a lot. That helps. The LazySeq will get a little faster for higher arity invokes. it'll safe on some first/next calls. But good to see that you're also having a bunch of IndexedSeq which are super easy to optimize.

dnolen15:07:10

@anmonteiro I fixed up those issues from your testing the modules yesterday - thanks for the testing/feedback

anmonteiro15:07:38

@dnolen awesome, I'll give it another spin in the next hour. Can you explain the use case for set-loaded! now that it's automatically emitted?

dnolen15:07:26

@anmonteiro I don鈥檛 know but it鈥檚 just a hook into the real API so I don鈥檛 see a problem with it

anmonteiro15:07:49

Did you also fix the console errors in the browser? I can probably look at those when I get a chance

dnolen15:07:11

@anmonteiro yes already fixed

dnolen15:07:32

bad goog.require for cljs_base.js file under :none

anmonteiro15:07:29

Right, I also couldn't come up with anything. But one thing I wonder is: should we emit that call to set-loaded! if it's already present (manually called)?

dnolen15:07:23

I don鈥檛 think it matters, @thheller believes it鈥檚 not idempotent but I couldn鈥檛 confirm that here

dnolen15:07:51

and I can鈥檛 think of a way multiple set-loaded! calls could ever be problem - need an example

dnolen15:07:33

it seems to me to make that not idempotent would be unwise, so I suspect this is handled

anmonteiro15:07:56

I don't know exactly what it's doing, but yesterday I had my code evaluating differently based on whether the call was at the top or the bottom of my namespace

dnolen15:07:07

now that鈥檚 a different

anmonteiro15:07:33

I guess if it's idempotent it wouldn't matter even for that case

dnolen15:07:55

well different only the sense when event handlers fire

dnolen15:07:14

they will fire immediately

dnolen15:07:22

but they won鈥檛 fire again

dnolen15:07:11

and I think relying on order of leaf entries in a module doesn鈥檛 make much sense to me

dnolen15:07:26

and theoretically not supported since all docs have :entries as a set

dnolen15:07:21

but if someone can come up with a real problem case worth worrying about - will consider it

anmonteiro16:07:15

@dnolen I see there's still a mention of the console errors in the gist. Probably confusing now that it's been fixed

dnolen16:07:38

oh thanks let me fix that

anmonteiro16:07:38

Also java -cp cljs.jar clojure.main -m cljs.repl.browser was an interesting realization

dnolen16:07:00

@anmonteiro yeah @mfikes version is neater 馃檪

thheller16:07:44

@dnolen :modules {:bar {:entries [bar.*]}} bar/a.cljs triggers set-loaded! but bar/b.cljs contains (defmethod some-multi :bar [_]) .. user triggers loading of :bar and calls (some-multi ...) in callback which isn鈥檛 defined since bar/b.cljs isn鈥檛 loaded yet

thheller16:07:00

at least in my head, didn鈥檛 confirm in code. too busy with other stuff

dnolen16:07:42

@thheller right I would not consider that a problem since you did not express a dependency between leaf entries

dnolen16:07:56

if there鈥檚 a dependency it must already be expressed via your ns form

thheller16:07:18

it is? (ns bar.b (:require [bar.a]))

thheller16:07:43

point is that a triggers the set-loaded! since its first in dependency graph

dnolen16:07:56

I鈥檓 just saying that code is already broken

dnolen16:07:08

you have implicit deps between leaf entries

dnolen16:07:16

if you had those deps w/o modules

thheller16:07:20

I don鈥檛 follow

dnolen16:07:24

you couldn鈥檛 guarantee order anyway

thheller16:07:53

the order is correct 鈥 the set-loaded call is just in the wrong place

dnolen16:07:59

what I鈥檓 saying is it if it wouldn鈥檛 work w/o :modules, then it doesn鈥檛 work

dnolen16:07:06

there is no order to leaf entries

thheller16:07:43

I don鈥檛 follow sorry 鈥 do you not sort :module entries in dependency order?

dnolen16:07:50

:entries are unordered entry points, if there鈥檚 dep you don鈥檛 need to express it

thheller16:07:46

I鈥檒l test it out when I have more time 鈥 I think there is a problem but could be wrong 鈥

dnolen16:07:04

relying on side effects in unordered entry points is just not something we are going supoprt

dnolen16:07:14

defmethod is a sideeffect

dnolen16:07:19

defs are a sideeffect

thheller16:07:22

they are ordered

thheller16:07:35

(ns bar.b (:require [bar.a])) produces the order

thheller16:07:42

doesn鈥檛 matter if :entries is not

dnolen16:07:45

then it won鈥檛 be a problem

dnolen16:07:56

you can鈥檛 have a parent rely on a side effect in a child

thheller16:07:24

what the heck 鈥 bar.a correctly loads before bar.b but incorrectly triggers set-loaded

dnolen16:07:45

but ok now I see what you mean talking about code here is just making it more confusing 馃檪

thheller16:07:50

of course bar.b can rely on side-effects of bar.a

dnolen16:07:11

no actually I can鈥檛 think of a problem here

dnolen16:07:20

because set-loaded is fired in parent

dnolen16:07:30

the child event will fire when the handler is attached when it is loaded

dnolen16:07:50

so anyways - still need to see a real problem case

potetm16:07:15

@dnolen So I was just fixing to file 2 issues with how the browser repl serves files, and then I wanted to ask if a patch would be welcome on it. Then I noticed this issue: https://dev.clojure.org/jira/browse/CLJS-2150 which could solve both of the issues I found.

potetm16:07:33

So I was wondering work on that would be welcome.

dnolen16:07:34

@potetm well what did you have in mind? I was going to close that ticket actually - I just changed what the browser REPL emits when starting up so it鈥檚 not confusing

dnolen16:07:53

but if you have a interesting idea I鈥檓 listening

potetm16:07:30

The issues I found were: 1) the browser repl shouldn't fail when query params are passed (this is useful for cache busting) and 2) that it should be possible to start a browser repl from opening a static file.

potetm16:07:07

So it seemed those could be accomplished by pulling out a proper standalone webserver. But I haven't tried either of those things on latest, so maybe it's fixed?

dnolen16:07:29

1) sounds easy and contained scope

dnolen16:07:36

what do you mean by 2) exactly?

potetm16:07:42

It's been a minute since I was in the guts of the repl, but IIRC opening an index.html from the filesystem was disallowed by the browser repl. The content had to be served from the repl process.

potetm16:07:01

And IIRC that's not a hard requirement. It's just fallout of how that was implemented.

potetm16:07:57

I think the deal was in send-repl-client-page. It blindly pulls the host off the request, which doesn't work for filesystem files. This could be fixed by either defaulting to localhost or allowing the user to pass a hostname (or both).

dnolen16:07:08

you can do that (specify host/port)

dnolen16:07:12

and you can specify a static files dir

dnolen16:07:16

so I don鈥檛 really follow

potetm16:07:36

Hmm... I will re-investigate and follow up.

dnolen16:07:04

as far as pulling out a proper standalone server - I鈥檓 kind of meh about that - what鈥檚 there is just a super lighweight thing for quick testing

dnolen16:07:27

and there鈥檚 no need to greatly increase it鈥檚 scope

potetm16:07:57

Fair enough. If nothing else I may file a bug report and patch for allowing query params then.

dnolen16:07:25

@thheller after some more thought I do see your point if we were just generating setLoaded for every entry but we don鈥檛 actually do that. Also given how Google Closure Modules works it seems to me the design was always for setLoaded call to be explicitly handled by the user in the ns involved in loading. But that鈥檚 just it, in the case of this ClojureScript enhancement you鈥檙e only going to get a cljs.loader/set-loaded! call in a namespace that actually requires cljs.loader, we don鈥檛 do it for any old entry. So my hunch is this is fine for typical use cases.

dnolen16:07:27

but time will tell - easy to revisit if people just absolutely want a different behavior

thheller16:07:05

but only the the :modules that want to load modules will include cljs.loader, the modules that are loaded don鈥檛 have to

dnolen16:07:01

sorry don鈥檛 follow, how does that matter?

dnolen16:07:16

if you don鈥檛 load it, you鈥檙e not involved in loading

thheller16:07:40

> you鈥檙e only going to get a cljs.loader/set-loaded! call in a namespace that actually requires cljs.loader

thheller16:07:56

bar.core does not need to :require cljs.loader

thheller16:07:12

yet it will have set-loaded! call

dnolen16:07:16

ah but you didn鈥檛 review the code so you didn鈥檛 see how this is done 馃檪

dnolen16:07:30

set-loaded! will mark all deps as loaded, since anything else is impossible

thheller16:07:45

sorry .. I鈥檓 confused trying to juggle work and this 鈥 I鈥檒l take some time tomorrow and see if this is an actual problem or just in my head

thheller16:07:22

I just remembered my first :module-loader impl had issues with calling setLoaded too early

thheller16:07:50

might have been a different issue 鈥 will see later

dnolen16:07:00

cool thanks

dnolen16:07:04

@mfikes thanks for the aget/aset patch this is nice

dnolen16:07:12

the fix and the inference stuff

mfikes16:07:33

@dnolen I realized afterward: I blindly copied some of the numeric test that appears to be dealing with sets鈥擨 didn't understand that part and it might not be appropriate to copy

dnolen16:07:22

it鈥檚 not really dealing with sets

dnolen16:07:34

we just represent inferred type union as as set

dnolen16:07:57

is that what you鈥檙e talking about?

mfikes16:07:11

Also, realized: the fix might slow some things down given the extra checks in goog.object/get鈥 (simple-benchmark` shows it is an order magnitude slower)

dnolen16:07:07

@mfikes for which ops?

dnolen16:07:39

hrm actually immediately, I know that native protocols & string cache will get hit for this

mfikes16:07:41

@dnolen no specific ops identified that it could matter. Just observing

(simple-benchmark [o #js {:a 1}]
 (aget o "a")
 1000000)
Is faster than
(simple-benchmark [o #js {:a 1}]
 (goog.object/get o "a")
 1000000)

dnolen16:07:04

@mfikes well I鈥檓 not so concerned about that 馃檪

dnolen16:07:10

just real perf effect

mfikes16:07:21

Cool. Yeah. It only matters if it matters.

petterik17:07:04

Regarding cljs.loader. Is there any interest in adding (defn prefetch [module-name]) and/or (defn loaded? [module-name])?

mfikes17:07:46

@dnolen the "set logic" part of the patch I didn't understand (and you suggest is type union stuff):

+      (when #?(:clj  (set? t)
+               :cljs (cljs-set? t))
+        (or (contains? t 'array)
+            (contains? t 'any)
+            (contains? t 'js))))))

dnolen17:07:05

@mfikes yeah that鈥檚 just checking for type union case

potetm17:07:55

Okay. Just to be clear: This is about starting a browser repl when you open a file from the filesystem directly. I'm not sure should be a supported use case, because I'm not sure what it provides beyond the use case itself. But the first issue you run into when trying to do this is an invalid ppu (`PEER_POLL_URI`) field. This is because the hostname is not propagated to repl/start-evaluator.

dnolen17:07:00

@petterik patch welcome, I only exposed what I needed to get this working

dnolen17:07:44

@potetm I don鈥檛 understand the usecase?

dnolen17:07:00

the whole point of having the browser REPL start a webserver is to get around all those problems

dnolen17:07:48

@mfikes also that particular microbench may just become a no-op in the first case

dnolen17:07:12

@mfikes gonna test with string cache - that should give use a accurate picture, we don鈥檛 currently test it

mfikes17:07:21

Cool. The only testing I did for the aget / aset revisions was to run the unit tests

potetm17:07:02

No yeah. I think that's a valid call. I just noticed that it could be made to work without a webserver at all. And some other repls support that use case. (Also, note that I was wrong about this involving making a standalone webserver. This is a separate issue that I got tangled in my head.)

dnolen17:07:33

@potetm yeah I think that kind of enhancement doesn鈥檛 seem necessary - I think the query params stuff is more impactful

dnolen17:07:50

in my mind at least browser REPL is primarily about making the Quick Start comfy

dnolen17:07:06

and for devs who want to quickly test something w/o making a project

potetm17:07:19

:thumbsup:

dnolen17:07:47

so any enhancements that make it better for these use cases are high priority

potetm17:07:03

Awesome. I'll get that bug filed and see if I can get a patch in. Should be an easy fix, but it's gonna take a minute for me to figure the whole process out.

dnolen17:07:56

@mfikes OK not an order of magnitude slower, but 50% on V8, 25% FF, and JSC 鈥

dnolen17:07:21

3X faster 鈥 LOL

dnolen17:07:05

@mfikes I think what we want is unsafe-get

dnolen17:07:17

something with an ugly name that isn鈥檛 checked

mfikes17:07:17

Does that exist?

dnolen17:07:30

no suggesting we address this in your patch

mfikes17:07:28

Ahh, right. A private unsafe-get macro that is a copy of the current aget impl

anmonteiro17:07:54

why make it private though?

anmonteiro17:07:05

I suggest making it 鈥渋nternal鈥, but not private 馃檪

dnolen17:07:05

it doesn鈥檛 need to be private, I agree

dnolen17:07:14

it鈥檚 an ugly enough name

anmonteiro17:07:14

we have some of those cases

mfikes17:07:04

@dnolen I could attach a revised "fix" patch that adds an unsafe-get / unsafe-set. If so, an open question: Replace all the detected invalid uses of aget / aset to these new variants, or only the few for which perf matters (with the others using goog.object as the current "fix" patch does)?

dnolen17:07:44

@mfikes I don鈥檛 think we need unsafe-set

dnolen17:07:13

I just don鈥檛 see how that won鈥檛 be optimized away either by Closure Compiler or VMs

dnolen17:07:43

I think your patch is totally fine except for the few perf sensitive cases

mfikes17:07:06

OK, the native protocols & string cache cases

mfikes17:07:33

(Only guessing, based on what you had said earlier.)

mfikes18:07:25

I suppose I could sort out, for each change, whether it is perf-sensitive

dnolen18:07:22

native-satisfies? hash-string quote-string defprotocol

dnolen18:07:26

I think that鈥檚 it

anmonteiro18:07:09

@dnolen just got bitten by adding an entry which doesn鈥檛 require cljs.loader

anmonteiro18:07:32

does it make sense to issue a warning if an entry doesn鈥檛 require cljs.loader, or we can鈥檛 because of backwards compat?

dnolen18:07:30

@anmonteiro I don鈥檛 know what there is to do here - you can have modules that don鈥檛 need to worry about loading

dnolen18:07:00

I might be easier to understand the problem if you explain your expectations a bit and what led to the confusion

anmonteiro18:07:50

gotcha, I guess I just need to understand the use case for modules that don鈥檛 need to worry about loading

dnolen18:07:28

right so you can have shared modules for even better code splits

dnolen18:07:46

cljs_base reagent page1

dnolen18:07:39

reagent isn鈥檛 involved in loading - it must be a script tag along with cljs_base on the pages that need it

anmonteiro18:07:34

@dnolen my next question was gonna be about 鈥渧endor splitting鈥 but I guess you just answered it 馃檪

dnolen18:07:30

you can also do interesting tricks w/ the algorithm

anmonteiro18:07:33

but that brings up another question: how does page1 get reagent?

dnolen18:07:46

like have an empty parent module

dnolen18:07:58

it鈥檚 only purpose is to get the algorithm to lift shared stuff and populate it

dnolen18:07:02

without polluting base

anmonteiro18:07:40

like an empty namespace that just has a bunch of requires?

anmonteiro18:07:55

鈥減ull reagent, react, react-dom into this module鈥

dnolen18:07:31

{:shared {:entries []} :foo {... :depends-on [:shared} :bar {... {:depends-on [:shared]}}}

dnolen18:07:59

:shared is empty, but it won鈥檛 be after the algorithm runs

dnolen18:07:07

you can collect stuff here

dnolen18:07:30

@anmonteiro no need for empty namespace, just empty module

dnolen18:07:28

every input has to be assigned to a module - but we try to keep each compiler input as deep as possible in the module graph

anmonteiro18:07:23

this is pretty cool, just tried it

anmonteiro18:07:45

module manager stuff and a bunch of goog libs got pulled into that shared thing

dnolen18:07:02

yeah this is what JSModuleGraph does, and I just copied it - it鈥檚 pretty slick, shadow-cljs has a variant too

anmonteiro18:07:43

@dnolen I鈥檓 guessing this warrants a blog post in the new & shiny CLJS blog

anmonteiro18:07:47

or maybe just a guide

anmonteiro18:07:09

or like a series of guides

anmonteiro18:07:14

from beginner to advanced stuff

anmonteiro18:07:33

I鈥檓 happy to help writing something but I don鈥檛 think I fully grasp the more advanced stuff

dnolen18:07:48

definitely the next release will highlight it, I can write that post

dnolen18:07:13

I think the guide can use my thing as jumping off point - and have a trick/tips section at the end

anmonteiro18:07:23

so far it looks really good and I haven鈥檛 found major problems

anmonteiro18:07:30

but I don鈥檛 have a major codebase I can try this on

dnolen18:07:46

well I think this passes the 鈥渆asy鈥 test finally

dnolen18:07:00

the Google Closure docs for this stuff is so cryptic/non-existent

dnolen18:07:08

so I鈥檓 not surprised that no one used it

dnolen18:07:50

Allen Rohner鈥檚 post was the best written reference and you can鈥檛 even get to it anymore 馃槢

anmonteiro18:07:19

I have a static copy of that post

anmonteiro18:07:21

let me find it

dnolen18:07:01

fwiw I discovered the Closure: The Definitive Guide has lot of this information

dnolen18:07:16

I鈥檓 embarassed to say I don鈥檛 own it - ordered a copy off Amazon the other day

anmonteiro18:07:43

I should probably buy that too

dnolen18:07:32

other issue was that :none and :advanced were different here for Modules based projects

dnolen18:07:46

shadow-cljs did the right here in blurring that distinction as otherwise it鈥檚 so cumbersome

anmonteiro18:07:34

^ Allen鈥檚 post

dnolen18:07:39

ah cool thanks

anmonteiro18:07:01

I predicted the website would go down someday.. that info was far too valuable to go to waste 馃檪

dnolen18:07:49

hey yeah and that post more less shows how much work/knowledge it required to do things correctly 馃槢

dnolen18:07:33

I agree that shadow-cljs is probably a bit more 鈥渄ummy-proof鈥 than my approach - but I just didn鈥檛 want to go down the path of code injection or adding stuff to append code generically

dnolen18:07:57

so the one real gotcha is leaf entries involved in loading must require cljs.loader

dnolen18:07:06

@anmonteiro something that would be pretty cool/slick to try - splitting + code motion of NPM modules

dnolen18:07:52

I think how Google Closure works is way better than Webpack in this regard - it just needed a easier interface

anmonteiro18:07:07

yeah, I鈥檇 love to see that

anmonteiro18:07:33

I think we鈥檙e not too far from that working

anmonteiro18:07:52

I think it would even work today with some tricks

dnolen18:07:09

@anmonteiro is there something off the top of your head that鈥檚 missing?

anmonteiro18:07:44

I was gonna ask you that

dnolen18:07:53

probably the next slightly annoying task in my queue is string based require

dnolen18:07:59

@anmonteiro I don鈥檛 think anything is missing

anmonteiro18:07:05

@dnolen I鈥檓 happy to put that together

anmonteiro18:07:21

I can probably work on that before the next release

anmonteiro18:07:35

is there a ticket for it?

dnolen18:07:37

yeah I don鈥檛 think there鈥檚 any issues with code splits and :npm-deps - it should just work

anmonteiro18:07:43

I might forget otherwise

anmonteiro18:07:03

@dnolen yeah, my line of thinking is: if Closure understands the modules, it can probably do splitting too

dnolen18:07:22

also if people don鈥檛 come with a better name than js_transforms.clj that鈥檚 what we鈥檙e going to go with 馃檪 \cc @juhoteperi

juhoteperi19:07:36

@dnolen That name is fine

juhoteperi19:07:01

Will those files be normal namespaces or "special"? I think they will need to require other namespaces.

juhoteperi19:07:46

I guess if they will reside in classpath root like data_readers.clj, they will be special

dnolen19:07:49

@juhoteperi yes just planning on copying data_readers.clj

petterik19:07:52

Trying to test cljs.loader with our 18LOC .cljc + 2k .cljs project and 18 modules, compilation of cljs.loader hangs (runs forever). Gist with output: https://gist.github.com/petterik/ee058c53a0fc3a1f1a328b10e35d325f

dnolen19:07:54

so you can do whatever you want in that file

petterik19:07:08

Tried with and without my patch to CLJS-2160

dnolen19:07:01

@petterik do you have a cycle in your graph?

petterik19:07:58

I'm pretty sure I don't because it works without cljs.loader

petterik19:07:05

but actually, it just finished. Took 10+ minutes

anmonteiro19:07:26

are you using :parallel-build?

dnolen19:07:41

hrm that shouldn鈥檛 matter

petterik19:07:52

Threw exception

dnolen19:07:19

so it didn鈥檛 take 10 minutes? it threw and appeared to hang but wasn鈥檛

anmonteiro19:07:20

I think we had a problem where :parallel-build didn鈥檛 detect cycles

dnolen19:07:31

yeah :parallel-build doesn鈥檛 detect cycles

petterik19:07:37

It took ~10 minutes, then threw that

dnolen19:07:41

known issue at the moment

petterik19:07:05

I'm using :parallel-build

petterik19:07:29

(rebuilding without it)

anmonteiro19:07:49

right, so that might tell if you have cycles

dnolen19:07:49

@petterik hrm I think you have a messed up :modules

dnolen19:07:08

so I don鈥檛 think this is a cycles problem (yet)

dnolen19:07:12

there at least 2 issues here

dnolen19:07:23

1) perf (deal w/ that later after we resolve 2))

dnolen19:07:41

2) you some how hit deepest-common-parent with no common parents

dnolen19:07:00

should be impossible since eventually you hit :cljs-base

dnolen19:07:44

so 2) might be bug, but need to find an example

dnolen19:07:13

(if your :modules is messed up somehow, we should error out before doing all this work)

petterik19:07:11

Very possible. Let me try a few things.

petterik19:07:31

:modules has been sort of hard to maintain

dnolen19:07:46

oh let鈥檚 step back

dnolen19:07:57

whatever you currently have :modules get rid of it

dnolen19:07:06

do not declare anything but leaf entries

dnolen19:07:20

if you鈥檙e manually managing that before you definitely likely made a mistake

dnolen19:07:27

it鈥檚 too hard to get that right

dnolen19:07:00

@petterik basically in the new world, you don鈥檛 need to be explicit at all

dnolen19:07:12

the algorithm will infer where everything should go

dnolen19:07:52

so if it鈥檚 not a leaf entry, or something you want to pin to a module, don鈥檛 worry about it

dnolen19:07:06

and don鈥檛 bother with wildcards

petterik19:07:10

That sounds fantastic. Very excited about this

dnolen19:07:42

updated the site doc issue to reflect this pitfall https://github.com/clojure/clojurescript-site/issues/85

dnolen19:07:53

@petterik thanks for trying this out on something serious, very helpful

anmonteiro19:07:12

more (commonJS) module processing fixes

dnolen19:07:29

anything cool/specific?

anmonteiro19:07:48

they haven鈥檛 put out the changelog, I was mostly looking at https://github.com/google/closure-compiler/compare/v20170521...v20170626

anmonteiro19:07:09

something specific to CLJS is they renamed a class that we import in cljs.closure

dnolen19:07:02

@anmonteiro hrm that patch doesn鈥檛 look like it鈥檚 sufficient then right?

anmonteiro19:07:06

but the API didn鈥檛 change

dnolen19:07:07

we have to update usae

anmonteiro19:07:10

we don鈥檛 use it

dnolen19:07:17

oh really?

anmonteiro19:07:22

I don鈥檛 really know why we import it anyway 馃檪

dnolen19:07:09

oh we use the API not the actual thing

anmonteiro19:07:07

@dnolen I actually think those imports could go away

anmonteiro19:07:21

Maria used them to check if we were compiling aginst old vs new version of Closure

anmonteiro19:07:28

and we eliminated all those checks when we migrated

anmonteiro19:07:38

I鈥檒l do a follow up ticket cleaning up the imports

dnolen19:07:45

@anmonteiro I鈥檓 there testing the patch

dnolen19:07:49

I can do it as a follow up

dnolen19:07:04

@anmonteiro applied w/ removed imports

petterik19:07:16

Nice, got further. One of the problems I had was that I had defined a module for :cljs-base with {:entries #{<my-main-ns>} :output-to "<main.js>"} Don't remeber why I did this earlier, but removing it got me to the next error, saying Assert failed: Module <my-module> does not define any :entries. Trying to compile again without that module defined.

petterik19:07:42

I had defined <my-module> with {:entries []}, and it's not unbelievable that it wouldn't get any entries in it.

dnolen19:07:28

@petterik I would remove that, that validation existed before (not new)

dnolen19:07:53

this means no entries ever got assigned to that module even after assigning everything

mfikes20:07:54

Here is the new unsafe-get macro in the CLJS-2148 patch:

(core/defmacro unsafe-get
  "Efficient alternative to goog.object/get which lacks opt_val and emits
  unchecked property access."
  [obj key]
  (core/list 'js* "(~{}[~{}])" obj key))

petterik21:07:15

@dnolen compilation succeeded! Output:

Optimizing 333 sources, elapsed time: 444366.645424 msecs
Successfully compiled "resources/public/release/js/out/main.js" in 487.734 seconds.
Quick look at the splits show that it's done optimizations I had to do by hand before, which is really cool!

petterik21:07:20

My resources/public/releases/js/out/main.js is empty, but that's expected, right? My "main namespace" ends up in :cljs_base, as it should?

petterik21:07:24

However, when trying to load cljs_base.js in my browser I get error: React is not defined Even thought I see adding entry (cljsjs.react) to :cljs_base I don't see minified React at the top of cljs_base.js (which there usually is with foreign-libs if I'm not mistaken?).

dnolen21:07:28

@petterik I don鈥檛 see how your :main could ever go in :cljs-base

dnolen21:07:41

:main is an entry point, it should never be in the base module

dnolen21:07:03

:cljs-base is the root, your logical main should be a leaf

dnolen21:07:51

@petterik that鈥檚 nice to hear that placements of the splits at least appear to be working in a large project

dnolen21:07:12

@petterik how long were your builds before? (curious if module computation is adding significant time, though I would be surprised)

petterik21:07:08

@dnolen Clean build used to be ~130seconds

dnolen21:07:40

@petterik so there鈥檚 definitely a perf thing to look at here as well

dnolen21:07:13

@petterik I have a feeling a little bit of strategic memoization could go a long way here

dnolen21:07:17

@petterik you鈥檝e never attached YourKit to your ClojureScript project have you?

petterik21:07:07

I have never

petterik21:07:11

My yourkit install needs an evaluation key

dnolen21:07:31

hrmm 鈥 but

dnolen21:07:10

if I could just get at your inputs and your :modules I could reproduce, I don鈥檛 need your whole project

dnolen21:07:22

@petterik all I need is the :provides, :requires for all your compiler inputs

dnolen21:07:50

you could probably use with-redefs to capture these values when calling build-modules

dnolen21:07:41

no need for this right away - but if you could do that, would be huge help

dnolen21:07:02

I鈥檓 sure with some info we can make this take a couple of seconds, instead of hundreds

petterik21:07:09

@dnolen I'll gladly do it

petterik21:07:24

I'm trying to understand what you mean

petterik21:07:39

modifying some clojurescript code?

dnolen21:07:54

so you鈥檙e probably using a build tool instead of just calling the ClojureScript compiler directly

petterik21:07:55

or when I'm building it? (I'm using lein cljsbuild)

dnolen21:07:09

all you need is a build script

dnolen21:07:19

which just calls the compiler directly

dnolen21:07:29

when you call build wrap in with-redefs

dnolen21:07:00

so change cljs.closure/build-modules so that you map over sources the first argument

dnolen21:07:24

for each value in sources invoke (juxt [:provides :requires] x)

dnolen21:07:45

then I just need this sequence and your :modules def and I can see what the algorithm does with that much info

dnolen21:07:59

or at least determine the problem isn鈥檛 in the module graph stuff

dnolen21:07:40

thanks much!

dnolen21:07:10

I鈥檓 done for the day, but I can totally look into this in the next couple of days

dnolen21:07:21

if you鈥檙e concerned about the information

dnolen21:07:37

feel free to email me privately at <mailto:[email protected]|[email protected]>

petterik21:07:50

I'll do that shortly! Thanks

anmonteiro22:07:18

is there any way to blacklist dependencies through a compiler option?

anmonteiro22:07:40

some way that I can say: 鈥淚 don鈥檛 want this namespace to be included in my build and I鈥檓 prepared to suffer the consequences鈥

dnolen22:07:05

No why would you want/need that?

dnolen22:07:33

You could intercept build of course if want

anmonteiro22:07:51

I鈥檓 getting things in my bundle that I don鈥檛 want

anmonteiro22:07:59

and Closure can鈥檛 remove because they鈥檙e side-effecty

danielcompton22:07:30

@anmonteiro I'm sure I'm not telling you anything new, but we have dev/prod files with the same namespace name that we include selectively depending on the build to avoid dev tools being included in prod. But it sounds like this might be a different use case?

dnolen22:07:48

That works toi

anmonteiro22:07:06

yeah. this particular use case is timbre -> taoensso/encore -> cljs.test -> cljs.pprint

anmonteiro22:07:29

so I鈥檓 getting 100KB of cljs.pprint in my bundle because it has defmethods

darwin22:07:53

similar problem here with cljs.pprint, and no good solution: https://github.com/binaryage/cljs-devtools/issues/37

anmonteiro22:07:54

and I actually want timbre in the bundle for logging 馃檪

darwin22:07:10

btw. even some less-common closure library namespaces could be side-effecty and not 100% DCE-friendly, like goog.userAgent

anmonteiro22:07:07

btw this works incredibly well with CLJS: https://github.com/danvk/source-map-explorer

dnolen22:07:33

@anmonteiro I think a feature to elide certain namespaces sounds pretty scary

anmonteiro22:07:50

I did say 鈥淚鈥檓 prepared to suffer the consequences鈥 馃檪

dnolen22:07:16

I think you could just write code to intercept build fns, but I guess you鈥檙e using a plugin

anmonteiro22:07:30

nah, I鈥檓 not using a plugin

anmonteiro22:07:32

I can write code

anmonteiro22:07:20

at Ladder we use Facebook鈥檚 Buck as our build system

anmonteiro22:07:36

which meant we had to write our own thing to compile ClojureScript 馃檪

anmonteiro22:07:57

(for better caching)