Fork me on GitHub
#cljs-dev
<
2018-03-14
>
danielcompton00:03:31

Is there a ticket to follow for this?

mfikes00:03:02

@juhoteperi’s suggestion to add newlines would work out nicely for self-hosted ClojureScript in that it would "just work"

dnolen08:03:51

for what it’s worth I’m skeptical there will be significant benefit

dnolen08:03:19

until the debugger stops being line based, it’s not going to be significant enhancement

dnolen08:03:26

ClojureScript is not line based

dnolen08:03:56

but if someone wants to try and experiment and show there’s is a significant experience delta - that’s great

mhuebert10:03:29

After several months of seeing these in every maria build, I’m wondering if we can do something about these warnings:

------ WARNING #1 --------------------------------------------------------------
 File: cljs/spec/test/alpha$macros.cljc:113:35
--------------------------------------------------------------------------------
 110 |   ([xs]
 111 |    `(instrument ~xs nil))
 112 |   ([sym-or-syms opts]
 113 |    (let [syms (sym-or-syms->syms (eval sym-or-syms))
-----------------------------------------^--------------------------------------
 Use of undeclared Var cljs.spec.test.alpha$macros/eval
--------------------------------------------------------------------------------
 114 |          opts-sym (gensym "opts")]
 115 |      `(let [~opts-sym ~opts]
 116 |         (reduce
 117 |           (fn [ret# [_# f#]]
--------------------------------------------------------------------------------

according to this discussion https://clojurians-log.clojureverse.org/lumo/2017-08-02.html, this is expected because we set! eval from somewhere else. would it work to just (declare eval) in that namespace?

dnolen10:03:52

hrm probably not a good idea - we could declare eval which does nothing in cljs.core for this case

dnolen11:03:43

@kommen try master, your test.edn thing takes ~78ms to validate even on a slow machine like mine

kommen11:03:59

@dnolen confirmed master fixes the issue

dnolen11:03:10

thanks for the confirm

kommen11:03:48

well, I guess the tools.deps migration with the ability to use git deps is paying off already

mfikes11:03:11

I wonder, for the eval problem: If the compiler is compiling all of the source in a directory, if it could skip any .cljc file for which there is a .cljs file. This would be consistent with the rule for require where it must load a .cljs file in preference to a .cljc file. (Which differs from the rule for require-macros.)

mfikes11:03:04

I suppose that is fairly close; it misses the possibility that there might be a .cljs file somewhere else in the classpath.

mfikes11:03:58

I could try an experimental patch to see if it resolves this without breaking anything. Unless we know it won’t work for some reason.

dnolen11:03:04

I dunno, I think in this case the fact that we use eval and it’s missing seems like a problem for that ns

dnolen11:03:32

in fact maybe eval should call *eval* should be a thing which throws if not bound to something

dnolen11:03:53

one problem is that bootstrapped eval interface is async

dnolen11:03:25

but maybe for portable usages of eval not a big deal

dnolen11:03:40

since you could force bootstrap eval to be synchronous and make it work?

mfikes11:03:20

Yeah, that’s how Planck and Lumo skate by, they have defined a synchronous eval, and monkey patch that namespace so that eval is defined in there.

dnolen11:03:49

ok, so this is just in line with people have already done too

dnolen11:03:56

patch welcome to do what I described above

mfikes11:03:12

OK, sounds good

mfikes11:03:37

Cool. I’ve often wondered if compiling that .cljc file as ClojureScript might accidentally overwrite the output that you would get for .cljs. Perhaps we have been getting lucky all along in that it gets compiled first. Dunno. Anyway, https://dev.clojure.org/jira/browse/CLJS-2660 ^

dnolen11:03:05

@mfikes I left a comment to clarify

dnolen11:03:20

@mhuebert I think ^ will solve your issue

mfikes12:03:39

@dnolen Saw your note. Cool. I was also pondering promoting this version of eval from the test namespace into the cljs.js namespace as the overridable default for that environment. https://github.com/clojure/clojurescript/blob/master/src/test/self/self_parity/test.cljs#L293 This might be too much change in public API, but just pondering that it might simplify things.

mfikes12:03:16

As usual, willing to try an exploratory patch for consideration. No worries if rejected after seeing what it looks like.

dnolen12:03:01

@mfikes default should throw. But cljs.js could set! to this test thing we have as you’re suggesting

dnolen12:03:46

No issue here with API change

dnolen12:03:13

since we never declared eval before

mfikes13:03:17

Hah. That's just cool:

$ planck -q
cljs.user=> (eval '(+ 1 2))
3

dnolen14:03:22

I guess benefit of stalling on some things for a couple years - it’s much clearer that something is a good idea or not 🙂

dnolen14:03:07

that is pretty cool, and I guess it means that cljsjs.js users have a pretty easy way to try things now

mfikes14:03:21

Yeah. I’m adding lots of tests to ensure things work out. Some wild runtime things are evaluated in this test namespace https://github.com/mfikes/clojurescript/blob/bf264dc790e077b4d330eeae3045e6477f84f8db/src/test/cljs/cljs/eval_test.cljs

dnolen14:03:44

@mfikes before I forget - a big new thing in release is :stable-names

dnolen14:03:10

which ensures less name churn between advanced builds thus proper vendorization if you’re using :modules

mfikes14:03:10

OK, will add to the main news post if it makes sense to have a paragraph there

dnolen14:03:23

hrm should probably default that to true if you’re using :modules

juhoteperi15:03:52

@dnolen If you are merging patches, this is simple fix: https://dev.clojure.org/jira/browse/CLJS-2658

dnolen15:03:28

it looks like Closure Compiler folks are OK with my patch, just want some test cases

dnolen15:03:57

will try to tackle that on Friday and hopefully that can make it into the next release

dnolen15:03:18

(Closure release)

dnolen15:03:38

@juhoteperi re: React, you’re talking about the latest React right? Seems like minor tweaks though?

thheller16:03:14

not sure if this work for rewritten code though

juhoteperi16:03:00

I think that wouldn't work as React CJS bundles are already minified: https://unpkg.com/[email protected]/cjs/react-dom-server.node.production.min.js

thheller16:03:44

ah right. thought you were processing sources directly for a moment.

juhoteperi16:03:25

React 16 node packages don't contain any other but CJS bundles 😕

juhoteperi15:03:04

It only affects React-dom/server + optimizations

dnolen15:03:11

I don’t know why they don’t just have that GCC build be a part of CI

juhoteperi15:03:14

i.e. Reagent test suite with optimizations

dnolen15:03:22

then you can’t merge in stuff that’s gonna break GCC build

juhoteperi15:03:05

They have two open PRs to build bundles using GCC so maybe they'll do that eventually

dnolen15:03:34

well I’m getting more excited about :npm-deps - it seems like more & more stuff is working

john16:03:15

made a little thing to kick the tires on Java-WebSocket: https://gist.github.com/johnmn3/a845f2bf67ad877d957bccd7f9cefe8c

john16:03:32

Yeah shared tire kicking is super simple with these new tools!

mfikes16:03:39

@mhuebert There is now a patch in https://dev.clojure.org/jira/browse/CLJS-2660 If you end up with some free time at some point, it would be nice to know if it causes the eval warning to go away for you.

mfikes18:03:25

Thanks for the confirm!

mhuebert18:03:06

thanks for the patch! and the instructions for deps, that was really simple to test. it threw the expected warnings for the eval function I had defined in my own namespace -- with this patch applied I could change that now to (set! *eval* ...) or exclude eval in the ns declaration.

mfikes18:03:04

Cool. I'm not seeing the big picture. Is maria self hosted?

mfikes18:03:25

The patch does a (set! *eval* ...) in the cljs.js namespace so you end up with a functioning cljs.core/eval in the self-hosted case

john18:03:36

Long running operations hang the browser, so I think so. I think I'm falling in love with Maria, btw 🙂

mfikes18:03:23

Oh, I just looked at the code @mhuebert. You may be able to ultimately delete Maria's eval and just use cljs.core/eval, given that cljs.js sets up one for you. (Depending on what Maria is really doing in its eval.)

mfikes18:03:24

In other words @mhuebert the scope of CLJS-2600 was expanded a little so that by default if you are using self-hosted ClojureScript, you get a functioning eval.

mhuebert18:03:20

that’s great

mhuebert18:03:54

our eval is indeed wrapped with a bit of extra stuff. we log intermediate steps, like the compiled-js, which lets us trace a user function back to the cell & time it was produced, and do some extra resolving of position info in the case of errors

mfikes18:03:29

OK @mhuebert since cljs.js does a set!, a good test would be to, perhaps right after Maria's eval is defined, to do a set! there to ensure that Maria's is the one that ends up being used within the app.

mhuebert18:03:31

I haven’t followed all the latest cljs.js developments, had a lot of work the last few months but am getting back into it now

mfikes18:03:27

This should work, as maria.eval requires cljs.js and it can then mutate a little further with its own set!

mhuebert18:03:55

ok. I’ll have to look at this, our eval functions weren’t really designed for user-space but rather to evaluate our code cells, hence the logging and what not. If users want to call eval it probably makes sense to let them use the most “normal” version

mhuebert18:03:15

so that the experience is as close to regular clojure as possible

mhuebert16:03:30

@mfikes wow that was fast! I will try it

mhuebert16:03:44

do I recall correctly that you have mentioned somewhere an easy way of testing patches like this with deps?

mfikes16:03:54

Yes, @mhuebert I have the patch in a branch. Just a sec and I’ll give you the right coordinates.

mfikes16:03:54

@mhuebert This has CLJS-2660 applied in it:

{:deps {org.clojure/clojurescript {:git/url "" :sha "ec281b735b5c3e1a80f1cc46f8996aba45d13987"}}}
and is also based of a recent master (1.10)

dnolen16:03:19

@john that looks pretty nice

mfikes16:03:59

We just need to convince tools.deps.alpha to add a :with-patches [""] and we’ll be in a whole different place 🙂

richiardiandrea17:03:41

that's an awesome idea Mike

mfikes17:03:05

It would serve a niche use case 🙂

richiardiandrea17:03:32

well...the other side is clj --create-jira-ticket 😄

john17:03:40

@dnolen yeah, a fun little addition would be to make it a cljc file and allow the client side to launch with cljs.main 🙂

john17:03:11

I assume the http connection would "upgrade" to ws somewhere around here: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/repl/server.clj#L202

john17:03:11

But a lot of the subsequent logic appears to depend on HTTP semantics

john17:03:19

So, perhaps that's not the right place...

dnolen17:03:05

@john actually I don’t necessarily see a reason to do that, if we support websockets I think it should be fully decoupled from the webserver

dnolen17:03:15

Clojure programmers use lots of different webservers

dnolen17:03:54

I think most of work here is going to be making sure it’s easy to integrate with Ring, Pedestal or whatever

john18:03:56

k, I'll investigate this evening

mfikes18:03:57

:stable-names added to a section in http://_1.10.xxx Release_ and as a compiler option PR https://gist.github.com/mfikes/bdbe214f03abac88ae384adb1ac26490

mfikes19:03:51

If we have a cljs.core/eval sitting there, someone will ask whether it works in regular ClojureScript. Of course it doesn’t but you can trick yourself into making one that evidently works in a REPL if you require cljs.js: https://gist.github.com/mfikes/8fcff361937d5a2880aa2520d44233d0

bhauman20:03:35

I keep getting what appears to be spurious analysis errors clojure.lang.Keyword cannot be cast to clojure.lang.IAtom

bhauman20:03:40

I'm in a cljc file and its the clojurescript compiler giving me these errors on 1.10.145

bhauman20:03:55

hmm let me bump that

bhauman20:03:33

oh darn super subtle, not CLJS as far as I can tell at this moment

bhauman20:03:08

horribly missleading error handling in figwheel led me astray

danielcompton20:03:39

To summarise the discussion about AOT cache, closure defines, and macros: Doing any kind of environment based compilation is unsafe with the new AOT cache, instead you should closure-defines which will safely invalidate/use the correct cache

danielcompton20:03:48

What about :external-config, is that still safe to use?

danielcompton20:03:21

And will different closure-defines for a single JAR (say dev vs prod) invalidate a cache, or add a second cache entry for each?

thheller20:03:58

@danielcompton closure defines do not affect the compilation of a single .cljs file, so it doesn't affect cache in any way

danielcompton20:03:30

Ah, I was thinking about advanced compilation where the defines are substituted, but that happens as a later stage right?

danielcompton20:03:59

What about :external-config?

thheller21:03:14

no idea, not a thing in CLJS. probably breaks the cache if used by macros in any way

darwin21:03:42

@danielcompton :external-config is not safe to use, search for prior discussions here

thheller21:03:07

side effects in macros pretty much always break caching

danielcompton21:03:30

Ok, I assumed it was a native ClojureScript feature, but it's not?

darwin21:03:07

no, it is not, we were abusing the fact, that compiler options is an open map, and we can access it in macros

darwin21:03:26

AFAIK there is no way how to safely use user-specified configuration in macros with enabled :aot-cache

danielcompton21:03:32

Yeah, I'm working on this with https://github.com/Day8/re-frame-debux/commit/144c9236969294c847fe7e3442a6d9b890f2f807, and I'm relying on Closure DCE to remove the code in advanced compilation

danielcompton21:03:47

and using closure-defines for the config

darwin21:03:37

LGTM, good luck and let us know how it went - I’m still a bit skeptical. DCE is a complex beast and does not work when you cross some complexity threshold, also it is a moving target and each new closure compiler version could behave differently in this regard.

darwin21:03:19

Maybe you should write some tests and make sure closure defines configured DCE elided code you expected.

danielcompton21:03:50

Yeah I'm going to write some tests on this. I found I had to hide the Closure-define behind a boolean typed function for the DCE to work. I have found it eliminates the traced function, I need to do some more work on removing it entirely form the app (will have a separate JAR dependency which isn't in the prod build to ensure this)

darwin21:03:57

instead of a separate JAR I would also consider using modules and loading optional code dynamically behind the :closure-define flag

darwin21:03:11

not sure if this is a viable solution, never tried this myself

thheller21:03:53

@danielcompton if you are on the 1.10.x versions you shouldn't need any of the type-hinting ceremony around the DCE goog-define. https://dev.clojure.org/jira/browse/CLJS-1439 now automatically type hints as boolean.

danielcompton21:03:12

Excellent, was just thinking about opening a ticket for this when I was going through all of that work

thheller21:03:08

hmm why is the ticket still open. I was sure it was merged. let me check.

thheller21:03:53

ok, it was merged. ticket just not updated.