@dnolen @anmonteiro It works! This is amazing 🙂 https://gist.github.com/Jannis/c101329b80bbc6a498129af3e702b3f8

Cool! :)

(One error still (even though it carries on) but that’s a circular dep inside graphql-js, which will be fixed in the next release.)

Other than that, not a single problem importing all those libs.

Even react-apollo works out of the box. Nice! (gist updated with an example)

Had a little fun with an experiment for warnings on private var use

cljs.user=> (def ^:private a 3)
#'cljs.user/a
cljs.user=> a
3
cljs.user=> (ns foo.core)
nil
foo.core=> cljs.user/a
WARNING: Use of private Var cljs.user/a at line 1 <cljs repl>
3
foo.core=> @#'cljs.user/a
3

@dnolen Right, the problem is however that the MetaFn is an (ugly) hack that breaks down. For instance when we have a defn like this:

(defn foo [a b & c] ...)
;; and call it like this:
(foo 1 2 3 4)
;; CLJS will generate code for static-fns like this:
the.ns.foo.cljs$core$IFn$_invoke$arity$variadic((1),(2),cljs.core.prim_seq.cljs$core$IFn$_invoke$arity$2([(3),(4)], 0))

This however will be a complete mismatch to the new IFn$_invoke$arity$variadic that got generated by the (fn [& args]). E.g. this fails:

(defn ^:dynamic *foo*
  [a b & c]
  [a b c])
(MD/ensure-static-fns! true)
(defn call-variadic []
  (*foo* 1 2 3 4))
(MD/restore-static-fns!)
(let [orig *foo*]
  (binding [*foo* (fn [& args]
                    (apply orig args))]
    (call-variadic)))

Though, this has nothing to do with 2641 and is separate bug. The problem with 2641 isn't dynamic binding but the fact that the dispatchers and the variadic impls will refer to the namespace object which will be the new (wrapped) impl of spec. Thus the stack overflow

Check the implementations for top level defn's:

(defn bar [& a] ())
(defn print-fn
  [f]
  (into {"dispatcher!!" (str f)}
        (map #(-> [% (str (aget f %))]))
        (seq (js-keys f))))

(print-fn bar)

And you'll see how the stackoverflow happens

And unfortunately the problem is that there seems to be no easy way to fix this. One solution is to write an apply that avoids the applyTo call and goes straight to all actual impls. FWIW, I've advocated for such a centralized applyTo last year. But I'm still waiting on the IFn refactorings and apply test-suit to be merged.

Wow, this is amazing. I didn't know you can have a git dep :sha pointing at a commit in a branch. You can trivially share an experimental change to the compiler to get feedback: https://gist.github.com/mfikes/e05fc6003f24b6565540d42f1c9dbbae

I was thinking a bit about the “runtime config” idea[1], one use case I had in some of my libraries was that based on external configuration my library would elide some code (e.g. elide logging or some diagnostics checks[2]). First, I thought it would not be possible to do that anymore, because it was done in macros at compile-time (based on configuration a macro would emit different code). But then I realised that the main use case for such optimizations was :advanced mode. It turns out I can still use :closure-defines and in advanced mode rely on google closure compiler to DCE code which doesn’t get called (thanks to the fact that :closure-defines gets baked-in as a static code in advanced mode). It is much more brittle but doable I guess. But still I’m thinking about something like “compile-time config” which would be available to macros. And clojurescript compiler would make sure anytime this compile-time config changes, AOT caches get invalidated. Sometimes it would be much more convenient to rely on macros to generate config-dependent code than to rely on google closure compiler. Any opinions? I saw David being strongly against blowing caches up, but I think this would be a rare thing and expectable to recompile whole project when compile-time config changes. Should I open a JIRA ticket for more persistent discussion about this? @bhauman @dnolen [1] https://github.com/darwin/clojurescript/compare/92cbedd12cd39d6bc3083a269a10fba9daa7fc40...runtime-config [2] https://github.com/binaryage/chromex/blob/master/src/lib/chromex/config.clj#L9

I’m not familiar with how AOT caches work in java/clojure but I can imagine that clojurescript could control cache directory based on compile-time config (e.g. hashing it and using this hash as a sub-folder). This way each compile-time config variant would get its own cachedir. It would not mean blowing up world cache for this to work. But maybe this is just a naive thinking on my part.

@darwin it already works that way

my stance on this remains unchanged

if your library breaks caching - it’s broken

@rauh I came to the same conclusions yesterday - spec instrumenting is just showing an issue that has been true for quite some time

the root of the issue is that we changed how top level defns work to support cross module code motion

@dnolen do you have some link where I could learn more about this to understand how it works? you said that relying on :external-config in compiler opts in my macros won’t work anymore, also I guess there is a lot of code which checks :advanced mode compiler option in a macro and does something different based on that, is it true, that this code won’t work anymore, because :advanced mode option is not part of cache-invalidation logic?

you keep talking about :external-config

this doesn’t exist

any library that does anything at macro time is 100% busted

people need to start realizing this is an anti-pattern, and the popular libs just need to stop doing it - full stop

@darwin however … libs can at macro time depend on cljs.analyzer/build-affecting-options

if those compiler options change - it’s a different cache

https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L3837

ok, thanks, I will look how it works there

the structure of the cache is right now

compiler-version/ns/path/build-affecting-options-sha/cached.*

@dnolen I see, so you have this mechanism already in place, would you be against adding another key into build-affecting-options? lib authors could safely rely on that configuration at macro-time (e.g. that :external-config me and @bhauman and maybe few others started using without you knowing[1]). There must be a way for me to elide some code at macro-time in a lib based on user-specified config. [1] https://github.com/bhauman/lein-figwheel/blob/master/support/src/figwheel/env_config.clj#L15-L16

How expensive would it be to just macroexpand the entire ns and then check if anything changed? That would solve all issues, though would need to be smart about all the gensyms... Just a thinking out loud

e.g. cljs-oops would be busted if it cannot do something like this at macro-time, it generates very different code under :advanced, see [1] and [2] [1] https://github.com/binaryage/cljs-oops/blob/master/test/transcripts/expected/ocall_dev.js [2] https://github.com/binaryage/cljs-oops/blob/master/test/transcripts/expected/ocall_static_core.js

@darwin cljs-oops is just broken then

libs cannot add keys to change the cache set, because libs will do conflicting things

then nothing will ever cache

@rauh not even going to go down the line of thought 🙂

everybody has to understand that caching is a global service, and ClojureScript controls it

allowing libs to muck with it isn’t solving anything

anyways, this isn’t the end of the world if some libs can’t do it

people will eventually learn to avoid such tooling

as they decide to adopt :aot-cache true

(also or they decide not to … it’s unlikely that :aot-cache will ever become the default given the state of things)

@rauh re: spec I sorted out a clever way to get out of the issue for now - since we bind f to itself via .call in the apply helpers we check that in the dispatch and choose that instead so we get the original properties and not the overriden ones

I can review if you have a patch or wanna just commit it

adding test and then will push to master

@darwin, actually cljs-oops is probably fine if you’re just checking advanced - while optimization level isn’t a cache key - we change other things that are

ok, that is good news, but still cljs-oops is currently more flexible than that, user can enable diagnostics in :advanced mode, yes, default behaviour was enabled in dev-time, disabled in advanced-mode, but technically there is no restriction to enable/disable it in either mode

yes if you’re doing more configuration via macros that will change compile output - of course that won’t work

@rauh https://github.com/clojure/clojurescript/commit/9dd4d41174079a568f27dcdf061568c985b2bd12

it’s a bit clever, but all the other various things I tried were even less desirable

@dnolen Why do we need the check in applyTo? Shouldn't this always be the fn? applyTo in only ever used in apply, nowhere else IIRC

you always need the check because in the generated code we go back through the namespace

Also I still think the MetaFn hack needs to go, the issue I described first (`variadic`) is still present. So there def needs to be a change in that spec function

@rauh I don’t understand your example from above

make some minimal case we care about that doesn’t work

and open an issue

@dnolen I dont understand that, unless somebody writes (let [f (.-cljs$lang$applyTo some-fn)] (f ...)) we always have this bound to the fn and the check will always go to your new way of invoking it.

@rauh I don’t think this point is really that important anyway - it didn’t work for me w/o but you can play around with it some more if you like

I’ve spent enough time on this 🙂

It's identical to https://dev.clojure.org/jira/browse/CLJS-1663, but instead of the compiler emitting f.cljs$core$IFn$_invoke$arity$1(...) it's for foo.cljs$core$IFn$_invoke$arity$variadic((1),(2),cljs.core.prim_seq.cljs$core$IFn$_invoke$arity$2([(3),(4)], 0))

if the cljs.compiler works with options that don't include a :main and it also supports code splitting which doesn't have a main, and figwheel works in these situations as well, then its not accurate to say that I can simple use closure-defines to propagate config @dnolen @darwin

@rauh MetaFn bit isn’t going to change unless you come up with an acceptable alternative for advanced compilation

This will throw now since the wrapper only defined foo.cljs$core$IFn$_invoke$arity$variadic( ONE_SINGLE_ARG )

it is accurate to say that artifacts that change based on the environment are probably a bad idea

@bhauman what you’re saying isn’t really true

code splitting has mains

and compiling without :main is a legacy thing

the output-to's are mains:

yeah but I encounter these things

I encounter folks who use don't use main

@bhauman but let’s be clear about what you are saying

if you don’t use main then you must do everything by hand

so you have to do Closure defines by hand too

there’s no issue here

I'm saying that closure-defines don't appear to be a silver bullet to solve my problem

@bhauman what I’m saying is you’re talking about a non-issue

if user doesn’t use :main they are doing it by hand already

OK figwheel currently works without a main

but that’s a Figwheel thing

not a ClojureScript thing

so your saying that figwheel will need to write the defines

yes

gotcha

I'll give it a try again, but when I've used them in the past it seemed spotty at best

@bhauman I don’t know how that can be true 🙂

this is how Google Closure works

BTW I also had a version of figwheel that tried to use code as an input source to propagate config

but a bug killed it immediately

@rauh anyways, just come up with something that isn’t working and make an issue - it’s too hard to understand what you are saying otherwise

I'lll have to go see what that was

I am very skeptical about claims about Closure defines

it’s just how it works, Google uses it all over the place

I’ve used it for years w/o issue, it’s how we do environment config

I never thought any of the macro things made much sense

other than it seemed “easy”

figwheel has been around a while and things have changed a lot

@mfikes CLJS-2624, so this just makes /out/main.js consistent yes?

@bhauman Closure defines have been there a long time - one problem in the past was people didn’t know how to make it worked uncompiled, but that’s not true anymore

@dnolen I mean there is more issues with such a wrapper. Eg. some people might add other props to a function. (Like we do for variadic fns). I'd suggest: Loop over all IFn and the variadic (those will have implementations) and wrap them individually. Much more robust.

Closure: The Definitive Guide - get it 🙂

@rauh this doesn’t seem important 🙂

@dnolen With CLJS-2646, the fact that it the default suggested has "/main.js" seemed like it might be an issue, if the user simply copies that template as is. I suppose things can work that way, but essentially the 3rd patch in that ticket tries to make it so that you have a relative path to main.js. It honestly turns into a bit of a hack.

(The relative path set up is out/main.js)

@mfikes I guess I’m wondering if the relative path thing is important here?

if you don’t set -d there’s no sensible value to show

@dnolen Arguably nothing is horribly broken as is. Things work. Perhaps I wrote the ticket out of aesthetics.

well, my problem probably was that when I wanted to use :closure-defines it usually was for optional code elision, and that would not work without dark-magic like adding ^boolean type hints to my code for closure compiler to recognise it, sometimes when I rewrote it to something more complex, DCE again stopped working because it was too complex for closure compiler to understand, in the light of these issue using a macro for generating only the code needed was a clear win

@mfikes yeah I think as long as we do the temp dir thing I think what it shows makes sense

and if you set -d out you’ll see the right thing

OK, yeah. (I'm never done much webdev, so the absolute path looked odd to me, but I can see that it could be viewed as normal; the a path starting from the root of where things are being served.)

@darwin that’s just end user trouble shooting - so again it’s just about ease and convenience - which sent everybody down the wrong path in the long term

@mfikes ok lowering priority for now

Sounds good.

@rauh I don’t think you can loop over IFn and the other properties as strings, not advanced compilation safe - you’d have to write something that unrolls into static operations

anyways that’s what this is about - advanced compilation

other approaches are just more work - and offer little actual benefits

if people adding random properties to top level fns - good luck with that anyway

it’s nothing we need to support, or have ever claimed to support

@dnolen Right, I didnt mean "loop" int he literal sense. I'd suggest a macro looping over all IFn & variadic

yeah no benefit

anyways this is fine for now - I have other things to look at

A cleaner solution IMO would be: Don't (set! ...) the given var-fn at all. Instead: Save the original fn as f.specImplFn = f, then a f = spec_checking_fn_which_calls_specImplFn_on_itself. Could even use a nice protocol for it.

Anyways, I guess unless user have issues with it... Nothing more todo. Would be a lot of work 🙂

yes I already thought about these options 🙂

as far as I’m concerned this is the cleaner way - less pointless churn

Yeah agree, though it could be cool.. You could spec data-types that way on their IFns 🙂

And I know: Not a feature in CLJ so not a feature for us to have 🙂

exactly 😉

@mfikes re: the path thing, Quick Start covers -d out so hopefully that gets people doing that most of the time

Cool

definitely personally benefiting from cljs.main just sorting through bugs

clj -M:cljs -co "{:source-map false}" -c cljs-spec-bug.core -s

is how I worked through the spec one

Yeah, this whole thing is an enabler, which we are only starting to scratch the surface with 🙂

@juhoteperi https://dev.clojure.org/jira/browse/CLJS-2633, is this supposed to work anymore w/o specifying :npm-deps ?

@dnolen Yes. I'm using 1.10.126 without :npm-deps, just package.json and node_modules

hrm k

seems something wrong with the js-dependency-index - some of the processed files don’t even show up in cljs_deps.js

oh …

@juhoteperi does Google Closure support scoped requires?

@material thing? Not sure

Issue says this would be regression

strange because report seems to imply it worked

But yeah, would be my guess this has something to do with that

but I don’t know how if Closure doesn’t

does Closure support requires like react/server? because scoped requires are the same thing

For some reason Closure CompilerInput .getRequires returns those "strange" names for snackbar/index.js requires

@sundarj but presence of @ char if not handled is enough to break things

hrm I don’t see how this ever worked, unless Closure supported it before and dropped it somehow

oh hrm @ should not a problem because that’s in the folder name?

yeah, that's what i meant

I think all the files are correctly processed etc. Closure just provides us with bad dependency information for some reason

Previously Closure added the goog.require calls itself, now we add those ourselves based on Closure information, which is not necessarily the same information as used by Closure previously

@juhoteperi so you’re saying result of getRequires for some of the requires are bad?

specifically @foo/bar

Yes

I wonder if this is specific to ES6 modules that use that?

@juhoteperi https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/deps/ModuleNames.java#L63

hrm

if they strip @ they will never find the module during resolution

of course we pass everything so everything gets processed

hrm but git blame for this stuff doesn’t seem recent enough

Not sure if @ causes problems in some later stage, but it shouldn't break .getRequires

Looks to me that @ should work, cljs_deps.js and everything looks correct for the modules that get included

It's just that snackbar/index requires module$$material$base$index instead of module$home$juho$tmp$string_requires_npm_deps$node_modules$$material$snackbar$node_modules$$material$base$index which would exist

I don't have more time to look into this now, but the easiest place to debug this is: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/closure.clj#L1761 and debug-prn or something module-name and (vec (.getRequires input))

@juhoteperi yeah was about to fire up IntelliJ debugger and start stepping

@juhoteperi thanks for the info so far, still helpful

The DependecyInfo is populated during either of these .process calls: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/closure.clj#L1825-L1826

I did quick check with closure-compiler master, and that didn't change this

k thanks, I think I have enough info to spelunk for a bit

sorry to report a degradation of browser-repl behavior in terms of a connection surviving a browser refresh

That was never guaranteed to work

just reporting a degradation of behavior

We don’t have reconnect logic, never did

it worked before fairly well

and it was nice to have

By accident far as I can tell

Not going to work on that for now

I expected so

Restart is fast enough

also the REPL is only supposed to work for optimizations none correct?

No work has been for higher modes

Also accidental if it ever worked

no it doesn't work

well it connects for whitespace

after errors and stuff

Yeah need focus on the fundamentals

Nice stuff is for contributors :)

yeah was just verifying, as I'm playing with it

also clj -m cljs.main -O simple -c example.core emits WARNING: :preloads should only be specified with :none optimizations and :browser-repl true in the cached opts

with the same behavior for whitespace

also the REPL works fantasticly well in "simple"

I'm doing these explorations to inform the design of figwheel so that it will work in simple and whitespace

yeah, it’s definitely possible just not priority

I know a few people find that useful, but many people won’t

yeah just trying to put some pressure on my core lib so that its general enough

but its freaking fantastic that I can try all these scenarios out so easily

@bhauman oh cool so you’re just running through all the different options via cljs.main

yeah it’s pretty awesome to just blast through stuff

yeah I'm wondering how goog base behaves under simple

well its super easy to check

and under whitespace

COMPILED is the goog define it checks for

they don’t care about the level

which is good

yep I was wondering if certain things got skipped in those modes that affect reload and such, but again easy to check out now

like does the dependency tree get built?

no

yeah thats what I though

because you don’t need it when compiled

yeah thats what I suspected

so while I think it’s neat the REPLs could work other under compilation modes

this is not a problem at all just an investigation

I don’t know that it’s worth it

totally get that, and not asking for changes here

just seeing if this design pressure produces "thoughts"

for figwheel

@juhoteperi https://github.com/swannodette/closure-compiler/commit/58012d3f1068aa588a47dc34ec6f39413aa59e62, this fixes it for me

will have to wait for next release though of course