Fork me on GitHub
#cljs-dev
<
2015-10-19
>
mnespor14:10:34

That snippet of cljs.analyzer in your comment on CLJS-1423 just introduced me to reader conditionals

dnolen17:10:44

if some people could apply this patch and try it out that would be super helpful

dnolen17:10:53

I would love to move on this one

bensu17:10:54

@dnolen: @juhoteperi should I apply both patches in order to try it out?

juhoteperi17:10:48

The A patch only introduces new functions, B chages existing build pipeline to use the new functions

bensu17:10:41

@juhoteperi: thanks. should I make a repo for example and try it or just apply and try it elsewhere (normal usage)?

juhoteperi17:10:25

Tried with Figwheel. Doesn't work. It's implementing alternative Compilable class which doesn't obviously implemtn find-sources

juhoteperi17:10:04

Not sure if there is anything we can do for cases where build tool is implementing Compilable itself instead of using cljs.build.api/inputs

juhoteperi17:10:09

Separate protocol could make errors more clear

juhoteperi17:10:46

Cljsbuild is also implementing it's own Compilable

juhoteperi17:10:29

Works with Boot-cljs

juhoteperi17:10:34

One solution would be to: - Add new protocol for find-sources - If build input-path doesn't implement the new protocol but implements Compilable old pipeline is used but warning is displayed

juhoteperi17:10:11

That should offer easy transition for everyone

bensu17:10:12

@juhoteperi: what versions of figwheel and cljsbuild are you using?

dnolen17:10:31

if somebody wants to send a fix to cljsbuild that would be awesome

dnolen17:10:40

if we do this would like to make sure everyone is ready for it

juhoteperi17:10:58

@bensu: Figwheel 0.4. Didn't try with cljsbuild, only checkted the code.

juhoteperi17:10:06

I guess both Figwheel/Clojurescript-build and Cljsbuild could just use cljs.build.api/inputs as it has already been available for quite long, so the change should be quite simple

bensu17:10:19

hhmm I don't know why I can't reproduce your results. in my case figwheel works ok

juhoteperi17:10:52

@bensu: Did you update your clojurescript dependency to correct version?

bensu17:10:52

yeah, after applying the patches and using ./script/build I get version 1.7.158 installed locally and I'm using that.

bensu17:10:22

but don't worry, the real issue is not me reproducing results, but getting your patch on track

juhoteperi17:10:13

@bensu: Did you apply the patches to latest master? I got version 1.7.159 so either you missing the second patch or applied to older checkout (which should be ok)

bensu17:10:07

doh! like a dummy I properly applied the first patch and then just checked the second one with git apply instead of git am. thanks

juhoteperi17:10:20

Ah, figwheel is no longer using clojurescript-build

bensu17:10:35

I see the failure with cljsbuild now

bensu18:10:23

@maria: might want to hear about this ^

bensu18:10:08

and I can also reproduce the error with figwheel.

juhoteperi18:10:11

Eating now but I'll send PRs after

bhauman18:10:20

@juhoteperi: @dnolen I was about to start using build inputs anyway.

juhoteperi18:10:37

bhauman: I saw your comment about strange things with build inputs. Did you use apply? inputs takes in var args

bhauman18:10:19

Ahh that may have been it. It was a while ago.

bhauman19:10:13

@derean already on it simple_smile

juhoteperi19:10:43

Also, I noticed my second patch has a problem. It still contains old -compile call so it's doing unncessary work. Will update patches.

bhauman19:10:07

no need to update patches already done

juhoteperi19:10:28

@bhauman: Referring to Clojurescript patch

juhoteperi19:10:29

@dnolen: Are separate patches still preferred to one patch?

juhoteperi19:10:24

Updated B patch.

dnolen19:10:47

@juhoteperi: one patch is always preferred

dnolen19:10:56

my mental cache is constantly getting evicted

dnolen19:10:17

and seeing multiple patches on a ticket a huge burden in terms of review

juhoteperi19:10:06

@dnolen: The patches are now combined. I think there was a point for separate patches before, something like testing the functions before updating build pipeline to use the new functions.

dnolen19:10:12

@juhoteperi thanks for the PRs! so awesome

bensu20:10:42

@dnolen: since we are discussing patch etiquette, in some cases it would be easier for the reviewer if I submitted patches with (very few) commits that show a logical progression instead of a big one. for example, a) solved the issue b) added warnings for other unexpected behavior. is that ok?

dnolen20:10:32

squashed commit is always preferred

dnolen20:10:48

just document the different bits in the description

bensu20:10:22

good that I asked then.

bensu22:10:43

report/cry for help in CLJS-1469 :modules regression: I can trace the NPE to a call to .getOptions for a Compiler object here: https://github.com/google/closure-compiler/blob/04dd916be73f961f489906231cfb1a1ad0fd583a/src/com/google/javascript/jscomp/JSModule.java#L263

dnolen22:10:10

@bensu: huh I wonder if we just need to supply dummy options?

dnolen22:10:17

or rather just some empty options I mean

bensu22:10:05

I don't know, what is weird is that and in the REPL (.getOptions (make-closure-compiler)) fails but the method is in the source!

dnolen22:10:46

@bensu I don’t think it’s getOptions it’s that getOptions returns nil

dnolen22:10:58

@bensu btw did you try @juhoteperi’s patch?

bensu22:10:05

I didn't try the new squashed one. I tried the two part one, it failed on cljsbuild and figwheel but it worked without problems in doo which uses inputs

dnolen22:10:55

@bensu so it worked on one of your own projects?

dnolen22:10:16

@bensu that Compiler.java doesn’t tell me anything.

dnolen22:10:26

I’m saying the module code I wrote probably never sets options correctly

bensu22:10:04

when using cljs.build.api/watch with inputs (which is what doo does under the covers) it worked well in two projects: a toy one and a very big closed source app.

bensu22:10:20

If you want to be extra careful I can try the new one in both places again

dnolen22:10:33

ok that’s great to hear, yes testing the new one would be great

bensu22:10:17

ahhh I understand now. If the options are nil, then we get a NPE

bensu22:10:19

of course

bensu22:10:35

what I find strange is that in the REPL the class doesn't even have the method!

dnolen22:10:42

we call sort without setting compiler options

dnolen22:10:56

that’s the bug

bensu22:10:14

ok, the code uses options on (make-options opts) I'll investigate those and if they are what's needed

bensu22:10:43

but I'll try juho's patch first

dnolen22:10:09

we probably just need to call setOptions or something

bensu22:10:34

on the patch. the results for figwheel/cljsbuild are the same (error!). uses of watch/`build` with inputs seem fine

bensu22:10:39

but on the big application I'm getting goog.require could not find: goog.testing.watchers which might be related to the Google Closure Bump

bensu22:10:27

I still have the 1.7.159 before @juhoteperi squashed it, and that compiles fine the big application. after the squash I apply the patch and get 1.7.160 and the difference maybe the Bump GClosure commit which leads to the goog.require error

bensu22:10:42

(12 days ago)

dnolen22:10:10

heh right, can you test by doing an exclusion and using the older one?

bensu22:10:54

an exclusion of gclosure? sure

bensu23:10:41

so, it properly works by excluding and requiring v20150920 both in the app, or by swapping dependencies in clojurescript itself

bensu23:10:06

some of my deps are requiring goog.testing.watchers and that file is just not there.

dnolen23:10:25

k yeah can’t do anything about that being removed

dnolen23:10:29

but glad to hear it works