Fork me on GitHub
#cljs-dev
<
2018-03-11
>
jannis01:03:55

(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.)

jannis01:03:37

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

jannis01:03:15

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

mfikes03:03:40

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

rauh10:03:40

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

rauh10:03:32

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)))
Where (MD/ensure-static-fns! true) is just a macro mutating the compiler (set! ana/*cljs-static-fns* x)

rauh10:03:26

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

rauh10:03:02

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)

rauh10:03:19

And you'll see how the stackoverflow happens

rauh10:03:36

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.

mfikes14:03:37

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

darwin15:03:32

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

darwin16:03:51

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.

dnolen16:03:19

@darwin it already works that way

dnolen16:03:11

my stance on this remains unchanged

dnolen16:03:19

if your library breaks caching - it’s broken

dnolen16:03:46

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

dnolen16:03:24

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

darwin16:03:26

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

dnolen16:03:52

you keep talking about :external-config

dnolen16:03:07

this doesn’t exist

dnolen16:03:46

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

dnolen16:03:39

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

dnolen16:03:52

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

dnolen16:03:41

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

darwin16:03:26

ok, thanks, I will look how it works there

dnolen16:03:16

the structure of the cache is right now

dnolen16:03:34

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

darwin16:03:16

@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

rauh16:03:30

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

darwin16:03:31

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

dnolen17:03:31

@darwin cljs-oops is just broken then

dnolen17:03:45

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

dnolen17:03:50

then nothing will ever cache

dnolen17:03:42

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

dnolen17:03:08

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

dnolen17:03:19

allowing libs to muck with it isn’t solving anything

dnolen17:03:03

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

dnolen17:03:10

people will eventually learn to avoid such tooling

dnolen17:03:29

as they decide to adopt :aot-cache true

dnolen17:03:55

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

dnolen17:03:36

@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

rauh17:03:30

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

dnolen17:03:05

adding test and then will push to master

dnolen18:03:17

@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

darwin18:03:53

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

dnolen18:03:12

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

dnolen18:03:59

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

rauh18:03:35

@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

dnolen18:03:26

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

rauh18:03:10

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

dnolen18:03:29

@rauh I don’t understand your example from above

dnolen18:03:50

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

dnolen18:03:57

and open an issue

rauh18:03:59

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

dnolen18:03:15

@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

dnolen18:03:26

I’ve spent enough time on this 🙂

rauh18:03:51

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

bhauman18:03:58

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

dnolen18:03:35

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

rauh18:03:39

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

bhauman18:03:43

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

dnolen18:03:01

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

dnolen18:03:07

code splitting has mains

dnolen18:03:17

and compiling without :main is a legacy thing

bhauman18:03:18

the output-to's are mains:

bhauman18:03:28

yeah but I encounter these things

bhauman18:03:45

I encounter folks who use don't use main

dnolen18:03:50

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

dnolen18:03:02

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

dnolen18:03:15

so you have to do Closure defines by hand too

dnolen18:03:22

there’s no issue here

bhauman18:03:22

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

dnolen18:03:38

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

dnolen18:03:55

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

bhauman18:03:12

OK figwheel currently works without a main

dnolen18:03:22

but that’s a Figwheel thing

dnolen18:03:25

not a ClojureScript thing

bhauman18:03:28

so your saying that figwheel will need to write the defines

bhauman18:03:14

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

dnolen18:03:33

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

dnolen18:03:38

this is how Google Closure works

bhauman18:03:29

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

bhauman18:03:56

but a bug killed it immediately

dnolen18:03:57

@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

bhauman18:03:18

I'lll have to go see what that was

dnolen18:03:32

I am very skeptical about claims about Closure defines

dnolen18:03:47

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

dnolen18:03:35

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

dnolen18:03:47

I never thought any of the macro things made much sense

dnolen18:03:51

other than it seemed “easy”

bhauman18:03:46

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

dnolen18:03:15

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

dnolen18:03:21

@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

rauh18:03:27

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

dnolen18:03:39

Closure: The Definitive Guide - get it 🙂

dnolen18:03:55

@rauh this doesn’t seem important 🙂

mfikes18:03:43

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

mfikes18:03:22

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

dnolen18:03:01

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

dnolen18:03:20

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

mfikes18:03:48

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

darwin18:03:20

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

dnolen18:03:22

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

dnolen18:03:32

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

mfikes18:03:12

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

dnolen18:03:57

@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

dnolen18:03:51

@mfikes ok lowering priority for now

mfikes18:03:02

Sounds good.

dnolen18:03:13

@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

dnolen18:03:32

anyways that’s what this is about - advanced compilation

dnolen18:03:56

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

dnolen18:03:10

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

dnolen18:03:49

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

rauh18:03:25

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

dnolen18:03:34

yeah no benefit

dnolen18:03:57

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

rauh18:03:12

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.

rauh18:03:55

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

dnolen18:03:08

yes I already thought about these options 🙂

dnolen18:03:41

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

rauh18:03:03

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

rauh18:03:38

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

dnolen18:03:45

exactly 😉

dnolen18:03:46

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

dnolen19:03:16

definitely personally benefiting from cljs.main just sorting through bugs

dnolen19:03:19

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

dnolen19:03:35

is how I worked through the spec one

mfikes19:03:45

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

dnolen19:03:08

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

juhoteperi19:03:22

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

dnolen19:03:26

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

dnolen19:03:39

@juhoteperi does Google Closure support scoped requires?

juhoteperi19:03:52

@material thing? Not sure

juhoteperi19:03:11

Issue says this would be regression

dnolen19:03:12

strange because report seems to imply it worked

juhoteperi19:03:41

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

dnolen19:03:43

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

sundarj19:03:39

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

juhoteperi19:03:31

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

dnolen19:03:19

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

dnolen19:03:45

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

dnolen19:03:42

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

sundarj19:03:05

yeah, that's what i meant

juhoteperi19:03:57

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

juhoteperi19:03:59

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

dnolen19:03:27

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

dnolen19:03:36

specifically @foo/bar

dnolen19:03:22

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

dnolen20:03:24

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

dnolen20:03:34

of course we pass everything so everything gets processed

dnolen20:03:14

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

juhoteperi20:03:59

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

juhoteperi20:03:08

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

juhoteperi20:03:16

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

juhoteperi20:03:42

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

dnolen20:03:13

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

dnolen20:03:27

@juhoteperi thanks for the info so far, still helpful

juhoteperi20:03:50

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

dnolen20:03:37

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

bhauman20:03:38

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

dnolen20:03:10

That was never guaranteed to work

bhauman20:03:31

just reporting a degradation of behavior

dnolen20:03:34

We don’t have reconnect logic, never did

bhauman20:03:01

it worked before fairly well

bhauman20:03:11

and it was nice to have

dnolen20:03:19

By accident far as I can tell

dnolen20:03:35

Not going to work on that for now

bhauman20:03:43

I expected so

dnolen20:03:46

Restart is fast enough

bhauman20:03:30

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

dnolen20:03:01

No work has been for higher modes

dnolen20:03:01

Also accidental if it ever worked

bhauman20:03:25

no it doesn't work

bhauman20:03:36

well it connects for whitespace

bhauman20:03:43

after errors and stuff

dnolen20:03:03

Yeah need focus on the fundamentals

dnolen20:03:32

Nice stuff is for contributors :)

bhauman20:03:39

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

bhauman20:03:35

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

bhauman20:03:47

with the same behavior for whitespace

bhauman20:03:26

also the REPL works fantasticly well in "simple"

bhauman20:03:36

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

dnolen20:03:06

yeah, it’s definitely possible just not priority

dnolen20:03:26

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

bhauman21:03:03

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

bhauman21:03:45

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

dnolen21:03:37

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

dnolen21:03:46

yeah it’s pretty awesome to just blast through stuff

bhauman21:03:21

yeah I'm wondering how goog base behaves under simple

bhauman21:03:28

well its super easy to check

bhauman21:03:39

and under whitespace

dnolen21:03:39

COMPILED is the goog define it checks for

dnolen21:03:03

they don’t care about the level

dnolen21:03:08

which is good

bhauman21:03:12

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

bhauman21:03:57

like does the dependency tree get built?

bhauman21:03:23

yeah thats what I though

dnolen21:03:24

because you don’t need it when compiled

bhauman21:03:03

yeah thats what I suspected

dnolen21:03:05

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

bhauman21:03:14

this is not a problem at all just an investigation

dnolen21:03:15

I don’t know that it’s worth it

bhauman21:03:37

totally get that, and not asking for changes here

bhauman21:03:16

just seeing if this design pressure produces "thoughts"

bhauman21:03:50

for figwheel

dnolen23:03:34

will have to wait for next release though of course