Fork me on GitHub
#cljs-dev
<
2018-03-12
>
richiardiandrea00:03:30

Every time I look at js.cljs I am in awe by its simplicity.. maybe because I have been there quite a bit, I really like the fact that in one unique file it is possible to completely understand the whole compilation pipeline. For instance today I learned that when the language returned by load-fn is :js basically :source is ignored completely. This means that implementations can avoid a disk read.

mfikes15:03:43

A couple of potential post-1.10.x features in the queue now 🙂 - Support for Git deps in Shared AOT Cache (CLJS-2651) - Warn on private var use (CLJS-1702)

richiardiandrea17:03:33

compiling replumb with the new version feels super quick! big improvements

john18:03:22

I'm not seeing a jira on this, but on 1.10.145, on a standard -m cljs.main repl, I get a Exception in thread "main" java.lang.StringIndexOutOfBoundsException: String index out of range: -57 when entering a form like js/blah where the the reference is not defined. Looks like the cause is cljs.repl/file-display

john18:03:35

Is this known or should I jira it?

john18:03:46

and the repl connection breaks

dnolen18:03:29

@john your description of the issue is not very clear

dnolen18:03:41

what are the exact cljs.main arguments

john18:03:52

just clj -m cljs.main with org.clojure/clojurescript {:mvn/version "1.10.145"} in my global deps file

john19:03:11

then, in the repl, typing js/blah

dnolen19:03:12

and what you are typing at the REPL?

dnolen19:03:29

I can’t repro that, try using master via git deps

mfikes19:03:35

I can't repro that with 1.10.145 as well

cljs.user=> js/blah
ReferenceError: Can't find variable: blah
cljs.user=> *clojurescript-version*
"1.10.145"

john19:03:49

using the git dep fixes it for me

mfikes19:03:00

@john Given that, one thing that might be worth trying is 1.10.145 with -co '{:aot-cache false}' to see if there is something funky in your ~/.cljs tree

john19:03:29

@mfikes hmm, doing clj -m cljs.main -co '{:aot-cache false}' now causes the terminal repl to exit without any stacktrace and the browser opens to the correct html content but the browser console says Failed to load resource: net::ERR_EMPTY_RESPONSE

mfikes19:03:16

Exiting is expected; you'd need to add -r But the console error is unexpected

mfikes19:03:31

(Perhaps that reflects a simple race with terminal shutdown)

john19:03:07

clj -m cljs.main -co '{:aot-cache false}' -r results in the same behavior

john19:03:44

js/blah causes the repl to disconnect. Should I try -Srepro?

mfikes19:03:48

Thanks for testing that; it at least excludes some class of potential explanations.

mfikes19:03:52

I was wondering the same @john — whether you have some other dependency in the mix that might be doing something strange

john19:03:02

trying in a fresh directory, same thing... Blowing away my caches would probably work, but then I'd probably lose the evidence...

dnolen19:03:55

@john well if we can’t repro w/o your caches then nothing to do anyway

dnolen19:03:05

I would empty your caches and see if it works

dnolen19:03:14

then next time need to know the exact steps

john19:03:45

right, yeah, 1.10.145 is acting strange on me too, with a blank deps files... I'll clear them

dnolen19:03:48

your report would imply that somehow you have funny stack traces

dnolen19:03:01

but I can’t think of how that would happen

mfikes19:03:18

Ahh, maybe it is in the statck trace generation code... did I supply a patch that touched that recently?

john19:03:24

in various environments, I'm getting

ReferenceError: "blah" is not defined
	 (NO_SOURCE_FILE <eval>:1:0)

mfikes19:03:34

Checking...

john19:03:34

rather than what @mfikes was getting

mfikes19:03:41

Which browser?

john19:03:59

I think it's launching in the jvm jsvm

john19:03:24

on one particular launch... How can you detect which env you've launched in?

mfikes19:03:37

It might be browser-specific. Which browser?

john19:03:07

oh, I'm on chrome standard Version 64.0.3282.186 (Official Build) (64-bit)

mfikes19:03:19

Repro'd locally

mfikes19:03:31

Trying master to see if it is gone for me too

mfikes19:03:24

Repro'd with master

john19:03:03

On 1.10.145 I'm getting strange behavior with just clj -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "1.10.145"}}}' -m cljs.main. Browser launches. No html content. repl hangs.

john19:03:12

with empty deps files

john19:03:33

I'm going to clear my caches

mfikes19:03:35

John, since I can repro with master, I'll log a ticket

john19:03:44

should I clear .cpcache as well?

mfikes19:03:10

Well, it is occurring for me, so I doubt it is cache-related

mfikes19:03:30

It is a derailment when processing the Chrome generated stacktrace, I suspect

john19:03:49

yeah, it's in that file-display function

dnolen19:03:29

@mfikes I thought we had a try/catch so if we can’t convert we show original trace

mfikes19:03:41

The REPL and main entry points document has been updated to match master https://gist.github.com/mfikes/bdbe214f03abac88ae384adb1ac26490

john20:03:54

hmmm. My environment might be tainted... or did something change? Now when I try your example @mfikes clojure -Sdeps '{:deps {github-mfikes/cljs-main-rebel-readline {:git/url "" :sha "27b82ef4b86a70afdc1a2eea3f53ed1562575519"}}}' -i @setup.clj -m cljs.main The browser never connects back to the repl. (on chrome)

john20:03:12

But it was just working an hour or two ago

mfikes20:03:48

@john I just tried that, with Chrome as my default browser, from a fresh directory in /tmp/ and it worked. Perhaps add -Srepro?

john20:03:25

strange, I don't know if I'm using -Srepro right. I tried that and it didn't work. Same behavior. I wiped out the local directory and that fixed it.

mfikes20:03:19

I suspect (I haven't looked at the code), if you have an out directory sitting there it might be used. This is definitely true if you have an index.html there.

john20:03:52

I did have an out

mfikes20:03:13

Yeah, the logic might be to use that if it sees it

bhauman20:03:50

we're only caching jar artifacts correct?

mfikes20:03:08

Right. Git deps is sitting in a patch in a ticket for future consideration

john20:03:15

so if I go about switching between master and release, my artifacts for cljs may not get updated in out? Should I try to trace that down?

bhauman20:03:49

I just got this comment in figwheel issues

mfikes20:03:04

@bhauman My guess: :aot-cache is broken (in some way), so removing its cache directory ~/.cljs/.aot-cache fixed it for that user

bhauman20:03:33

but only jar artifacts get put there correct?

mfikes20:03:13

It would be a defect if compilation artifacts derived from source not in JARs was put in the shared AOT cache

bhauman20:03:56

just verifying

mfikes20:03:30

@bhauman Yep. FWIW, this patch extends that logic to git deps: https://dev.clojure.org/jira/browse/CLJS-2651

bhauman20:03:52

interesting 🙂

dnolen20:03:13

@bhauman we only want to put cacheable stuff into the AOT cache

dnolen20:03:19

JAR deps, and later maybe GIT deps

mfikes20:03:49

The Figwheel commenter perhaps had a JAR dep with a macro that had a side effect upon expansion or somesuch

dnolen20:03:05

@bhauman not sure about that user report - the only way to get into the cache is through compile-from-jar

bhauman20:03:13

that makes sense, perhaps the user above got it wrong as well, and drew the wrong conclusion

bhauman20:03:42

its easy to do

dnolen20:03:48

so a cache-unfriendly JAR/git dep will be head scratcher

dnolen20:03:15

anyways - it’s why it’s off now

bhauman20:03:29

yeah that could cause it, and it would look like project.clj caching

bhauman20:03:44

as that was title of the issue

mfikes21:03:51

It is interesting that -ro {:node-command "nodejs"} won't get passed down to be used to start node (it appears that there are setup opts that differ from REPL options, and these are not controllable via cljs.main

mfikes21:03:57

(Not suggesting we do anything about this now. If it proves to be an issue, people will let us know.)

john21:03:54

so, in my case, the file being passed into file-display in cljs.repl is coming in as NO_SOURCE_FILE

john21:03:18

It looks like in Clojure, when there's no source file, it just prints that blah in this context, compiling:(NO_SOURCE_PATH:0:0). Would similar behavior be desired here?

john21:03:20

This seems to work: clj -Srepro -Sdeps '{:deps {org.clojure/clojurescript {:git/url "" :sha "9ef53ad2767b873dc2277bb7174877b084cf6746"}}}' -m cljs.main -ro '{:launch-browser false}' -r

john21:03:56

Seems about the simplest change. Though I'd still need to test for when there is a source file, it doesn't break on chrome.

mfikes21:03:25

@john If you've submitted a CA perhaps you could submit a patch to https://dev.clojure.org/jira/browse/CLJS-2653

mfikes21:03:58

If you are not familiar with how to do that: https://clojurescript.org/community/patches

mfikes21:03:40

Also be sure to not commit the auto-generated change to set the *clojurescript-version* dyn vars

john21:03:52

I'll send a patch in

mfikes21:03:29

(Cool. Not speaking to the suitability of the patch; just indicating that this is how they are normally reviewed.)

mfikes22:03:48

@john Without digging into it too deeply, I wonder if the problem is in the mapped-stacktrace fn (or somewhere producing a bad value for :file) https://github.com/johnmn3/clojurescript/blob/9ef53ad2767b873dc2277bb7174877b084cf6746/src/main/clojure/cljs/repl.cljc#L172

mfikes22:03:54

My reading of that is "<NO_SOURCE_FILE>" is a legal value, but "NO_SOURCE_FILE" is not bueno.

mfikes22:03:30

The problem with conditionally handling the offensive :file value is that other REPLs may have their own code that also expect a proper :file value. (Thus arguing for rooting out where it went wrong.)

john22:03:00

Would you rather mapped-stacktrace not emit "NO_SOURCE_FILE" instead?

john22:03:47

I'm actually not sure why the index is out of bounds.. perhaps it's just a simple error, and subs should be returning "NO_SOURCE_FILE" trimmed?

john22:03:18

If the intention is to just get the file name out of the canonical path, I think there's a java method for that. I'll try that.

mfikes22:03:24

The problem could be over in cljs.stacktrace/chrome-st-el->frame and if so, fixed down in there

mfikes22:03:22

(The architecture allows for flexibly parsing stack traces produced by different JavaScript environments, and my hunch is that the problem is not in the cljs.repl code but in the code specific to the Chrome user agent.)

mfikes22:03:45

There are a boat load of "manual" tests for :chrome in the comment, starting around here https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/stacktrace.cljc#L147

mfikes22:03:23

My immediate guess is that it could be changed simply to "<NO_SOURCE_FILE>"

john22:03:58

right. I'll try that

john22:03:52

clojure
cljs.user=> js/blah
ReferenceError: blah is not defined
	 (<NO_SOURCE_FILE>)

john22:03:11

See, but in your example, on safari, you had no extra line

john22:03:15

it was elided completely

mfikes22:03:19

@john Right, the Safari initial exception is probably different enough to result in there being nothing to elide

cljs.user=> js/blah
ReferenceError: Can't find variable: blah
cljs.user=> (println (.-stack *e))


eval code
eval@[native code]

clojure$browser$repl$evaluate_javascript@http://localhost:9000/clojure/browser/repl.js:116:4

deliver@http://localhost:9000/goog/messaging/abstractchannel.js:141:21
xpcDeliver@http://localhost:9000/goog/net/xpc/crosspagechannel.js:734:19
messageReceived_@http://localhost:9000/goog/net/xpc/nativemessagingtransport.js:321:23
fireListener@http://localhost:9000/goog/events/events.js:744:25
handleBrowserEvent_@http://localhost:9000/goog/events/events.js:870:34

nil

john22:03:54

Well that was all, just changing "NO_SOURCE_FILE_" to "<NO_SOURCE_FILE>". If the above output is desired, I can send up a patch this evening.

mfikes22:03:19

Seems like a good change regardless as it causes it to be a proper :file value.

mfikes22:03:59

And be sure to submit a CA if you haven't

mfikes22:03:50

Unless you are John MIchael Newman III 🙂

john22:03:19

I am 🙂

john22:03:44

Submitted one a while back

john23:03:59

Hoping I did that right. Patch added to 2653.

mfikes23:03:39

@john That patch doesn't resolve the issue for me (at least with the way the ticket is written)

mfikes23:03:02

The issue appears to be a regression caused by https://github.com/clojure/clojurescript/commit/92ccc3b90a6aef944620856002f44cd4683dd5f8 But your patch helps by giving it a way to detect when to opt out of running the code in the branch temp-output-dir? is true in temp-output-dir?

john23:03:51

You are still getting the same stacktrace?

mfikes23:03:30

Yes, for me the root problem is when it attempts to do the "canonicalize" logic

mfikes23:03:47

(defn file-display
  [file {:keys [output-dir temp-output-dir?]}]
  (if (and temp-output-dir?
           (not (string/starts-with? file "<")))
    (let [canonicalize (fn [file] (.getCanonicalPath (io/file file)))]
      (subs (canonicalize file) (inc (count (canonicalize output-dir)))))
    file))
when combined with your patch, fixes it for me

john23:03:47

ah! me too!

john23:03:56

I had to run the repro

john23:03:01

ay caramba

mfikes23:03:02

Notice the extra test for "<"

mfikes23:03:19

(That canonicalize stuff is new code, and probably isn't robust. It was just added with https://dev.clojure.org/jira/browse/CLJS-2610)

john23:03:52

In what circumstance will the file name not be NO_SOURCE_FILE, when out is a temp dir? When we have an input file at the cli?

mfikes23:03:12

One place where you can get file information is for functions in the core library, for example (ffirst [1])

mfikes23:03:48

Clearly, my patch in CLJS-2610 is wrong in that it assumes file is a something you can pass to (io/file ...)

mfikes23:03:00

This will fail if file is <anything here>

john23:03:18

and it resolves to out/cljs/core.clj

mfikes23:03:38

So a minimal fix (that could be combined with yours is to add a bit that fixes the regression I introduced)

john23:03:57

ah, so that's how the output dir relates to the subs, gotcha

mfikes23:03:59

(and temp-output-dir?
           (not (string/starts-with? file "<")))

mfikes23:03:07

That little bit might be sufficient

mfikes23:03:32

Not quite sure yet...

john23:03:03

What if the subs part was smart enough to deal with both situations, a file and a NO_SOURCE_FILE

mfikes23:03:20

On the surface, it seems like adding that extra check for "<" is enough. (It certainly fixes things for me with js/blah in Chrome.

mfikes23:03:52

Yeah @john if you see another approach to more gracefully handle it

john23:03:56

So, we just actually print the NO_SOURCE_FILE

john23:03:11

How would clojure do it in this particular situation?

john23:03:20

I thought this situation was a little different

john23:03:22

user=> blah
CompilerException java.lang.RuntimeException: Unable to resolve symbol: blah in this context, compiling:(NO_SOURCE_PATH:0:0)

mfikes23:03:32

The file canonicalization stuff in CLJS-2610 was handling the case where you get a long path like /private/var/folders/gx/nymj3l7x4zq3gxb97v2zwzb40000gn/T/out667248762303295388152139830015529/out/cljs/core.js and it was (naively) trimming off the temp output dir from the beginning. And tripping up on NO_SOURCE_FILE case

mfikes23:03:16

I suspect any approach that makes that naive code less fragile would fix things