Fork me on GitHub
#cljs-dev
<
2018-03-05
>
mfikes02:03:50

It turns out that https://dev.clojure.org/jira/browse/CLJS-2611 was a bit more serious of a problem than simply failing to map stacktraces: A mixture of "out" and synthetic temp dir were being used.

mfikes02:03:30

The cljs.jar that ships with 1.10.126 includes Clojure 1.10.0-alpha4. (Wondering if that is intentional)

mfikes02:03:32

I guess that is for prepl support?

dnolen05:03:21

for uberjar that seems fine yes

mfikes12:03:13

One Quick Start feedback repeated another regarding separating Windows and Unix https://twitter.com/mamuninfo2/status/970588433713389568

mfikes12:03:50

IMHO, this would help, but perhaps Windows version of clj isn't that far off in the big scheme of things.

dnolen12:03:21

Yeah just not concerned about it

dnolen12:03:39

if that’s big feedback, then we’re doing something right 🙂

jannis13:03:15

Argh, I’m debugging the hell out of the :npm-deps indexing and conversion to Closure modules but I still only have vague ideas of what might be going wrong.

jannis13:03:26

(Trying to add support for .mjs still)

mfikes13:03:21

With the shared AOT cache, if code in a JAR results in an analyzer warning, you generally only see it once, whereas previously you would see it after a "clean". (I have no profound thoughts on this; just sharing.)

dnolen13:03:39

@mfikes yeah I don’t think we should necessarily do anything about that other than document that :aot-cache false is a good idea when sorting through an issue

mfikes13:03:54

Ahh, good workaround.

dnolen13:03:55

yeah :aot-cache flag needs to be well documented

dnolen13:03:45

@jannis do you have theories where .mjs is getting dropped?

jannis13:03:21

@dnolen Here’s my current take: * They are present as :foreign-libs before https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/closure.clj#L2771 * They are moved to :libs via the handle-js-modules call * They are dropped (as in: only the entry point .mjs file survives) in add-js-sources: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/closure.clj#L2777

dnolen13:03:20

@jannis does Clojure support .mjs in it’s own Node.js resolution

jannis13:03:29

And more specifically, add-js-sources tries to traverse the JS depedencies via js-dependencies but they are not present in the :js-dependency-index right here: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/closure.clj#L704

jannis14:03:24

Assuming they should be in :js-dependency-index, I haven’t figured out why they aren’t there.

jannis14:03:00

One difference I noticed is that e.g. ...$graphql$index-mjs depends on ...$graphql$execution but the only thing we have is ...$graphql$execution$index-mjs.

jannis14:03:56

Even though the :provides of the indexed .../graphql/execution/index.mjs includes all of ("graphql/execution/index.mjs", "graphql/execution/index" "graphql/execution").

dnolen14:03:36

yes so that’s the problem

dnolen14:03:41

Closure Node.js resolution

jannis14:03:47

Regular .js modules from other dependencies (e.g. React) are typically written with goog.provide('...$react$index') (instead of e.g. goog.provide('...$react$index-js')).

dnolen14:03:52

let’s rewind so you understand

dnolen14:03:19

we need to pass all the NPM JS stuff to Closure that we expect we might need

dnolen14:03:30

all this song and dance is to figure the potential set of JS inputs

dnolen14:03:55

Closure module processing does not use the disk to resolve

mfikes14:03:02

@dnolen captured need to document :aot-cache compiler option https://github.com/clojure/clojurescript-site/issues/182

dnolen14:03:13

@jannis everything must be in the input set

dnolen14:03:40

inside of Closure it uses the Node.js resolution algorithm to figure out the requires

dnolen14:03:53

as in they wrote that in Java

dnolen14:03:07

but it must be kept in sync with Node.js community practice

jannis14:03:41

@dnolen Ok, but don’t we write the cljs_deps.js file the processed node_modules dir with the goog.provide entries?

dnolen14:03:02

I think you’re missing what I’m saying 🙂

dnolen14:03:09

see the above

jannis14:03:42

Maybe I am. 😉

dnolen14:03:49

if this file above doesn’t pick up .mjs it won’t work

dnolen14:03:03

it doesn’t matter what else we do

jannis14:03:14

Ok, I can see it clearly has no support for .mjs extensions.

dnolen14:03:46

so the easiest thing to do is pull that project, try fixing it, build and pulling that in as dep

dnolen14:03:59

and testing against ClojureScript

dnolen14:03:10

then if that works, submit a PR etc.

jannis14:03:13

Ok, I’ll do that.

jannis14:03:34

What I’m still confused about / don’t understand is what our input to Closure should be once it supports .mjs.

dnolen14:03:45

does npm have explicit .mjs support?

dnolen14:03:08

@jannis I think you’re probably doing everything right

dnolen14:03:20

but you were missing a part of the pipeline

jannis14:03:33

I’d be surprised if npm didn’t have support… .mjs originates from the NPM project as far as I understand.

jannis14:03:23

Actually, that may be totally wrong.

bhauman14:03:19

https://dev.clojure.org/jira/browse/CLJS-2617 Add events for significant compiler signals (compile finished, warnings, exceptions)

jannis14:03:49

@dnolen Assuming I can add support for .mjs to Closure, wouldn’t we still have to write all dependencies between .mjs sources to cljs_deps.js?

dnolen14:03:35

@jannis I don’t know what you’re talking about, cljs_deps.js is automatically correct if all previous steps worked 🙂

jannis14:03:07

Them being dropped as part of add-js-sources (and them not being in :js-dependency-index) is still wrong, I assume?

jannis14:03:14

Or is that influenced by what Closure does?

dnolen14:03:22

@jannis yes you’re missing what I said above

dnolen14:03:27

fix Closure then it should just work

dnolen14:03:47

stop trying to figure this out 🙂 just add .mjs support where it’s missing

dnolen14:03:53

you’re likely just seeing symptoms

dnolen14:03:33

if Closure doesn’t work, then no way the index will be correct

dnolen14:03:46

and then no way cljs_deps.js will be correct

jannis14:03:33

Ok, this lead me to check where Closure is used in the first place, and it’s in handle-js-modules, which comes before add-js-sources.

jannis14:03:54

@dnolen I’ll do as you say and see if I can fix Closure. Thanks 🙂

dnolen14:03:33

@jannis by all means understand the pipeline - just saying we can save you some debugging goose chases

jannis14:03:52

Hehe, I’d say that’s great 🙂

mfikes14:03:44

@dnolen Captured draft content ideas for either an AOT cache guide or news post here: https://github.com/clojure/clojurescript-site/issues/183

dnolen14:03:29

@mfikes great, gonna need a socket REPL / pREPL one too but let’s worry about that later

dnolen14:03:38

and I can tackle that one

mfikes14:03:14

Sometimes it is not clear to me whether these should be news or guides. No big deal.

dnolen14:03:58

news is good to radiate new features - some do and don’t need guides

mfikes14:03:08

I guess :aot-cache doesn't need so much of a "guide" flavor, as it is a self-running feature.

mfikes14:03:55

I wonder if :cache-analysis means "local cache, including source maps", and if so perhaps the global key name could be derived from that key name

mfikes14:03:17

(Pondering the name of :aot-cache.)

mfikes14:03:30

I think the key name is fine—I can't think of a better name for it right now.

jannis14:03:06

@dnolen Nice, found out why those .mjs files would result in ...$index-mjs instead of ...$index like .js files. There’s code in Closure to strip the extension when generating the identifier. I should have looked at Closure earlier! 🙂

jannis14:03:19

@dnolen I guess for our purposes it’s safe to build the compiler without GWT?

dnolen14:03:01

far as I know

jannis14:03:35

Nevermind, it already finished compiling.

jannis14:03:53

Has anyone seen this before when running lein with-profile +closure-snapshot test against ClojureScript? java.lang.IllegalArgumentException: No matching ctor found for class com.google.javascript.jscomp.Es6RewriteModules

jannis14:03:01

Perhaps the parameters have changed.

jannis14:03:53

Yep, that was it

juhoteperi14:03:06

Looks like the new value is Nullable, so probably we can just pass in null

dnolen14:03:19

@bhauman https://dev.clojure.org/jira/browse/CLJS-2617, I think we should just copy the warning handler pattern, using compiler atom for events is not attractive to me

dnolen14:03:56

we could have *handlers* dyn var, and bind this from compiler opts, and then in the compiler invoke for different event types

bhauman15:03:19

@dnolen if handlers could be an observable that would be the best

jannis15:03:31

Yay, simple single .mjs file dependency still works, now with ...$iterall$index instead of ...$iterall$index-mjs.

bhauman15:03:41

because currently otherwise it's really hard to reliably bind warnings from inside the compiler thread

dnolen15:03:38

@bhauman I don’t know what you mean

dnolen15:03:46

binding from inside doesn’t sound like a good idea to me

bhauman15:03:53

its a incredible idea

dnolen15:03:09

I don’t like your ticket at all, the proposal

bhauman15:03:17

see my minimal figwheel

dnolen15:03:29

that’s fine, but we’re not gonna base big decisions off that

dnolen15:03:34

binding from inside is a mess

dnolen15:03:41

it’s clear from your ticket description

dnolen15:03:34

also parallel stuff sounds bad with this approach

bhauman15:03:22

I'm just trying to prevent having to compose over the compiler

dnolen15:03:35

yes I understand that

bhauman15:03:52

and allow figwheel to be a library that you can require from a cljs.main repl

dnolen15:03:00

but I also think it leads in a lot of wrong directions architecturally

dnolen15:03:41

we should look for a more principled way to invert the relationship - but I’m not sure it’s possible

bhauman15:03:14

yeah that's all I'm asking for, would really love for this to be possible

bhauman15:03:16

parallel compiles shouldn't share the same env ever

bhauman15:03:28

parallel builds should and do of course

jannis15:03:58

Interesting: goog.require("Require{symbol=module$Users$jannis$Work$oss$clojure$clojurescript$node_modules$graphql$graphql, rawText=./graphql, type=ES6_IMPORT}"); I guess the output of CompilerInput.getRequires changed too.

juhoteperi15:03:34

@jannis symbol is the interesting part

bhauman15:03:54

and simply stamping the compile as finished in the compiler env shouldn't hurt the architecture or any racy builds

juhoteperi15:03:08

@jannis We probably can't apply these Closure fixes to Cljs master before next Closure release, but you can prepare issue with patch to be applied once we update Closure

jannis15:03:42

Of course, that makes sense

dnolen15:03:50

@juhoteperi or we could finally start releasing Closure ourselves 🙂

mfikes15:03:16

I was wondering about that. What if ClojureScript outlives Closure. Could happen.

juhoteperi15:03:43

I doubt there would be much benefit, they have lately released quite predictably once per month and they probably do some testing we would have to do ourselves

mfikes15:03:46

Thinking 15 years from now. 🙂

bhauman15:03:07

15 years from now it's a crap shoot anyway

dnolen15:03:30

@bhauman while I think your simple figwheel thing is cool, I’m not really convinced that this is better in the end than just providing a repl-env

bhauman15:03:32

JS will be unrecognizable by then

juhoteperi15:03:44

And based on previous release dates, there is good chance the march release coming soon

dnolen15:03:14

I added enough stuff to compose over main, you can add flags, etc.

bhauman15:03:11

yes but I can't use the browser repl and built-in watcher

dnolen15:03:04

right but this is not a real problem 🙂 built-in browser / watcher is just for beginners, and experienced people who want to test something or do something simple

dnolen15:03:40

for a normal end user - they add a dep clj -A:fig -id dev

dnolen15:03:58

they don’t care about browser repl or build in watcher

dnolen15:03:18

to me from a usability stand point what matters most here is semantics of the standard flags

bhauman15:03:00

I was envisioning the browser repl becoming more robust over time, the watcher as well, and core tools would really take over

bhauman15:03:39

and figwheel would be sidelined

bhauman15:03:54

which would be as fantastic an outcome as I could imagine

dnolen15:03:35

that’s a possibility but I’m still convinced the innovation here shouldn’t be hampered by mainline dev

dnolen15:03:56

and with clj I think it’s about easy as Node.js to plug something in

dnolen15:03:06

to me the lack of coordination around ClojureScript tooling has mostly been a good thing 🙂

dnolen15:03:42

it’s taken a long time to get ClojureScript core compiler stuff solid, and that probably isn’t going to change anytime soon

dnolen15:03:23

so from my perspective at least, shadow-cljs, figwheel - this is all good stuff

dnolen15:03:32

and of course, some of that stuff filters back, but it’s way slower

bhauman15:03:16

I'm bringing this up because it seems we're currently integrating some of those lessons back into the core. A figwheel like workflow has been a significant part of the growth of CLJS. So I'm kinda pushing to enable this without needing to compose over the compiler ... Of course I can easily continue to do that. But if the core of the Figwheel workflow is only a 200 line cljc file and it doesn't need to compose around the compiler, well that seems like an interesting trade off to me.

bhauman15:03:29

There is no urgency to this

bhauman15:03:07

just an interesting idea

dnolen16:03:40

I agree it’s interesting, but probably on hold for now 🙂 that said a generic events handler thing still seems like a good idea, no reason to limit it to warning as it is now

mfikes16:03:12

@dnolen Do you know if anyone has tracked down this odd behavior with 1.10.x?

ios:cljs.user=> (def r (om/reconciler {:state {:a 1}}))
#'cljs.user/r
ios:cljs.user=> (om/reconciler? r)
false
I’m digging, but don’t want to waste time if already sorted.

dnolen16:03:56

or rather I did after report

dnolen16:03:02

om.next master works

mfikes16:03:16

Thanks. Will try master. I’m on beta2

mfikes16:03:00

Confirmed fixed. And, the broadened scope on https://dev.clojure.org/jira/browse/CLJS-2616 is a good call as it allows a lein install of non-snapshot JARs as well (thus enabling me to just lein install master of om.next)

dnolen16:03:20

@mfikes that’s good to hear

jannis16:03:53

I don’t remember fixing anything 😉

mfikes16:03:11

Right, David did the fix, based on your report.

jannis16:03:08

I think the report came from someone else also

jannis16:03:15

So no credit for me 😉

mfikes16:03:19

When I see them, I think I’m going to start adding comments to tickets like the one in https://dev.clojure.org/jira/browse/CLJS-2451 (unless there is some other precedent on how to handle these).

mfikes16:03:45

(Tickets without a minimal repro, linking to github repos that use lein etc.)

richiardiandrea16:03:34

Funny I was wondering as well yesterday if a robust socket repl for node should be in core instead. Same train of thought of more robust tooling in core.

richiardiandrea16:03:37

About prepl, yesterday I did not have to time to get to it, I could write a post on new clj + socket repl so that folks start using it

dnolen16:03:36

@richiardiandrea yes that would be nice it should cover the new socket REPLs + pREPLs, feel free to take a run at that, I can help edit etc.

richiardiandrea16:03:39

Cannot promise now at the beginning of the working week 😃 🤞

dnolen16:03:57

@richiardiandrea no rush, it would come after release post anyway

richiardiandrea17:03:00

Ok cool, I can maybe sneak in some time on https://dev.clojure.org/jira/browse/CLJS-2613 today

alpeware16:03:50

I'm trying to concatenate all unoptimized JS files into one large JS file (same as closure-library/closure/bin/calcdeps.py), but having a hard time getting a hold of all the JS Sources in dependency order. Seems like this should be trivial, but can't seem to figure this out. Any pointers?

dnolen17:03:14

@alpeware is there a reason to not use the tools to do this? we don’t provide direct to accomplish this - you can root around the compiler and probably figure it out though

alpeware17:03:51

i'm trying to work around some memory constraints where i'm building. the closure compiler requires a lot of memory since it's pulling everything into a giant string apparently.

alpeware17:03:31

anyway, good to know i'm not missing anything in the cljs.build.api namespace

alpeware17:03:13

i'll dig through cljs.closure and see what i can find

dnolen17:03:07

@alpeware hrm I don’t think I’ve seen that Closure uses a lot of memory except under advanced

bhauman17:03:21

I agree that the generic handler is a good idea

richiardiandrea21:03:44

So is this command wrong somehow?

clojure -Srepro -m cljs.main -co "$(cat cljsc_opts.edn)" -re node -c cli-repro.core -r

richiardiandrea21:03:29

if not there is probably a regression, because it used to work

mfikes21:03:43

Looks correct to me.

richiardiandrea22:03:52

yep it is a regression, it was working at 04e55f88bf61006e0abd4e276eaf2297d91d40ce

richiardiandrea22:03:34

I thought it was new, but it is not, it has been reported in https://dev.clojure.org/jira/browse/CLJS-2613

richiardiandrea22:03:16

let me try one more time just to be sure

richiardiandrea22:03:22

I am confused...seems fine now against master 😕

richiardiandrea23:03:02

@dnolen I did the grunt work for https://dev.clojure.org/jira/browse/CLJS-2613, probably at this point only the fix is what's missing there. I am not sure how you want to handle it though so I am seeking advice