Clojurians
#cljs-dev
<
2017-07-03
>

This page is not created by, affiliated with, or supported by Slack Technologies, Inc.

dnolen00:07:52

merged the module stuff to master since I couldn’t find a way to break it here - feel free to do your worst and report back :slightly_smiling_face:

dnolen00:07:33

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

dnolen00:07:59

I don’t 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’t 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’s 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’t know but it doesn’t 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’t 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’t 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 “significant” difference is that shadow-cljs covers the set-loaded! call as well

thheller08:07:56

any particular reason you don’t?

dnolen11:07:00

@thheller no reason other than I wasn’t 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’m not against it - but it should probably be done via some generic simple feature we don’t 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 :slightly_smiling_face:

dnolen12:07:28

@anmonteiro I haven’t 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’m thinking we should do something similar for enabling console print

dnolen13:07:51

really what I’m 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’t an issue

dnolen13:07:13

we could probably cover Nashorn / Rhino users via :target so this being set to true isn’t 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’t know but it’s just a hook into the real API so I don’t 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’t think it matters, @thheller believes it’s not idempotent but I couldn’t confirm that here

dnolen15:07:51

and I can’t 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’s 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’t fire again

dnolen15:07:11

and I think relying on order of leaf entries in a module doesn’t 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

dnolen16:07:03

@anmonteiro fixed

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 :slightly_smiling_face:

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’t defined since bar/b.cljs isn’t loaded yet

thheller16:07:00

at least in my head, didn’t 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’s 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’m 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’t follow

dnolen16:07:24

you couldn’t 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’m saying is it if it wouldn’t work w/o :modules, then it doesn’t work

dnolen16:07:06

there is no order to leaf entries

thheller16:07:43

I don’t follow sorry … do you not sort :module entries in dependency order?

dnolen16:07:50

:entries are unordered entry points, if there’s dep you don’t need to express it

thheller16:07:46

I’ll 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’t matter if :entries is not

dnolen16:07:45

then it won’t be a problem

dnolen16:07:56

you can’t 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 :slightly_smiling_face:

thheller16:07:50

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

dnolen16:07:11

no actually I can’t 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’s not confusing

dnolen16:07:53

but if you have a interesting idea I’m 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’t 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’m kind of meh about that - what’s there is just a super lighweight thing for quick testing

dnolen16:07:27

and there’s no need to greatly increase it’s 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’t 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’s just it, in the case of this ClojureScript enhancement you’re only going to get a cljs.loader/set-loaded! call in a namespace that actually requires cljs.loader, we don’t 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’t have to

dnolen16:07:01

sorry don’t follow, how does that matter?

dnolen16:07:16

if you don’t load it, you’re not involved in loading

thheller16:07:40

> you’re 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’t review the code so you didn’t see how this is done :slightly_smiling_face:

dnolen16:07:30

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

thheller16:07:45

sorry .. I’m confused trying to juggle work and this … I’ll 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—I didn't understand that part and it might not be appropriate to copy

dnolen16:07:22

it’s not really dealing with sets

dnolen16:07:34

we just represent inferred type union as as set

dnolen16:07:57

is that what you’re 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’m not so concerned about that :slightly_smiling_face:

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’s 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’t 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’t 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’t 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’t 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 “internal”, but not private :slightly_smiling_face:

dnolen17:07:05

it doesn’t need to be private, I agree

dnolen17:07:14

it’s 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’t think we need unsafe-set

dnolen17:07:13

I just don’t see how that won’t 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’s it

anmonteiro18:07:09

@dnolen just got bitten by adding an entry which doesn’t require cljs.loader

anmonteiro18:07:32

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

dnolen18:07:30

@anmonteiro I don’t know what there is to do here - you can have modules that don’t 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’t 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’t 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 “vendor splitting” but I guess you just answered it :slightly_smiling_face:

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’s 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

“pull 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’t 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’s pretty slick, shadow-cljs has a variant too

anmonteiro18:07:43

@dnolen I’m 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’m happy to help writing something but I don’t 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’t found major problems

anmonteiro18:07:30

but I don’t have a major codebase I can try this on

dnolen18:07:46

well I think this passes the “easy” test finally

dnolen18:07:00

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

dnolen18:07:08

so I’m not surprised that no one used it

dnolen18:07:50

Allen Rohner’s post was the best written reference and you can’t even get to it anymore :stuck_out_tongue:

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’m embarassed to say I don’t 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’s so cumbersome

anmonteiro18:07:34

^ Allen’s 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 :slightly_smiling_face:

dnolen18:07:49

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

dnolen18:07:33

I agree that shadow-cljs is probably a bit more “dummy-proof” than my approach - but I just didn’t 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’d love to see that

anmonteiro18:07:33

I think we’re 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’s 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’t think anything is missing

anmonteiro18:07:05

@dnolen I’m 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’t think there’s 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:37

@anmonteiro https://dev.clojure.org/jira/browse/CLJS-2061 major enhancement ticket here

dnolen18:07:22

also if people don’t come with a better name than js_transforms.clj that’s what we’re going to go with :slightly_smiling_face: \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’t matter

petterik19:07:52

Threw exception

dnolen19:07:19

so it didn’t take 10 minutes? it threw and appeared to hang but wasn’t

anmonteiro19:07:20

I think we had a problem where :parallel-build didn’t detect cycles

dnolen19:07:31

yeah :parallel-build doesn’t 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’t 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’s 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’re manually managing that before you definitely likely made a mistake

dnolen19:07:27

it’s too hard to get that right

dnolen19:07:00

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

dnolen19:07:12

the algorithm will infer where everything should go

dnolen19:07:52

so if it’s not a leaf entry, or something you want to pin to a module, don’t worry about it

dnolen19:07:06

and don’t 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:58

@dnolen updated Closure Compiler to latest release https://dev.clojure.org/jira/browse/CLJS-2161

anmonteiro19:07:12

more (commonJS) module processing fixes

dnolen19:07:29

anything cool/specific?

anmonteiro19:07:48

they haven’t 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’t look like it’s sufficient then right?

anmonteiro19:07:06

but the API didn’t change

dnolen19:07:07

we have to update usae

anmonteiro19:07:10

we don’t use it

dnolen19:07:17

oh really?

anmonteiro19:07:22

I don’t really know why we import it anyway :slightly_smiling_face:

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’ll do a follow up ticket cleaning up the imports

dnolen19:07:45

@anmonteiro I’m 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’t 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’s 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’s 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’ve 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’t 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’m 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’re 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’t in the module graph stuff

dnolen21:07:40

thanks much!

dnolen21:07:10

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

dnolen21:07:21

if you’re 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: “I don’t want this namespace to be included in my build and I’m 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’m getting things in my bundle that I don’t want

anmonteiro22:07:59

and Closure can’t remove because they’re 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’m 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 :slightly_smiling_face:

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 “I’m prepared to suffer the consequences” :slightly_smiling_face:

dnolen22:07:16

I think you could just write code to intercept build fns, but I guess you’re using a plugin

anmonteiro22:07:30

nah, I’m not using a plugin

anmonteiro22:07:32

I can write code

anmonteiro22:07:20

at Ladder we use Facebook’s Buck as our build system

anmonteiro22:07:36

which meant we had to write our own thing to compile ClojureScript :slightly_smiling_face:

anmonteiro22:07:57

(for better caching)