Fork me on GitHub
#cljs-dev
<
2017-08-10
>
anmonteiro02:08:57

it looks like the Maven release does have my commits

anmonteiro02:08:00

yep, just confirmed the JAR sources

anmonteiro03:08:27

what a weird release cycle

anmonteiro03:08:57

they cut v20170806 and maven-v20170806 and they’re not cut 1. at the same time 2. from the same commit

mhuebert08:08:56

this new :global-exports support is fantastic

mhuebert08:08:37

I migrated all of Maria & Re-View React deps to use React from npm/node_modules. It worked flawlessly, but compile times for :optimizations :none Maria builds increased by 3-4x (one build went from 43 to 133 seconds, another from 28 to 118). Then I discovered that the latest React from cljsjs has :global-exports, so I didn’t have to change my code at all, I just got rid of the npm dependency and added [cljsjs/react "16.0.0-beta.2-0"]. Now compile times are even faster than before, each build runs in 28 seconds.

mhuebert08:08:42

(unrelated: everything ‘just worked’ migrating from React 15.* to 16-beta, which I didn’t expect, as we make heavy use of its api surface)

mhuebert08:08:25

and getting rid of all these references to js globals (`js/React`, js/ReactDOM) is so nice.

juhoteperi08:08:11

@mhuebert :global-exports doesn't allow proper optimization of JS code etc. which using node_modules and Closure module processing allows

juhoteperi08:08:57

3-4x increase in built times sounds quite a lot, was this for initial compile only or did it affect recompiles also?

mhuebert08:08:17

@juhoteperi by ‘proper’ optimization do you mean smaller file size / faster, etc., or something else that would cause problems to avoid?

juhoteperi08:08:20

I don't remember seeing much difference when testing Reagent with node modules

juhoteperi08:08:00

@mhuebert Module processes JS code goes through the same advanced optimization pass as Closure JS and Cljs results, this includes Dead Code Elimination etc.

juhoteperi08:08:19

foreign-libs JS files (with :global-exports or not) are included as is

mhuebert08:08:40

@juhoteperi incremental compiles did not appear to be slow

mhuebert08:08:15

we are not using :advanced optimization as we use the self-hosted compiler

mhuebert08:08:31

it seems like the time required for google closure to process React is related to the size of the project it is being added to it looks like processing React/ReactDom took 90 seconds in both cases

juhoteperi08:08:56

Did you try with React 15.6 or 16 from npm?

juhoteperi08:08:28

(I'm not sure if 16 from npm even works, they have changed completely how it is packaged)

juhoteperi08:08:01

Yeah, React 15.6 consists of few hundred JS files plus the dependencies, and Closure has to read and convert all these

dnolen10:08:27

@mhuebert I think in the near future we’ll want to pursue a smarter approach to caching for dependencies - node_modules and sources from JARs. Compile these to some other location, and then copy them to :output-dir).

mhuebert10:08:50

@dnolen yeah that would make sense

anmonteiro15:08:49

@dnolen what failure did you get with 2315?

dnolen15:08:57

@anmonteiro will paste, running tests again

anmonteiro15:08:38

I forgot to stage the test files….

anmonteiro15:08:49

amending the patch now

anmonteiro15:08:04

replaced the patch

dnolen16:08:54

great, thanks

anmonteiro16:08:03

@dnolen I will add a comment to the patch in CLJS-2312, but wanted to check with you that the approach makes sense

anmonteiro16:08:44

It doesn’t munge default if the var-name has a namespace, because it’s my understanding that those cases will be munged to my.ns.var.default

anmonteiro16:08:51

which is valid in ES5

anmonteiro16:08:15

but if the var-name does not have a namespace we should munge it to default$ because a standalone default is not valid

dnolen16:08:09

ah so this works for var def & ref

dnolen16:08:26

and we have a test case for this already?

anmonteiro16:08:42

@dnolen the test case for this was included in CLJS-1620

anmonteiro16:08:04

and reverting those names to default serves as a regression test

anmonteiro16:08:33

the reason why I changed those variable names to something other than default in the first place was because they would throw errors

anmonteiro16:08:02

but let me amend the patch to add a comment, even if only in the commit message

anmonteiro16:08:22

@dnolen OK, patch replaced with an inline comment

dnolen16:08:15

great, thanks again