Fork me on GitHub
#cljs-dev
<
2017-02-24
>
dnolen00:02:48

Looks like Closure February release is out

anmonteiro00:02:21

Oh I was just yesterday thinking about when that would be due

anmonteiro00:02:32

I can try and upgrade it later today if no one gets to it first

dnolen00:02:25

definitely won’t take a look until tomorrow

anmonteiro01:02:57

of course, I spoke too early

anmonteiro01:02:05

cannot find modules now 🙂

anmonteiro01:02:39

we need to (.setModuleResolutionMode compiler-options ModuleLoader$ResolutionMode/NODE)

anmonteiro01:02:21

probably worth discussing if we want to have a new compiler option for the module resolution modes. there exist LEGACY, NODE and BROWSER. more context: https://github.com/google/closure-compiler/wiki/Releases#february-18-2017-v20170218

anmonteiro01:02:27

I think we’ll mostly want NODE, not sure what BROWSER is for. I think only Microsoft Edge supports it

anmonteiro01:02:30

patch attached

thheller08:02:00

I recently rewrote the whole node.js support in shadow-build as I was getting rather frustrated with the default node.js bootstrapping mechanism and its differences to optimized output

thheller08:02:18

tried writing down my motivations and implementation

thheller08:02:05

I'd be interested to hear some other opinions on the subject

thheller08:02:19

currently setting up a proper demo so you can actually play with it

thheller08:02:42

but at this point I have no idea if the things I "fixed" actually bother other people as well or just me

thheller08:02:23

:none vs optmized and (js/require "./rel/path") not working as expected

dnolen19:02:58

@anmonteiro can you explain this patch a bit?

anmonteiro19:02:39

@dnolen sure. We’re using compile-root which ends up calling that function. Given it’s a #?:clj only function, it shouldn’t be finding .cljc files if a .cljs file with the same name exists (since those are not really CLJS files but macros to be expanded in Clojure)

dnolen19:02:12

but how was that happening?

anmonteiro19:02:15

we saw the problem with om.dom, for example, where the compiler would end up compiling dom.cljc (with the wrong requires) instead of dom.cljs

dnolen19:02:18

that’s an or with .cljs first

anmonteiro19:02:49

yeah, but it ends up including both

dnolen19:02:26

there’s no explanation about that

dnolen19:02:46

because it just calls file-seq

anmonteiro19:02:18

@dnolen anyway, we’re using a version of CLJS with that patch now, in production

anmonteiro19:02:32

and it has been working great for the past 2 weeks (which means like 200 builds or more)

dnolen19:02:52

cool I understand the problem - just was trying to figure out the patch

dnolen19:02:37

might be related to that :parallel-build issue reported by @tonsky

dnolen19:02:46

going to try that next

dnolen19:02:18

@anmonteiro did you see the problem under :parallel-build or it was just an issue in general

anmonteiro19:02:22

interesting. I wouldn’t think they are connected

anmonteiro19:02:04

@dnolen nope, this is unrelated to :parallel-build

anmonteiro19:02:07

always happened

dnolen20:02:14

now would be a good time to try master

anmonteiro20:02:22

I’ve seen different entries for the same NS after ordering deps, but somehow I thought that was normal

anmonteiro20:02:46

I can see how that would impact parallel build now 🙂

bronsa20:02:24

@dnolen i probably have

bronsa20:02:58

we've had some intermittent issues with parallel builds in the past and decided to turn them off, but never had any time to find a repro

moxaj20:02:23

@dnolen the fix for CLJS-1948 might fix CLJS-1921, but there still might be some weirdness going on, see ticket http://dev.clojure.org/jira/browse/CLJS-1921

bronsa20:02:27

i'll give parallel builds another try after that fix

dnolen20:02:10

@moxaj someone needs to confirm that master is still an issue for 1921

dnolen20:02:05

@anmonteiro yeah not normal 🙂

anmonteiro20:02:36

@dnolen can we expect a CLJS release today, or the parallel build issue doesn’t justify it?

dnolen20:02:31

let me check CLJS-1921 first

anmonteiro21:02:13

definitely looks like the same symptom of cljs-1948

anmonteiro21:02:19

let me try and repro cljs-1921 too, against current master

dnolen21:02:02

can’t repro CLJS-1921 with master here

moxaj21:02:52

I can't build master because i'm on windows 😞 But I suspect it would fix it. However, wouldn't it make more sense to eliminate the duplicates right as they are inserted? See the java.io.File - java.net.URL mismatch

moxaj21:02:25

(unless that is intentional)

dnolen21:02:59

too risky to change it there I think so I did the simplest thing I could think of

dnolen21:02:23

somebody else will need to spend some time to suss out the deeper issue and determine if it isn’t going to break the world

moxaj21:02:12

nitpick, but i'd argue that was the easiest 🙂

dnolen21:02:30

not if I can’t test every lib

dnolen21:02:50

de-duping inputs seems more clear

moxaj21:02:13

I mean, I understand your reasoning, i'm just riding on simple vs easy

dnolen21:02:40

right but I don’t think you’re quite clear what’s going wrong

dnolen21:02:47

it doesn’t have anything to do with file vs. url

dnolen21:02:12

but the fact that we use the compiler input as a key

dnolen21:02:18

so everything about that key has to be same

dnolen21:02:39

auditing for that is a massive pain when I just want to fix the bug

anmonteiro21:02:42

I can’t repro cljs-1921

anmonteiro21:02:58

but I also didn’t manage to repro it with and older cljs.jar, so I don’t know what that says

moxaj21:02:16

@anmonteiro there are some very specific conditions required to repro, I've tried my best to describe those in the ticket

anmonteiro21:02:33

I know how issues like these are not deterministic

moxaj21:02:44

it relies on set ordering etc.

anmonteiro21:02:46

I’ve tracked down 2 or 3 related ones

dnolen21:02:57

right but the set ordering issue goes back to what I said

dnolen21:02:11

which is my belief that using the the entire source input as the key was a bad idea to begin with

dnolen21:02:20

instead key on :provides

anmonteiro21:02:30

my rubber duck tells me it should be solved by the fix to cljs-1948

dnolen21:02:35

the same namespace appearing twice in a build can’t possibly be right ever

dnolen21:02:05

trying to fix this at :url and :file isn’t a good idea

dnolen21:02:10

nearly everything is sourced from the classpath anyway

moxaj21:02:26

oh, now I see what you mean - it is an issue, but shouldn't be solved directly

anmonteiro21:02:26

I agree that we shouldn’t mess with :url and :file

dnolen21:02:44

I’ll leave 1921 open for now and cut a release then