Fork me on GitHub
#cljs-dev
<
2018-03-23
>
mfikes00:03:10

Wow, an exploratory change for ^ shows a 5x speedup, and the amount of extra code to achieve that is not too scary. (I've attached the exploratory patch, just to show this.)

mfikes04:03:01

ClojureScript is painfully slow on the Nashorn that ships with Java 10.

mfikes04:03:16

I wonder what changed.

mfikes04:03:48

Good news is that tests on master pass with Java 10.

thheller07:03:46

@mfikes you recently did a pass over the :closure-warnings which now breaks if the user pins a specific closure-compiler version.

thheller07:03:30

I wonder if we can maybe create a solution that either picks these out via reflection or gracefully skips things it can't find

thheller07:03:22

there is a bug in the latest closure release which means some users wanted to pin to a previous release

thheller07:03:10

ideally this shouldn't pretty much breaks with every closure release and always has to be updated manually

dnolen08:03:18

@theller @mfikes or we can revert for now

dnolen08:03:27

They didn’t seem that important

rauh08:03:09

There already is a public DiagnosticGroup/forName, just need to camelCase the input. There is also CompilerOptions/fromString.

dnolen08:03:53

That works too

rauh08:03:49

There is also DiagnosticGroup/getRegisteredGroups which can be used to error out to the user if he/she selects a group that doesn't exists. "Cannot find DiagnosticGroup XYZ, available: ...`

thheller09:03:20

weird

public Map<String, DiagnosticGroup> getRegisteredGroups() {
    return ImmutableMap.copyOf(groupsByName);
  }

thheller09:03:34

its a non-static fn accessing a static field

thheller09:03:03

but yeah that seems like a good idea just resolving it from the keyword

thheller09:03:13

I'll see about creating a ticket/patch for this

thheller09:03:13

public static final DiagnosticGroup CHECK_USELESS_CODE =
      DiagnosticGroups.registerGroup("uselessCode",
          CheckSideEffects.USELESS_CODE_ERROR,
          CheckUnreachableCode.UNREACHABLE_CODE);

thheller09:03:55

:check-useless-code is the current keyword for this but its registered under uselessCode

thheller09:03:14

with automatic lookup this would break if someone is actually using this

thheller09:03:45

it is not the only one

dnolen09:03:59

I think something simple based on what we have is fine

dnolen09:03:53

DiagnosticGroup/forName + checking that the keyword the user supplied is in there

thheller09:03:41

so you'd be ok if users have "invalid keywords" from previous versions and just warn about those not existing?

thheller09:03:09

if the user currently has :check-useless-code we can't look that up via forName because its registered under a different name

thheller09:03:45

I doubt that many people are doing this so it should be fine

rauh09:03:17

@thheller There is also cljs.util/suggestion if you want to get fancy

mfikes11:03:14

There could also be changes in the ClojureScript compiler that depend on the latest Closure release, outside of the set of warnings. Some of that stuff could also be conditional / reflective, I suppose. Apart from users wanting to pin to a specific older version, simply being able to flip back to an older version can help isolate regressions that might crop up. (The scenario where you think it is the latest Closure release breaking something, and you want to try an older one to see if it still works with that.)

mfikes11:03:17

I guess now we get into this position where ClosureScript strongly depends on the very latest if not careful.

thheller11:03:10

it just replaces the static warning-types map and generates it dynamically

thheller11:03:26

ugh .. it already breaks some reference in cljs itself 😞

juhoteperi11:03:37

@mfikes It would be very hard to support multiple Clusure versions, module stuff changed A LOT during last few releases

thheller11:03:40

hmm true ... maybe it just feasible to support multiple closure versions

juhoteperi11:03:42

For some parts, like warnings etc. it is probably easy, but having working module processing with e.g. both v20180204 and v20170910 would require lots of checks

juhoteperi11:03:55

Maybe for next Closure releases we can implement changes in a way that works also with current, the changes are hopefully smaller in next releases

mfikes11:03:42

Right. That's the point, really. Addressing the warnings keywords issue is insufficient. It might be prudent to at least have the ability to to back to the previous version that ClojureScript dependent on, but even then, I wonder if that leads to a convoluted set of checks. We do this with Java, for example (see here https://github.com/clojure/clojurescript/blob/8670cc40cf09f90f1d36cb4bf0a2c68713fb1d68/src/main/clojure/cljs/js_deps.cljc#L19-L37), but Java is much more conservative that Closure. Hrm.

juhoteperi11:03:36

If the changes are small in future, we can think about this, but the previous update was so complex that supporting old version wasn't in my opinion sane. And some of the regressions are due to changes in our side.

mfikes11:03:45

If you are going to take "one small step", do it with one foot. πŸ™‚

mfikes11:03:36

Yeah, I wonder what other consumers of Closure do? This must be painful. The same sort-of happens with React, right? (But not as badly.)

juhoteperi11:03:12

Are there many other Closure consumers? Besides obviously Google itself.

juhoteperi11:03:35

If I recall correctly, Scala JS projects use Webpack or something.

juhoteperi11:03:59

My guess is that no-one else is integrating with Closure on same level as we do.

mfikes11:03:00

Perhaps the teams in Google, themselves don't have other dependencies that have dependencies on Closure. (Perhaps it is only at the "leafs" in Google.)

mfikes11:03:37

Otherwise, I would imagine a lot of strife within Google, and complaints about change.

juhoteperi11:03:10

Google has much easier time as they probably only use "simple API", like single build call or something.

juhoteperi11:03:50

Due to our special node module handling, we use quite many Closure classes directly.

mfikes11:03:06

Ahh. This raises the idea, if ClosureScript is so heavily dependent, if we had our own fork to act as a buffer.

juhoteperi11:03:54

That would allow getting fixes in quicker also. But it would be more work for someone to keep the fork up-to-date, especially if we try to keep compatibility.

mfikes11:03:23

Yeah, perhaps a less aggressive idea is to split off the bits in ClojureScript that touch Closure directly into another lib, and keep the API to that new lib simple. Then if you want to revert to an older version, you just revert both that lib and Closure.

mfikes11:03:46

I bet keeping the API to such a lib "simple" would also be lots of work.

juhoteperi11:03:49

And the previous Closure update affected analyzer/compiler due to changes to how we need to emit Cljs code which will call module-processed JS.

mfikes11:03:59

Yeah, ClosureScript is deeply intwined with Closure.

juhoteperi11:03:19

No API wrapper will help with change where Closure changed how it converts CJS modules.

mfikes11:03:49

All problems in computer science are solvable by another level of indirection, unless you are trying to indirect from yourself.

mfikes13:03:13

I ran master through 1.10.x Quick Start on Windows Source mapping not working there https://dev.clojure.org/jira/browse/CLJS-2695 And using optimizations can still be flakey https://dev.clojure.org/jira/browse/CLJS-2401

dnolen13:03:33

Probably will just punt on those for now.

mfikes13:03:10

Right, just reporting mostly for completeness.

dnolen13:03:37

re: Closure Compiler/Library this problem has been true for some time, I don’t think we should spend lots of effort in it. Best way to prevent breakage is to get our own tests in

dnolen13:03:00

My impression is that’s perfectly acceptable by the Closure Compiler devs

mfikes13:03:32

Oh, we can contribute tests to them?

dnolen13:03:22

Yes I have a fix for module processing for node scoped modules. It needs tests though.

dnolen13:03:41

This tests a path unused by Closure users except ourselves

dnolen13:03:20

Adding simple tests for defects that affect us is a good way to prevent breakage

dnolen13:03:42

Doesn’t apply in all cases of course but that’s an improvement

mfikes13:03:13

Definitely. Arguably we are only using public API, but perhaps exercising subtle corner cases in Closure’s code paths.

dnolen13:03:33

Well also some of the stuff was half baked and not much used

dnolen13:03:46

So I expect churn and then things will settle down

mfikes14:03:41

Thinking out loud: I’m wondering if we can construct our own CI that always builds with the latest from the Closure tree. Then we would also have another way to catch things earlier.

dnolen14:03:02

also not a bad idea

darwin14:03:48

I think we could setup clojurescript’s own tests to run in canary jobs as well (maybe as part of compiler build), and as a next step maybe configure it to use closure master

mfikes14:03:41

I don't know the details of how the unshaded JAR gets created (I just tried the latest Closure release, but changing the version numbers was insufficient.) Yeah, maybe a script/test-closure-latest or somesuch could do all the needed actions to test things that way.

mfikes14:03:30

(Like pulling, building, setting all the version numbers in the deps files.) With that, it would be trivial to add a Travis call to such a script.

Garrett Hopper16:03:42

I haven't quite been able to consistently generate this error, but I'm fairly sure it has something to do with my use of macros in a node cljs repl:

cljs.repl/repl*                repl.cljc:  930
cljs.repl/evaluate-form                repl.cljc:  553
boot.user.Delegatingcljs_repl_node_NodeEnv._evaluate           NO_SOURCE_FILE:  122
cljs.repl.node.NodeEnv/-evaluate                 node.clj:  234
cljs.repl.node/node-eval                 node.clj:   66
java.lang.NullPointerException:  {"type":"result","repl":"nREPL-worker-1","status":"success","value":"true"}{"type":"result","repl":"nREPL-worker-2","status":"success","value":"true"}
I'm not familiar enough with the node repl to understand what it's doing here https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/repl/node.clj#L66 Does anyone have any idea what the issue might me? I'll continue trying to find consistent reproduction steps.

Garrett Hopper16:03:37

It's fairly intermitent, and Cider doesn't catch it at all. I have to kill the repl and restart is when this happens. Is it possibly an issue with Cider's eval-buffer command? I've noticed that I sometimes have to manually evaluate the ns declaration before I can evaluate the buffer.

mfikes17:03:12

Quick Start works perfectly on macOS with master currently

mfikes18:03:29

Quick Start also works perfectly with Java 10 on macOS

mfikes18:03:35

Oh, perhaps we already have a way to test with the latest Closure

lein with-profile +closure-snapshot test

mfikes18:03:26

This is great:

actual: java.lang.NoSuchMethodError: com.google.javascript.jscomp.Es6RewriteModules.<init>(Lcom/google/javascript/jscomp/AbstractCompiler;)V

mfikes18:03:40

That's the kind of regression that is nice to catch. πŸ™‚

juhoteperi18:03:27

Hmm, maybe I should send patch which fixes this Es6RewriteModules in a way that works with current & next version?

juhoteperi18:03:52

I have already the fix somewhere and it should be possible to implement this in a way that works for both

mfikes18:03:48

@juhoteperi Yeah, perhaps that is a good test of straddling two versions

mfikes18:03:25

Perhaps tools.deps doesn't see upstream aliases. A :closure-snapshot alias works if you are in the ClosureScript tree, but not if you are elsewhere pointing at that tree: https://gist.github.com/mfikes/84a83abec3860ac86a3096a5b8a62a04#file-deps-edn-L16

Alex Miller (Clojure team)19:03:01

you have to resolve aliases to build the dependency tree, which you can’t do before you build aliases to build the dependency tree to build aliases to build the dependency tree, well you get the idea

richiardiandrea19:03:06

are the sources coming with 1.10.217 encoded somehow? I see cljs/core/specs/alpha.cljs does not contain any code?

mfikes19:03:44

@richiardiandrea Missing an s : cljs/core/specs/alpha.cljs

❓ 4
mfikes19:03:23

(The namespace is the specs for core.)

mfikes19:03:02

Does that make sense? (Put an s on the end of spec in your path.)

richiardiandrea19:03:23

Oh yeah I noticed that, I misscopied ... ok i might have something strange going on here

mfikes20:03:03

https://dev.clojure.org/jira/browse/CLJS-2698 would give us the ability to script/bootstrap with the latest snapshot of Closure Compiler (akin to lein with-profile +closure-snapshot test but then letting us run script/test to really use the entire thing to do the typical :advanced stuff script/test does)

mfikes20:03:00

script/bootstrap --closure-compiler-snapshot
and then script/test is another way to repro the Es6RewriteModules stuff

Garrett Hopper20:03:32

cljs.main with -t nodejs -r

Exception in thread "main" java.lang.AssertionError: Assert failed: :nodejs target with :none optimizations requires a :main entry
(not (and (= target :nodejs) (= optimizations :none) (not (contains? opts :main))))

Garrett Hopper20:03:42

Should this work?

richiardiandrea20:03:30

@ghopper you need -re node in addition to the IIRC

Garrett Hopper20:03:21

Thanks πŸ™‚

richiardiandrea20:03:29

the -t vs -re difference can be difficult to grasp but I think the latter is always necessary with -r...

mfikes20:03:54

I wonder if -t implies -re or if it is only the other way around

Garrett Hopper20:03:28

@mfikes It seems to work with just -re.

richiardiandrea20:03:42

the two are effectively decoupled, one defines the Repl Env, the other is a compiler option

richiardiandrea20:03:10

maybe because one might want a Nashorn REPL targeting node?

richiardiandrea20:03:22

is it even possible?

mfikes20:03:37

There is an implication between the two, I just can't recall the direction of the arrow πŸ™‚

πŸ‘ 4
richiardiandrea20:03:05

I would guess that -re implies -t according to what @ghopper just did

Garrett Hopper20:03:28

It certainly seems to.

mfikes20:03:10

Ahh, so if you -re node then the that REPL environment code will set the target https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/repl/node.clj#L209

richiardiandrea20:03:59

we could add a warning, when you have -r you might want to be notified when you miss -re

richiardiandrea20:03:20

ah actually no, that defaults to browser

mfikes20:03:51

Yeah, clj -m cljs.main -t node -r is an easy trap to fall into

πŸ‘ 4
mfikes20:03:18

In this case you can see that it is actually cljs.repl.browser, and then during the build stage it complains about the node target. https://gist.github.com/mfikes/e8e514d5c67ffb37a83f0cc0b35ea68d

mfikes20:03:10

Effectively the same as if you had explicitly done

clj -m cljs.main -t node -re browser -r

mfikes20:03:52

This is because even though -re node implies -t node, -re browser doesn't set a target

richiardiandrea20:03:14

we could detect it in cli instead of let it through

john20:03:40

if -re is websocket, you may want different targets?

mfikes20:03:40

Right, probably some way to check for sane combinations

mfikes20:03:55

There is also -t webworker πŸ™‚

mfikes20:03:09

Combinatorial

mfikes21:03:04

Perhaps these issues have always existed. There just so easy to explore now. πŸ™‚

πŸ‘ 4
john21:03:16

I'd vote to allow the traps for a while, to see how things compose

richiardiandrea21:03:05

agree, but probably release should have checks, I still make this mistake even now that I know about it πŸ˜„

richiardiandrea21:03:17

I mean "official" release

john21:03:42

probably not a bad idea

mfikes21:03:11

Then there are strange twists like this path AntΓ³nio sent me down with :target :nodejs but while actually doing React Native πŸ™‚ http://blog.fikesfarm.com/posts/2017-07-17-cleaner-clojurescript-react-native-interop.html

mfikes21:03:24

The mind bending aspect there is that there is the React Native packager running in Node.js while you are actually targeting JavaScriptCore as the JS engine.

richiardiandrea21:03:30

server side rendering is a thing actually

mfikes21:03:36

But debugging in V8 if you want

mfikes21:03:49

So you end up using 3 JS engines

πŸ˜‚ 4
john21:03:20

πŸ˜‚

john21:03:51

The websocket thing is pretty powerful. I just made a little chrome extension the uses it. I can inject a repl env into any running browser tab. This connects back to my ws repl server which is hanging off of a socket repl. Then, as each new connection connects to the ws repl, I can connect more and more socket repl clients and switch to the different clients connected to the single compiler env.

john21:03:27

So socket repl combined with websocket env... you can manage tons of simultaneous connections

john21:03:55

And I could certainly imagine some of envs having different -ts

richiardiandrea21:03:04

Sooooo each connection shares nothing?

john21:03:19

they share the same compiler

richiardiandrea21:03:56

Right they target the browser yeah going back to -t vs -re πŸ˜„ once again cool

john21:03:22

yeah, thus far all my experiments are in the browser. I'm not actually sure how I would go about composing heterogeneous targets in the same compiler env...

john21:03:39

But there's certainly a lot of power in the setup

πŸ‘ 4
john21:03:03

So what could cljs-dev use the most help with this weekend? Are we still gunning for a monday release?

john21:03:34

I guess I should look at the critical jira page first...

john21:03:18

Probably couldn't hurt to try out some of these here patches @mfikes put together πŸ™‚

r0man22:03:11

hey @john , I just saw you commented on this https://dev.clojure.org/jira/browse/CLJS-2679 . under which environment did you try this? browser, os?

john22:03:29

chrome, macos

r0man22:03:21

ok, thanks. it's really strange. it may be some platform specific problem or either my system is messed up πŸ˜•

john22:03:37

did you try it again against master?

r0man22:03:16

yes, I just tried again after I read your comment.

john22:03:43

from your description (edn/read-string "{\"repl\":\"main\",\"result\":\"{:status :success, :value \\\"true\\\"}\"}")

john22:03:03

Is that just how it got pasted into jira?

john22:03:25

oh I misread that

john22:03:57

But that's json going into edn

r0man22:03:04

That's the string I got when I printed the "ret" value from the browser-eval function

r0man22:03:39

yes the outer bit is json, and the value of "result" is edn

john22:03:11

So you're saying that wouldn't work

john22:03:38

I see what you're saying

r0man22:03:40

this is what happens in browser-eval

john22:03:47

But I'm not getting the exception πŸ˜•

r0man22:03:51

I just printed the value

r0man22:03:02

and tried it in the REPL again

r0man22:03:12

are you saying you can evaluate that statement above? πŸ˜‰

john22:03:14

Invalid token

john22:03:28

I don't get an exception when I follow these instructions:

john22:03:39

cd clojurescript
./script/uberjar
cd src/test/cljs_build/hello-modules 
java -cp ../../../../target/cljs.jar:src clojure.main repl.clj

Garrett Hopper23:03:35

I can't figure out why, but apparently the latest node repl times out after a few minutes when running under piggieback, but it works fine when running in a normal terminal. The error is own thrown after waiting a while and then trying to evaluate something. I've started trying to trace cljs.repl.node and cemerick.piggieback, but I haven't been able to make heads or tails of it yet. Has something changed in the structure of the node repl recently that would cause this?

nREPL server started on port 35919 on host 127.0.0.1 - 
REPL-y 0.3.7, nREPL 0.2.13
Clojure 1.9.0
OpenJDK 64-Bit Server VM 1.8.0_152-16
        Exit: Control+D or (exit) or (quit)
    Commands: (user/help)
        Docs: (doc function-name-here)
              (find-doc "part-of-name-here")
Find by Name: (find-name "part-of-name-here")
      Source: (source function-name-here)
     Javadoc: (javadoc java-object-or-class-here)
    Examples from : [clojuredocs or cdoc]
              (user/clojuredocs name-here)
              (user/clojuredocs "ns-here" "name-here")
To quit, type: :cljs/quit
nilcljs.user (+ 1 1)
{"type":"result","repl":"nREPL-worker-1","status":"success","value":"2"}
java.lang.NullPointerException
        at cljs.repl.node$node_eval.invokeStatic(node.clj:66)
        at cljs.repl.node$node_eval.invoke(node.clj:60)
        at cljs.repl.node.NodeEnv._evaluate(node.clj:234)
        at boot.user.Delegatingcljs_repl_node_NodeEnv._evaluate(Unknown Source)
        at cljs.repl$evaluate_form.invokeStatic(repl.cljc:553)
        at cljs.repl$evaluate_form.invoke(repl.cljc:484)
        at cljs.repl$evaluate_form.invokeStatic(repl.cljc:491)
        at cljs.repl$evaluate_form.invoke(repl.cljc:484)
        at cemerick.piggieback$eval_cljs.invokeStatic(piggieback.clj:271)
        at cemerick.piggieback$eval_cljs.invoke(piggieback.clj:270)
        at cemerick.piggieback$do_eval.invokeStatic(piggieback.clj:297)
        at cemerick.piggieback$do_eval.invoke(piggieback.clj:278)
        at cemerick.piggieback$evaluate.invokeStatic(piggieback.clj:320)
        at cemerick.piggieback$evaluate.invoke(piggieback.clj:318)
        at clojure.lang.Var.invoke(Var.java:381)
        at cemerick.piggieback$wrap_cljs_repl$fn__6035$fn__6037$fn__6038.invoke(piggieback.clj:351)
        at cemerick.piggieback$enqueue$fn__6007.invoke(piggieback.clj:253)
        at clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__5917.invoke(interruptible_eval.clj:190)
        at clojure.lang.AFn.run(AFn.java:22)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)