@juhoteperi nice work on the GCC module stuff. I’m looking over the patch. looks “easier” than initially thought

although “easier” is relative, I imagine how much work you put into this 🙂

at least the diff is small if that’s some measure

diff is relatively small, but time used per line is quite high 😄

I’ve got a few questions: 1. js-transforms has been changed to do a little less work. is this because all modules now need to be processed in 1 go?

2. did you build the compiler and test it with the circle-color project? does it work with advanced compilation too?

1. Yes it is related to that. Previously each file would have been transformed just before running convert just for that file.

3. in the case of 2., probably externs are not needed anymore for the consumed libraries, or is this not true?

Because all files are converted in one step, all the files need to be transformed before Closure compiler tries to parse them.,

I did build the compiler, but did not try with circle-color yet

gotcha, interested in seeing if that works out ^

1. and js-transforms has not in fact been changed, write-javascript was changed so that transform and convert are run outside of it, by process-js-modules

@juhoteperi hah, I didn’t expand the diff 😄

that’s right

Yep, I was confused myself at first

;; FIXME: Should this contain other module types also?
;; And both from foreign-libs and ups-foreign-libs (cljs-1682)?
;; But probably other module-types can't be parsed by this compiler?

L1503

Yes, and the change of convert-js-modules API

But I guess the module-types are mostly internal detail anyway as they depend on Closure compiler

So people probably won't define their own convert-js-modules methods (and it is in implementatio ns)

right

I don’t understand that last part

what change in the API are you talking about?

doesn’t it look the same for CLJS-Compiler consumers?

Hmm yes

I guess calling it API is wrong

Just implementation change, convert-js-module -> convert-js-modules and different parameters

ah I see it now

I didn’t notice the pluralization

It used to be called for each foreign-lib with module-type, now it is called for each used module-type with list of all files with of that type

yeah, I got that

@juhoteperi so if I understand the problem correctly, my feedback for the FIXME I pasted earlier would be to include ups-foreign-libs too, filter by the ones that actually have :module-type and convert them

the problem might be dependency order, maybe?

I don't think we need to care about dep order here

Oh, but this is in the context of :commonjs

I get the problem about other module types now

Ah right, there is problem with amdjs

I think I added comment about that somewhere

amdjs and commonjs should be processed together

so they see each other

https://github.com/clojure/clojurescript/compare/master...Deraen:cljs-1762-2#diff-84ffd22349e5ca1fe6322cb0e379b3b1R1851

what module-types are you testing with?

I’ll build the compiler and test the Circle-color project

at least that’s what I was planning to do

it might be nice to have a script that downloads the deps for that (so we don’t need to include it) and builds it?

@dnolen the circle-color stuff?

I think @juhoteperi emulated that in the tests, not sure if we actually need it in full

Yeah in tests I just included "minimal" files, I manually selected small parts of the files

I could change it to download the files

The JS code probably doesn't really work, but it can be transformed and checked that transformation creates proper Closure module defs

@juhoteperi what’s blocking it from working? (maybe I missed that you mentioned this earlier)

The mininal React file contains just some function stubs, no real implementation

oh right, but I mean didn’t Maria’s version actually work?

Yes that should work

right so I think that’s preferable as a test since it’s the most comprehensive thing we have

@juhoteperi the circle-color project works with your patch 🎉

testing advanced compilation now

Sep 28, 2016 2:39:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/anmonteiro/Documents/github/circle-color/resources/public/js/out/Circle.js:5: WARNING - dangerous use of the global this object
  return React$$module$resources$public$js$libs$Circle.createElement("svg", {width:"200px", height:"200px", className:"center"}, React$$module$resources$public$js$libs$Circle.createElement("circle", {cx:"100px", cy:"100px", r:"100px", fill:this.props.color}));
                                                                                                                                                                                                                                                ^

Sep 28, 2016 2:39:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/anmonteiro/Documents/github/circle-color/resources/public/js/out/react.js:4: WARNING - constant module$resources$public$js$libs$react assigned a value more than once.
Original definition at /Users/anmonteiro/Documents/github/circle-color/resources/public/js/out/react.js:1
  module$resources$public$js$libs$react = f();
  ^

Sep 28, 2016 2:39:14 PM com.google.javascript.jscomp.LoggerErrorManager printSummary
WARNING: 0 error(s), 2 warning(s)

generates those 2 warnings, and doesn’t really work in the browser

probably not even related to the module stuff?

I actually get a hard error using advanced compilation with 1.9.229

@juhoteperi actually it was an externs problem… It kinda works with advanced compilation using your patch

I thought externs wouldn’t be necessary though?

Probably depends on if module does something dirty, like dynamically create methods

@dnolen there are now a bunch of new closure warning categories that we should probably consider adding to :closure-warnings https://github.com/google/closure-compiler/wiki/Warnings#warnings-categories

should this be a different ticket or included in @juhoteperi’s patch?

Could be probably done easy in different patch?

maybe, after yours has been committed

@anmonteiro different patch is going to be better

@juhoteperi @anmonteiro yes if the JS code is generating methods and then statically using them later - there will be problems

http://dev.clojure.org/jira/browse/CLJS-1795

@anmonteiro out of curiosity how big is the gzipped circle example now?

the compiled artifact, right?

yep

@anmonteiro also interested in whether there’s actually any improvements to advanced compile time? (i.e. building the tests)

right

I don’t really know how to serve gzip compression locally

should I just gzip the artifact?

@dnolen wow this can’t be right

the whole thing is 351kb, 85kb gzipped

DCE 🎉

actually that seems about right to me? would need to compare with CLJSJS React

the fact that React can’t work without externs is a bit of a bummer

externs damage DCE

well, I thought it would be a lot more

@anmonteiro it ain’t shabby! 🙂

extern inference will reduce the damage as the infered extern would only include really used properties

@juhoteperi yes for CLJS code

but we can’t fix the internal DCE issue for React and other libs

Ah, right

Well, maybe that can eventually be fixed on Closure or React side

still this is pretty cool

(use without exterrns)

CLJS master without <@U061V0GG2>’s patch:
Compile sources, elapsed time: 12941.43335 msecs
Compile sources, elapsed time: 0.135049 msecs
Applying optimizations :advanced to 71 sources
Optimizing 71 sources, elapsed time: 39783.06221 msecs


CLJS master with the patch:
Compile sources, elapsed time: 13317.732659 msecs
Compile sources, elapsed time: 0.194513 msecs
Applying optimizations :advanced to 71 sources
Optimizing 71 sources, elapsed time: 40123.820472 msecs

so not really any improvement there

@anmonteiro heh, little change - that’s ok

@anmonteiro oh what memory settings are you using though?

did you run it multiple times? looks small enough to be just error

oh.

I would provide at least 1gb of RAM

@dnolen any easy way to do that in the test script?

I don’t think so, we should fix that 😛

@juhoteperi the compilation time is long enough for HotSpot to take effect

hrm actually:

java -server -Xms1g -Xmx2g -cp "$CLJSC_CP" clojure.main "$CLOJURESCRIPT_HOME/bin/cljsc.clj" "[email protected]"

ah k

this is bin/cljsc

then yeah, so much for perf improvements 🙂

yeah 😞

that’s ok though

no change is better than a change for worse 🙂

yep

oh one more thing

I’m going to check the bundle sizes

between different Closure versions

Btw. once interesting change in 20160911 was that input sources (e.g. foreign-libs) can now include source-map

ah makes sense

for JSX stuff for example

@juhoteperi yes I saw that, I also saw some chatter about source map merging

but I didn’t investigate further - so annoying that we had to roll our own thing for that

but I guess we were way ahead of that curve

@dnolen the ClojuTRE talk portion of source mapping was compelling haha 🙂

seemed like a really hard thing

haha, certainly not the most fun feature to implement

now got 38920.114401 msecs compiling the compiler tests

and the bundle size is unchanged

@anmonteiro huh, I’ll give the patch a shot too later and see what the work machine says

ah right, you might get better times in that machine

this is a 2 year-old 15” Macbook Pro

this is awesome work @juhoteperi, thanks! 🙂

I second that ^

Thanks 🙂

We should probably check if it is really Closure-compiler compile which takes 40 sec, or if our optimize function is doing something unncessarily slow

@juhoteperi we should, though I’m pretty sure the advanced compile time thing only measures Closure time - if it doesn’t we should log that

@dnolen it seems it’s measuring more than that

It measures optimize function call time, which initializes compiler, builds input file lists etc, and creates the source map

Most of the stuff it is doing should be very fast, but I don't know about source map generation

source map generation for advanced output is not fast

last I checked that alone might double compilation time

but we’re not doing that for the test build anyway

@anmonteiro ah, we should fix that

meaning we should also measure Closure time only in addition to our whole optimization pass

@dnolen should be a simple change, I’ll open a ticket for that as well

thanks

s/change/addition

http://dev.clojure.org/jira/browse/CLJS-1796

@dnolen (eval-str "42") .... I removed the :statement check from (emit* :constant ..., it works and tests pass. Is it that simple?

@pat that doesn’t sound like a good idea

i didnt think so ha

first I would come up with a real goal see if co-aligns with issues we already know about and then ask questions on how to accomplish that goal

my impression is you want to be able to do simple scripting a la Clojure with ClojureScript

(where you don’t need explicit ns forms etc.)

Yes mostly already works, just trying tounderstand edge cases

“mostly already works"

as I said this is very non-trivial task

and not newbie friendly at all

however if you really want to understand what all needs to be done, I’m more than happy to walk you through it

let's do it

so first off we’re not interested in solving this for any particular usecase

i.e. simple scripting isn’t something we care about in and of itself

however we would like to get more portable semantics with Clojure - this is the real and only goal

right

things like deps.cljs, data literal files, simple scripts all share common need for ns-less files

in Clojure of course it’s an illusion

there is a default ns - the user ns

so we would like for this to work but there’s a serious problem which needs to be addressed first

how can we make it work for Google Closure compiler

you cannot have one logical ns splits across multiple files

this means we need fairly serious compiler support to preserve the desirable semantic

and we also need to impose constraints on what is permissible so that the work and user expecations are both reasnoable

for example - we would like require etc. to work

but it’s desirable to restrict their occurrence to the top of the file

@pat there’s some context in this issue http://dev.clojure.org/jira/browse/CLJS-1346

@dnolen just remembered, still awaiting feedback on the comment there ^ (I had forgotten about it, sorry)

ok I will study the closure code then and try to get a grip on things

@anmonteiro responded I think the thing that I’m getting stuck on is in my new comment

@pat as you can tell we’re in the very deep end - which is why no one has worked on it 🙂

@dnolen I think a :top or :toplevel key in the AST would probably be easier

as per your comment, as opposed to my proposal

@anmonteiro that sounds reasonable too - feel free to leave that comment

wrt. your question, don’t we have something we use for REPLS that’s :merge?

wouldn’t that work for files without NSes to see each other?

maybe it serves a completely different purpose and I didn’t get it right 🙂

you’re right that we can probably hack this to work for dev time without issue

I guess the only issue is compile time where we have to create the cljs.user file

we need to be clear that side-effecting requires cannot be depended upon

(when compiling a production thing anyway)

@dnolen when is cljs.user created now?

it never is - it’s only a REPL thing

ah right

well I mean technically it’s set in the compiler

but nothing is around it to make it actually work

so proposal: we create it upon seeing the first file without a NS statement

if we never see one, things work as before (currently)

oh hrm …….

here’s an interesting idea

so you can use multiple provides in a Closure file

cljs.user.file1

cljs.user.file2

etc.

and we can have a custom resolve for cljs.user to look in these sub namespaces to resolve a var

then everything should just work - hunky-dory

oh you might have mentioned this before

I don’t think so

feels like a new idea! 🙂

but maybe you’re right

and I just forgot

in anycase if we can make Google Closure do all the work - that’s better

and easier

@dnolen ah no you mentioned “gen a random NS"

https://clojurians-log.clojureverse.org/cljs-dev/2016-08-16.html

heh yeah

I don’t think it needs to be random

yeah, if we have the file name it could work

actually yes that would be best

probably also need to take path into account and munge it somehow

cljs.user.foo_file

though we’ll still need to disambiguate

since multiple deps.cljs and data literal files is a thing

yeah but they’ll probably be at different paths in the classpath, right?

actually they won’t be

deps.cljs and data literals files need to be at the top

definitely deps.cljs

right

so there would be no way of mapping errors to those files?

it should just work

maybe that’s wrong, we probably know which file "we’re compiling right now"

I don’t think we need to do anything at all

as long we generate closure namespaces

the only trick here is the custom var resolution

so this would be simpler than my other previous suggestions

source mapping, advanced compilation, dev time - it should all just work

@dnolen I’ll capture all this in a comment to the issue, but there’s something I’m not getting still

if we’re analyzing a file with an NS statement, do we use the :toplevel thing?

the name might be misleading to me

did you mean it only in the context of files without NS or every top-level declaration??

this is why I suggested putting top level into :env

and discarding it anywhere recursive

analyze-seq yada

@dnolen just to make sure I understand correctly: we’d assoc :toplevel true in the analyzer entry points (e.g. analyze-file) but remove it from the env in the recursive calls?

yes

and if we are in parse ‘ns it better be :top-level

and we also need to add specials forms for require etc.

right, no nested ns statements

and these special forms also check for :top-level

so then we could relax the assumption that every file needs ns

yes

this first part doesn’t seem hard to implement at all at first sight

we will need some compiler voodoo

if the first form isn’t ns we need to emit the code for declaring a cljs.user sub-ns

ah! that’s what I was missing

bbiab

@dnolen added a comment to the issue, let me know if anything doesn’t look quite right; I might have misunderstood something http://dev.clojure.org/jira/browse/CLJS-1346?focusedCommentId=43974&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-43974

this looks promising! I’ll only be partially happy when we have support for data_readers.cljc!

@anmonteiro summary looks good to me

awesome, thanks

looks like a fun weekend project 😉

@dnolen: now that I think about it, what did you mean by :top? Top-level expressions or top of the file?

I might have misunderstood what you meant by top

will need to be two separate things

top-level expression is one thing - top of the file another thing

the second one is a bit simpler, just some state for analyzing/compiling files

Yeah, but what do we want to do with toplevel expressions beyond disallowing ns, require and such?

that’s it

Or rather, only allowing them at the toplevel

Gotcha

So I suppose top of the file is not really needed since it'll just be the first form we find

Like it's done now

right the first form logic stuff should be simple

it’s just the hack we need in the compiler for emitting the first form, but even that should be ok

Are you talking about something that's already in place or?