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

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

http://dev.clojure.org/jira/browse/CLJS-1437

I would love to move on this one

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

@bensu: yes

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

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

@bensu: Normal use

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

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

Separate protocol could make errors more clear

Cljsbuild is also implementing it's own Compilable

Works with Boot-cljs

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

That should offer easy transition for everyone

@bhauman: ^

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

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

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

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

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

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

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

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

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

@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)

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

Ah, figwheel is no longer using clojurescript-build

I see the failure with cljsbuild now

@maria: might want to hear about this ^

and I can also reproduce the error with figwheel.

Eating now but I'll send PRs after

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

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

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

https://github.com/emezeske/lein-cljsbuild/pull/421 https://github.com/bhauman/lein-figwheel/pull/263

@derean already on it

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

no need to update patches already done

@bhauman: Referring to Clojurescript patch

oh cool

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

Updated B patch.

@juhoteperi: one patch is always preferred

my mental cache is constantly getting evicted

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

@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.

@juhoteperi thanks for the PRs! so awesome

@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?

squashed commit is always preferred

just document the different bits in the description

good that I asked then.

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

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

or rather just some empty options I mean

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!

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

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

@dnolen: not quite: https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/Compiler.java#L2311

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

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

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

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

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.

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

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

will do.

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

of course

yep

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

we call sort without setting compiler options

that’s the bug

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

but I'll try juho's patch first

we probably just need to call setOptions or something

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

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

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

which is perfectly explained by this commit https://github.com/google/closure-library/commit/cd9bc3672c3bb2c50ddcbe190d46fc58355e628d

(12 days ago)

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

@bensu: ^

an exclusion of gclosure? sure

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

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

@dnolen: ^

k yeah can’t do anything about that being removed

but glad to hear it works

@bensu ^