going through JIRA - if there’s something you want me to look at - let me know

@dnolen pending tickets for me are: http://dev.clojure.org/jira/browse/CLJS-1822 and http://dev.clojure.org/jira/browse/CLJS-1847

@anmonteiro yeah testing 1847 - what’s the idea behind 1822?

@dnolen hrm, say React, for example, has dev and production bundles. The use case is for the Closure Compiler to process the production bundle when compiling with :simple or :advanced

@anmonteiro but what about foreign libs that can pass through advanced?

I just want to understand that the implications have been considered

should be on the ticket really

I don't understand your question

happy to add more context to the ticket too

foreign libs that are JS modules can pass through advanced compilation

not all of them of course

the intent of this patch is for JS modules only

but if you’re careful it works

but what I mean we need to document the semantics here

if you want your :file to pass through :advanced, don't specify a :file-min

is the idea that :file-min is a thing for JS modules where we know :advanced won’t work

OK but nothing in the ticket talks about that

I'll provide more context in the ticket

thanks

just trying to point out it isn’t obvious 🙂

and it will need to be documented because it could easily be a source of confusion

AFAIK there's already a :file-min option in foreign-libs

this patch is just mimicking that for foreign libs that are JS modules

@anmonteiro can’t be true

foreign libs don’t pass through advanced compilation

oh OK, I get what you're saying

🙂

@dnolen updated the description. Let me know if anything is still not clear http://dev.clojure.org/jira/browse/CLJS-1822

@anmonteiro so the patch doesn’t affect modules that can be advanced compiled?

not at all. It just uses :file-min (if specified) as a drop-in replacement for the one in :file to go through Closure Compiler processing

the easiest example is to think of React. It has a bunch of invariants in the non-minified version which we don't want to include in our production builds

so when building for production, we want to use react.min.js instead

(both still go through the Closure Compiler. but the production version is used when we're compiling with :simple or :advanced)

of course you know this, but this is not about minified or not, since everything goes through :advanced in the end anyway. It's about libs which have different dev/prod versions

again, let me know if I can make any of this clearer

hrm this seems undesirable to me

if you have :file-min you don’t want it to go through Closure - it will likely just cause trouble

why though?

because e.g. requires might be named something else?

you have minified that thing which Closure will analyze

but none of those internal names or definitions are real

the file is something which has already passed through some other minification process

right, but does it matter?

yes since it doesn’t work 🙂

the only thing that I can think that might break is externs for some internal stuff

this is why I created :file-min in the first place

you often get broken output

oh OK, so this could probably be my misunderstanding of what :file-min was supposed to do 🙂

yes let’s step back

:foreign-libs was always about random JS libs

they necessarily must come with externs and they could never be part of the build

I get what the problem is. feel free to disregard the patch for now

but say that React can be processed by the Closure Compiler. We still need a story for production builds there

i.e. I don't want to be using the development version of React in my :advanced builds

I’m fine with the big idea, it’s something I’ve been thinking about - but it needs to be more careful

and they differentiate the checks based on process.NODE_ENV or whatever it's called

yeah can’t think about it more at the moment - but it needs some thinking through

agreed

@dnolen FWIW in the React case, it works perfectly going through :advanced in the Circle Color project

I'm trying it out with my patch to CLJS-1822 applied

nice, but we probably want some more data points here

consuming react.min.js and react-dom.min.js

yeah, this is just a first attempt

probably should try to do the same with other libs

just got the Circle Color project working fully with :advanced / cc @juhoteperi

needed to add some synthetic event externs

(also working with the CLJS-1822 patch and :file-min)