Fork me on GitHub
#cljs-dev
<
2016-11-11
>
dnolen18:11:07

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

dnolen18:11:33

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

anmonteiro18:11:42

@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

dnolen18:11:01

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

dnolen18:11:12

I just want to understand that the implications have been considered

dnolen18:11:18

should be on the ticket really

anmonteiro18:11:23

I don't understand your question

anmonteiro18:11:27

happy to add more context to the ticket too

dnolen18:11:42

foreign libs that are JS modules can pass through advanced compilation

dnolen18:11:58

not all of them of course

anmonteiro18:11:59

the intent of this patch is for JS modules only

dnolen18:11:02

but if you’re careful it works

dnolen18:11:21

but what I mean we need to document the semantics here

anmonteiro18:11:31

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

dnolen18:11:33

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

dnolen18:11:46

OK but nothing in the ticket talks about that

anmonteiro18:11:07

I'll provide more context in the ticket

dnolen18:11:26

just trying to point out it isn’t obvious 🙂

dnolen18:11:39

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

anmonteiro18:11:25

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

anmonteiro18:11:37

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

dnolen18:11:57

foreign libs don’t pass through advanced compilation

anmonteiro18:11:16

oh OK, I get what you're saying

anmonteiro18:11:38

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

dnolen18:11:22

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

anmonteiro18:11:35

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

anmonteiro18:11:25

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

anmonteiro18:11:47

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

anmonteiro18:11:36

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

anmonteiro18:11:34

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

anmonteiro18:11:11

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

dnolen18:11:21

hrm this seems undesirable to me

dnolen18:11:35

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

anmonteiro18:11:06

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

dnolen18:11:32

you have minified that thing which Closure will analyze

dnolen18:11:42

but none of those internal names or definitions are real

dnolen18:11:57

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

anmonteiro18:11:14

right, but does it matter?

dnolen18:11:24

yes since it doesn’t work 🙂

anmonteiro18:11:34

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

dnolen18:11:37

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

dnolen18:11:41

you often get broken output

anmonteiro18:11:39

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

dnolen18:11:51

yes let’s step back

dnolen18:11:01

:foreign-libs was always about random JS libs

dnolen18:11:12

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

anmonteiro18:11:12

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

anmonteiro18:11:08

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

anmonteiro18:11:31

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

dnolen18:11:55

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

anmonteiro18:11:09

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

dnolen18:11:34

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

anmonteiro18:11:46

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

anmonteiro18:11:02

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

dnolen18:11:13

nice, but we probably want some more data points here

anmonteiro18:11:18

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

anmonteiro18:11:28

yeah, this is just a first attempt

anmonteiro18:11:42

probably should try to do the same with other libs

anmonteiro19:11:25

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

anmonteiro19:11:43

needed to add some synthetic event externs

anmonteiro19:11:06

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