Fork me on GitHub
#cljs-dev
<
2018-03-02
>
mfikes00:03:34

@dnolen At the bottom of out\cljs\core.js is

//# sourceMappingURL=core.js.map
if that’s what you are asking. It appears that this may not be a regression introduced with cljs.main: https://github.com/google/closure-compiler/issues/2611#issuecomment-345595935

mfikes00:03:49

Good news is that all of the examples in the "`cljs.main` tour" news post work properly on Windows, apart from the issue encountered in CLJS-2588.

dnolen01:03:35

ah, yeah that seemed pretty strange

dnolen01:03:45

the Windows thing - I don’t think we can do anything about that

dnolen01:03:03

@mfikes that’s great! I enjoyed the videocast today 🙂

mfikes01:03:42

Yeah, that was fun. I’m happy that cljs.main somehow became a prominent part of it 🙂

mfikes01:03:12

About the Windows thing, I wonder if this affects all attempts to use Closure on Windows. (I’m gonna see if I can come up with some slight variation so that this portion of the “tour” works on Windows.)

mfikes02:03:26

@dnolen FWIW, if you (.setResolveSourceMapAnnotations compiler-options false) things compile

mfikes02:03:14

(This is in cljs.closure/set-options)

dnolen02:03:33

ok fixing that here now then

mfikes02:03:58

Cool. Hopefully source mapping doesn’t actually need that path resolved. 🙂

dnolen02:03:06

I don’t think so we only generate source maps for our own stuff & we handle that completely

mfikes02:03:18

Ahh. Hah. Cool!

dnolen02:03:22

Closure generates one of course when compiled, but we just merge that with our own thing

mfikes02:03:07

I wonder if this actually implies nobody uses ClojureScript on Windows for :advanced Hrm.

dnolen02:03:36

we can drop this soon as it gets fixed in Closure

dnolen02:03:18

hrm yeah that’s not a new issue

dnolen02:03:21

@mfikes hrm in your example core.js.map did exist yes?

mfikes02:03:53

Yes, it did

mfikes02:03:39

Here is what I see after your patch:

C:\Users\mfikes>cd out\cljs

C:\Users\mfikes\out\cljs>dir
 Volume in drive C has no label.
 Volume Serial Number is 483F-8B01

 Directory of C:\Users\mfikes\out\cljs

03/01/18  02:21 PM    <DIR>          .
03/01/18  02:21 PM    <DIR>          ..
03/01/18  09:36 PM           329,933 core.cljs
03/01/18  09:36 PM         1,276,371 core.js
02/28/18  10:07 AM           665,773 core.js.map
03/01/18  09:36 PM             1,088 nodejs.cljs
03/01/18  09:36 PM             1,001 nodejscli.cljs
03/01/18  01:30 PM           127,865 pprint.cljs
03/01/18  08:12 AM           154,429 pprint.cljs.cache.json
03/01/18  01:30 PM           493,716 pprint.js
02/28/18  10:07 AM           179,817 pprint.js.map
03/01/18  01:30 PM             2,154 repl.cljs
02/28/18  10:10 AM             1,265 repl.cljs.cache.json
03/01/18  01:30 PM            11,562 repl.js
02/28/18  10:07 AM             5,055 repl.js.map
02/28/18  10:07 AM    <DIR>          spec
              13 File(s)      3,250,029 bytes
               3 Dir(s)  20,814,352,384 bytes free

dnolen02:03:59

@mfikes actually I’m going to revert that for now /C:/Users/mfikes/out/cljs/core.js shouldn’t those be backslashes in the name?

dnolen02:03:25

well reverting and re-opening for now, I think we need to look at it more closely

mfikes02:03:17

Not that this implies it is valid, but here is an interesting test done on that box:

user=> (io/file "/C:/Users/mfikes/out/cljs/core.js")
#object[java.io.File 0x53de625d "C:\\Users\\mfikes\\out\\cljs\\core.js"]
user=> (count (slurp *1))
1276371

mfikes03:03:50

@dnolen I can no longer reproduce CLJS-2588 as originally written, and made a comment showing the exact steps I took attempting to reproduce it https://dev.clojure.org/jira/browse/CLJS-2588?focusedCommentId=48452&amp;page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-48452 I’ll close it as “Not Reproducible” for now. Without the ability to reproduce the issue, it is not clear to me how to even work on a fix.

mfikes13:03:41

@dnolen I was pondering writing a separate suite of tests (maybe script/test-cli) that essentially drives the CLI through its paces using clojure.java.shell/sh, setting up each test in a temp dir (maybe via some appropriate use of (File/createTempFile)), setting up src in there, etc, and ultimately looking at :out to see if -e or other output prints correctly. These tests would be slow (and thus separate). I'm thinking the script can take as args what would go to -re, and perhaps optional -ro EDN. That way we could cause it to drive things with -re node and -re nashorn in CI, etc. I was also thinking that, if we add an additional browser REPL env flag (along the lines of the :launch-browser just proposed) to make it be "silent", to optionally turn off these lines

Compiling client js ...
Serving HTTP on localhost port 9000
Listening for browser REPL connect ...
and with that we could do something like script/test-cli browser '{:silent true}` and at least run all of these tests manually letting it launch browsers on our desktop. Of course you could also do script/test-cli ambly '{:choose-first-discovered true} as an example to drive a downstream REPL. Sorry for the wall of text, but just sharing in case there could be improvements to a way to test this. Probably won't start right away.

dnolen13:03:40

yeah having a way to test cljs.main would be nice

dnolen14:03:01

@mfikes so I’m thinking we should change the Quick Start to show two paths, Windows is uberjar, but Linux / OS X is clj

mfikes14:03:28

Yeah, I can't think of any issues with that approach.

dnolen15:03:22

It’s also likely @anmonteiro can provide some insight here as to why .mjs isn’t picked up by this

dnolen15:03:54

@jannis your Clojure patch will only fix the issue at the top level - but in order to get all deps for top level stuff you need to make sure the cljs-oss fork of module-deps is working correctly

jannis15:03:30

@dnolen Yeah 😕 I already noticed that and commented on the issue.

jannis15:03:15

Still working out how I can get e.g. graphql to work, which uses .mjs all over the place (and also has CommonJS .js files in parallel to .mjs…).

jannis15:03:58

The current patch will only add index.mjs to the index but won’t include all other .mjs files (it’ll pick the .js ones instead), which is not going to work.

dnolen15:03:44

@jannis yes 🙂 and I’m giving you more information about what you might have to fix

dnolen15:03:08

we only do top-level stuff, we rely on Node.js to get the transitive deps

dnolen15:03:19

via that cljs-oss lib

jannis15:03:41

I haven’t even looked at @cljs-oss/module-deps yet. So far I’m trying to get the .mjs files to get indexed properly.

jannis15:03:43

Ah, cool, thanks

jannis15:03:52

I was just starting to look at where the transitive deps come from.

jannis15:03:01

So far I’ve only looked at module_deps.js in the CLJS source tree. There’s some .js-extension-only logic in there, but changing that may indeed not be enough.

jannis15:03:06

Digging deeper 🙂

jannis16:03:49

Ok, yeah, @cljs-oss/module-deps returns empty deps for graphql/index.mjs.

dnolen16:03:34

we use other libs to the resolution, maybe they just need to be bumped?

dnolen16:03:41

resolve, browser-resolve

jannis16:03:09

I’ll check

jannis17:03:46

Bumping all dependencies doesn’t change things. I’ll check their code too

tmulvaney17:03:57

I was playing round with the clj -m cljs.main for compiling stuff and it looks like om.next isn't being compiled correctly in these newer 1.10.* releases of clojurescript.

tmulvaney17:03:02

The failing case is:

clj -Sdeps "{:deps {org.clojure/clojurescript {:mvn/version \"1.10.63\"}
                    org.omcljs/om             {:mvn/version \"1.0.0-beta1\"}}}" \
    -m cljs.main \
    -re node \
    -e "(require '[om.next :as om])
        (def c (om/reconciler {:state {}}))
        (prn (type c))
        ;; Should return true but doesnt...
        (prn (om/reconciler? c))
        "

dnolen17:03:57

@tmulvaney need more information than “isn’t being compiled correctly”

dnolen17:03:02

what does that mean?

dnolen17:03:29

@jannis right, those projects may need to be fixed for this

tmulvaney17:03:21

Sure. So it compiles and runs with out any issues, but (om/reconciler? (om/reconciler {:state {}})) doesn't return true

jannis17:03:44

@dnolen I’m giving it a shot 🙂

dnolen17:03:51

@tmulvaney seems like you could easily launch a browser REPL to see what’s going on in the debugger

dnolen17:03:46

that said, seems pretty strange - like unlikely

tmulvaney17:03:10

Digging a little deeper (implements? om.next.protocols/IReconciler (om/reconciler {:state {}}))

tmulvaney17:03:50

is returning false as well. I started digging into this as the om app wouldn't mount and it was complaining about the reconciler not being a reconciler. The example I pasted is the minimal case i could produce.

dnolen17:03:34

@tmulvaney yeah I think it’s an om.next bug

dnolen17:03:12

@tmulvaney line 299 in om.next, hardcoded to 1.9

dnolen17:03:23

patch welcome

dnolen17:03:41

actually let me just fix this now

dnolen17:03:48

and you can use deps.edn to test this 🙂

dnolen17:03:13

This is definitely a massive step in the right direction just from bug reporting standpoint

tmulvaney17:03:13

Yes, much easier

jannis18:03:11

@anmonteiro That improves things a little. It changes all the indexing entries from

:file "/Users/jannis/Work/oss/clojure/clojurescript/node_modules/graphql/subscription/index.js",
to
:file "/Users/jannis/Work/oss/clojure/clojurescript/node_modules/graphql/subscription/index.mjs",
which is correct. What it doesn’t solve yet is that there is only a single addDependency entry for the whole graphql package in out/cljs_deps.js:
goog.addDependency("../node_modules/graphql/index.mjs", ['module$Users$jannis$Work$oss$jannis$cljs_npm_deps_mjs_issue$node_modules$graphql$index_mjs'], ['module$Users$jannis$Work$oss$jannis$cljs_npm_deps_mjs_issue$node_modules$graphql$graphql', 'module$Users$jannis$Work$oss$jannis$cljs_npm_deps_mjs_issue$node_modules$graphql$execution', 'module$Users$jannis$Work$oss$jannis$cljs_npm_deps_mjs_issue$node_modules$graphql$subscription', 'module$Users$jannis$Work$oss$jannis$cljs_npm_deps_mjs_issue$node_modules$graphql$error']);

anmonteiro18:03:07

is that not correct?

anmonteiro18:03:13

should there be more entries?

anmonteiro18:03:25

@jannis did you add .mjs in both extensions and moduleExtensions?

anmonteiro18:03:37

(I’m not sure what moduleExtensions does though)

jannis18:03:00

The last vector is dependencies, isn’t it? So at runtime, if there’s no entry for e.g. ...$graphql$graphql, you’ll get this error in the browser: base.js:1357 Uncaught Error: Undefined nameToPath for module$Users$jannis$Work$oss$jannis$cljs_npm_deps_mjs_issue$node_modules$graphql$graphql.

jannis18:03:23

I think I did, but I’ll do it again to be sure 😉

jannis18:03:09

I think the indexing may be correct now but what happens between indexing and writing the dependencies to out/cljs_deps.js is broken.

anmonteiro18:03:48

@jannis are you targeting the browser or nodejs?

tmulvaney18:03:57

@dnolen i tested lastest master looks like qualifier is still pinned?

tmulvaney18:03:32

scratch that. you've updated it in a new commit.

dnolen18:03:07

yeah try that, seems like it should work

dnolen18:03:15

definitely the issue in anycase

jannis18:03:12

@anmonteiro adding ‘.mjs’ to moduleExtensions doesn’t fix it but it might not hurt to add it there anyway.

tmulvaney18:03:15

@dnolen it works! Thanks! Sorry about polluting the channel with an om issue

dnolen18:03:35

@tmulvaney great! no worries

dnolen18:03:17

If something goes horribly wrong - there’s the “slim” classifier

mfikes18:03:27

Integration tests for cljs.main came out fairly readable. Could probably be improved more, but I don't want to over-engineer it for now. Small example is

(deftest eval-test
  (-> (cljs-main "-e" 3 "-e" nil "-e" 4)
    (output-is 3 4)))
I have a few other initial tests in there, all the way up to compiling an optimized build for Node: https://github.com/mfikes/clojurescript/blob/69f2751f38b5b898dcda379f19ad9950b658970a/src/test/cljs_cli/cljs_cli/test.clj Patch forthcoming shortly. (I think they also already caught a browser REPL bug, which I'll put together a minimal repro for and file a ticket.)

jannis19:03:07

This is in cljs.closure/build.

anmonteiro19:03:24

@jannis you may have to mess with cljs.js-deps

jannis19:03:35

Will do 🙂

anmonteiro19:03:03

@jannis would be interesting to know at which point the dependencies become missing

anmonteiro19:03:10

I would guess add-js-sources

anmonteiro19:03:30

(-> ... (doto clojure.pprint/pprint) ...) is your friend here

jannis19:03:46

Yeah, I'll see what I can find out, probably tomorrow though

jannis19:03:42

I so want to be able to just require graphql (and Apollo) in ClojureScript via :npm-deps. That's my goal. :)

dnolen21:03:48

master global caches JAR deps now, definitely a huge performance boost - not configurable (only really interested in supporting disabling if anything)

dnolen21:03:52

please give it a shot on your projecrts

mfikes21:03:26

It doesn't deal with git deps, right?

dnolen21:03:54

no, not for now - I’d rather focus on the most impactful case first

dnolen21:03:18

this is likely to introduce subtle bugs due to some messiness in the compiler

dnolen21:03:25

so mostly looking for issues

dnolen21:03:44

also testing that cljs.main works, and that you see caching behavior if you blow away out

dnolen21:03:49

I think I already see some problems 🙂

richiardiandrea21:03:22

should I just try building using the latest commit? what am I looking for?

dnolen21:03:06

Build yes, verbose output shows compiling from global cache

dnolen21:03:11

So just check that

mfikes21:03:32

@bhauman @dnolen I took a stab at reducing the copy in the default browser REPL page. Here is a shorter version. I'm not sure how it sits with me, but leaving it here for impressions.

dnolen21:03:13

that’s nice and to the point for me, but I’m not feeling particular about it, so more feedback welcome

dnolen21:03:32

I tweaked master, the verbose logging around caching was not quite right

mfikes21:03:02

It might be too terse or curt compared to the previous one, which was a bit more warm and welcoming.

dnolen21:03:11

yeah that’s what I was thinking

dnolen21:03:18

the tone could be lighter / warmer

mfikes21:03:54

Maybe working the word "Welcome" in the beginning would do it... will ponder some more.

sundarj21:03:38

maybe something like "happy hacking!" at the end?

mfikes21:03:41

Wow, @dnolen the caching is awesomely faster! (Trying core.async after blowing away out is very fast.)

dnolen21:03:22

yes this will be awesome - when it’s actually working 🙂

dnolen21:03:27

there’s some bugs

richiardiandrea21:03:08

compiles fine my lambda project here, one suggestion...why not using $HOME/.clojure as base dir? have you considered that?

dnolen21:03:15

yeah fixing some bugs

dnolen21:03:31

zero coordination is better

mfikes21:03:54

Ahh @dnolen I was wrong. (I have bugs in my testing process.) I was using node and failing to blow away .cljs_node_repl. Regardless, I'm only seeing cljs files under ~/.cljs/.aot_cache/

dnolen21:03:07

yeah I fixing some bad bugs right now 🙂

richiardiandrea21:03:05

this global caching is nice and could by reused by self-host compilers as well I guess

mfikes21:03:52

@richiardiandrea My thought: Feel free to read from it, but I wouldn't write anything produced by the self-hosted compiler into it.

dnolen21:03:38

ok master should much better, give it a shot

richiardiandrea21:03:40

but i see now another thing, only .cljs files are cached here, no analysis cache, so i am off topic 😄

dnolen21:03:52

(still bugs)

mfikes21:03:53

See backlog. try master now

dnolen21:03:37

with master you should see cache analysis, source files, and source map files

richiardiandrea21:03:34

I see them all now 👍

mfikes21:03:38

And .m2/repository/org/clojure/clojurescript/1.10.101/clojurescript-1.10.101.jar is AOTd 🙂

richiardiandrea21:03:28

quite an improvement

dnolen21:03:53

right, not bad, will see decent savings on cold builds when you have lots of deps

dnolen22:03:06

I think what we have might be mostly working - but try it on some projects

dnolen22:03:21

I do see an issue in the REPL which might need some sorting through

dnolen22:03:10

there seems to be race when we have to copy over cached things in JARs now

dnolen22:03:31

second time works because it’s in out, but not the first time when we copy from the cache into out

dnolen22:03:43

other than this annoyance (just start the REPL again) this seems to be working

dnolen22:03:52

probably need a flag (to disable), but I’d like people to test this first

mfikes22:03:18

@dnolen It appears to not put stuff derived from JARs in the global cache if ClojureScript itself is not a built dep (if it is :local/root or a git dep.) That seems like the right thing to do, but I don't yet see the code that prevents this.

mfikes22:03:54

(Don't need to dig up the code for me... just wondering if that was the intent.)

mfikes22:03:45

I updated the Bocko gist, which depends on actual Bocko JARs to point to the latest ClojureScript. https://gist.github.com/mfikes/e00202b2de7cc2352fedcf92b1fe60dc

mfikes22:03:21

Ahh, I was mistaken: Global caching works if ClojureScript is not a built dep. The gist above result in:

.cljs/.aot_cache/0.0.90964463958/BF21A9E/bocko/core.cljc
.cljs/.aot_cache/0.0.90964463958/BF21A9E/bocko/core.cljc.cache.edn
.cljs/.aot_cache/0.0.90964463958/BF21A9E/bocko/core.js
.cljs/.aot_cache/0.0.90964463958/BF21A9E/bocko/core.js.map

mfikes22:03:46

(The 0.0.90964463958 is the synthetic ClojureScript version number in this case.)

mfikes22:03:44

This might be a bug. If I remove out I see what looks like evidence that it is copying a compiled artifact from the cache, but then also compiling

Copying cached /Users/mfikes/.cljs/.aot_cache/0.0.90964463958/BF21A9E/bocko/core.js to out/bocko/core.js
Compiling out/bocko/core.cljc to out/bocko/core.js

mfikes22:03:15

Maybe this ^ is exactly the race David referred to above

mfikes23:03:25

Another tougher problem to solve (which Maven itself never solved for a long time...) is if you have two different projects compiling at the same time, potentially corrupting caches.

mfikes23:03:14

I guess one solution is to write all of the files for something (like those 4 bocko files above) into a temp dir, and then to swap the directory in place with one atomic operation via a filesystem move call

richiardiandrea23:03:39

I confirm the REPL thing happens the first time, the socket repl in particular seems to hang completely