ah we do have a little bit more work to do wrt. all this modules I think - REPL support

I’ve updated the Blocker/Critical list to reflect

will spend time on this tomorrow

can you post the links to those again?

(unless somebody nabs first)

https://dev.clojure.org/jira/browse/CLJS-2229

no I mean, the lists

https://dev.clojure.org/jira/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+CLJS+AND+resolution+%3D+Unresolved+AND+priority+%3D+Blocker+ORDER+BY+key+DESC&mode=hide

👍

https://dev.clojure.org/jira/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+CLJS+AND+resolution+%3D+Unresolved+AND+priority+%3D+Critical+ORDER+BY+key+DESC&mode=hide

thanks

Maybe in the future: A diagnostic for non-core code using js*

@mfikes it’s possible but I don’t actually care that much about that one

there are no rules, and nothing to impose

a warning would be nice perhaps for people who want to lint

don’t really understand CLJS-1801

Right, it is unlikely to change

@anmonteiro let me add a description it may no longer be valid

IMO there may be good reasons to use js* for people who know what they are doing 🙂

btw. canary just generated first downloadable jar release: https://github.com/cljs-oss/canary/releases/tag/r1.9.769-33e8067

@anmonteiro updated - sadly I didn’t record my thoughts when I opened it and it’s pretty old

gotta run for now

@mfikes https://dev.clojure.org/jira/browse/CLJS-2231 I don’t think this is a problem in master, we check to see if the fn is set

Well, it is checked for truthyness

(The default is a fn that throws.)

Testing this patch out, which may meet the need: https://github.com/mfikes/clojurescript/commit/c163bd72e3566640c9a2be59b9e8853b78240020

@mfikes I think we should stop setting them to fns that throw and push that logic one level up

i.e. move the throwing to string-print

This ticket goes down that road https://dev.clojure.org/jira/browse/CLJS-2002

I’ll try that and see how things behave.

updated the ticket with desired change

moving throw to string-print is fine

Lumo + CLJS-2232: https://twitter.com/anmonteiro90/status/885741059065528321

Do we have other examples for :preprocess than JSX?

I thought quickly about TypeScript, but I think that needs proper compilation instead of per file preprocessing.

Hmh, where did the message go 😄

Yes, Closure does have some kind of support for TypeScript.

:commonjs/`:amd` processing uses compiler options :language-in to process the sources, :es6 processing uses always :es2016

Perhaps js-modules should allow setting :language-in per module-type, or directory

Hmh, or at least Closure supports "TypeScript-style type annotations: > Initial work on supporting TypeScript-style type annotations, with the --language_in=ES6_TYPED flag.

That note from 2015 is only thing in Closure changelog about TS, so I'm not sure how good the support is

I updated the preprocess post with a bit more content: https://github.com/clojure/clojurescript-site/pull/112

The headers can be probably improved, but I think the order of content should be okay

@juhoteperi funny enough I was thinking about that exact case yesterday - I think we should hold off on how we will bulk preprocess - I don’t really think anything we’ve decided closes any interesting doors

Yeah, if we need to later change how preprocessing works, we can for example group-by the sources similar to module processing. And this doesn't break the :preprocess option.

@juhoteperi yeah also there’s a lot of new stuff here and I think further changes should probably be guided by usage feedback

I’m working through an issue where it appears a warn is printed for a bad aget or aset and this is potentially in the communication infrastructure between Figwheel and the app, and the warn itself seems to cause yet another, and it cascades. I end up seeing this in the Figwheel console, but no warnings there

Figwheel: message from client couldn't be read!
Figwheel: message from client couldn't be read!
Figwheel: message from client couldn't be read!
...

^ Sorted the above. Will send a PR to Figwheel. Also, less critical but cool: The new feature is pointing out invalid uses of aget at runtime in core.async. A simple example is here where aget is called, and then the result is checked for nil rather than also first checking to see if the array is non-empty: https://github.com/clojure/core.async/blob/master/src/main/clojure/cljs/core/async/impl/timers.cljs#L84

@mfikes nice

Good thing: Nothing wrong with the ClojureScript compiler so far with my tests.

that good news 🙂

testing things at the REPL is good thing to focus on. Going to start digging into requiring all the different supported module types at the REPL myself.

watching all these single function lodash modules get copied over during testing 😒

just refactored resolve-var to clean up the module stuff like I preferred https://github.com/clojure/clojurescript/commit/4e85b044902b5a32a0dcc6e1e5f4ff22bbaf0505

lein test and script/test passing but could definitely use more testing - and if anything fails - more test cases

further cleanup is possible if we unify JS module indexing instead of doing it separately as we are now

There might be a useful change to consider for the aget runtime logging. I think we are only getting the message and not the stack. I suspect this is due to str being applied to the exception.

I’ll try such a change locally and see what I get. I have bad aget somewhere occuring at runtime, but all I’m seeing is the "Error: Assert failed: (or (array? array) (js/goog.isArrayLike array))" portion, making it hard to track down.

@mfikes maybe unrelated, but in cljs-oops in dev-mode, I did extra work to make sure that assertions are properly reported pointing to user code at call site location - not somewhere into my library code

here’s the trick: https://github.com/binaryage/cljs-oops/blob/a356c548077bf499409f3485569147663a5793bd/src/lib/oops/codegen.clj#L283-L285

Yes. In our case, I think we may have the call to checked-aget at the top of the stack, and the rest of the stack being user code. (It is really just using assert)

for example in devtools console user will see link to your checked-aget, which is not that useful

if you don’t also print full callstack (which is verbose)

Ahh, right. A challenge too is this is all after macroexpansion. The line number appear to point back to the macro call site.

cljs-oops is also just a macro expansion, the trick is to generate js/Error. object at the call site and then throw it from within you lib code

https://github.com/binaryage/cljs-oops/blob/master/test/transcripts/expected/oget_dev.js#L22

but this might not be appropriate for cljs.core, just wanted to mention this technique

@mfikes where is that happening? re str on checked array error, I thought we call *print-err-fn* directly?

I tested this in the browser and I saw reasonable traces

(meaning I saw the caller)

I haven’t tracked it down. This might be something somewhere in the RN stack

yeah I’m pretty sure what’s being done is entirely reasonable for both Node.js & Browser

@dnolen When you tested, were you using Figwheel?

no, I’m not doing any testing with anything else and I don’t really intend to

I’m focusing on the fundamental surface area

(which is already huge)

That makes sense. I’m digging into Figwheel code and I see stuff that explains it I believe. Not a compiler problem. It seems to let the console log it, which would be RN, and then it attempts to serialize the exception back, which leads to issues:

ios:cljs.user=> (*print-err-fn* *e)
Figwheel: message from client couldn't be read!

@mfikes I just mean there’s a ton for me to QA with the base REPLs + the modules work

getting it to work will make it work downstream

I did one more careful read of the “Checked Array Access” sneak preview post and found one small error (it referred to unsafe-get instead of unchecked-get). Pushed a fix for that. Otherwise it LGTM.

@mfikes let me read over it one more time

Feel free to send nits to fix

@mfikes I submitted a bunch of small edits

mostly to provide slightly clearer narrative in the 1st half

Cool. Nearly finished updating them

@dnolen Updated. I took a stab at interpreting the change you had in mind surrounding the “Readers familiar with JavaScript will note…” revision.

(The other ones were pretty clear.)

cool!

looking

I added a commit nit too

Ahh, I see there are a couple of further changes I missed right near the end. What was the commit nit?

er commit -> comma

@mfikes there are 2 new comments on the latest PR

Ahh, add two comments. Got it.

Commas 🙂

Two commas have been added. I’m reading it again through to QA my changes.

I suppose with the removal of the change to js* to support parameters, “Again, the theme of successive refinement.” odd (the “again” part.)

reading over that again now

@mfikes agreed I would drop, I think the theme of refinement is clear just in the flow of the prose

Cool. I slipped it in with one word: “As time moved on, our friend aget (as well as aset) was refined to”

nice!

also pluralize point is right

alternatives

Cool. I see no other errors at this point.

@mfikes I say ship it

Me too. Go for launch of 3rd post

done

It is appearing now. Taking a look…

I don’t see any formatting errors 👍

Something recently put on master is causing something downstream in my RN project to complain about module-deps. Will bisect. It is not causing any behavior to break AFAICT. https://gist.github.com/mfikes/be782a414965cb3d2b2637c1ed29d54a

Another aside: Figwheel hot-load updates after changing files used to take 2 seconds for me with the 1.9.671 release but now take about 12 seconds with master (with :checked-arrays off). Sorry for sharing without a minimal repro.

The module-deps observation above is with the very last commit on master ((https://github.com/clojure/clojurescript/commit/2ba8ec76ec3977143afc5137e22c47838ac9d5a7) but not the prior.

I’m bisecting to find the 2 s -> 12 s slowdown

@mfikes we do index the top level of node_modules though that would surprise me if that was a problem

@mfikes great!

if you find a clue will divert some time today to look at it

Yeah, I again apologize for no minimal repro. Willing to try ideas locally if any are had.

I’ll also try to find out more.

@mfikes do you have :verbose on? if so you’d see if base node deps are getting installed in the case of your gist

I don’t have :verbose on. Will try that after the 2 -> 12 bisect

if you do then somehow Figwheel is skipping that step, and your error would make sense

bbiab

@mfikes the 2->12 problem might be related to us reindexing the node_modules directory on every file save?

I have a few more steps to go on the bisect. Will let you know.

@anmonteiro I suspect you are correct. The 2->12 problem: https://dev.clojure.org/jira/browse/CLJS-2238

@dnolen Negative result on :verbose shedding any light. It doesn’t appear to log anything relevant: https://gist.github.com/mfikes/8a4d210657fc8baa3064c869d5646520

I’ll dig locally to see if I can find something minimal

Might need to look at how Figwheel calls build?

One thing I’m wondering: Just because I have a node_modules and package.json file sitting in my current directory, does that mean that ClojureScript should assume I’m using it? (I suspect it is just there for the React Native packager to have fun with.)

(This is a bit of ignorance on my part—I didn’t build re-natal)

I think probably (node_modules) but should probably only index once, memoized like classpath libs.

It’s probably OK. I added some util/debug-prns to look at the sets involved in handle-js-modules: The requires set is stuff I recognize, like ClojureScript and some goog namespaces, taking maybe 2 inches to print in my console. The top-level set takes approximately 1/3 of a mile of console to print, and their intersection (the important bit, node-required), is emtpy. So it doesn’t seem to functionally hurt, and the perf would be fixed with good memoization.

In terms of counts:

requires 77
top-level 14748
node-required 0

I agree we should memoize it somehow

might be hard to do because the indexing code actually has side-effects

actually there’s pure code in there

@mfikes let me put together a tentative patch that you can try

@anmonteiro I was just about to commit something unless you’re already there

I was gonna start working on it right now

I was just going to name index-node-modules-dir -> index-node-modules-dir*

and then (def index-node-modules-dir (memoize ...))

separating the getting where we get the file-seq, to the part where we transform

same as the class path stuff

@dnolen I don’t think that’s enough

because the contents of node_modules should change

and we could handle that gracefully

right I don’t really care about that case

we don’t handle that for classpath libs

so I don’t know why we have to change expectations here

I’m pretty sure it doesn’t take long to get the fileseq

little trouble for good benefit

but not really

since it’s different expectations for different things

and that would have to be explained - better for it just be the same way

hrm so should I put something together for you to look at or don’t you see any value?

I’m just asking what your answer is for the fact that it will be different

and for very non-obvious reasons for many users

I guess we shouldn’t memoize impure code 🙂

might be a reason you don’t care about

I’m also ok with making it work, but then it’s nothing to point out

cool let me put something together

if we find it still has a significant perf cost we can simplify it to your approach

As an aside, I’m now pretty sure the module-deps stuff that gets logged (https://gist.github.com/mfikes/be782a414965cb3d2b2637c1ed29d54a) is somehow related, as it does 3 of them, each 12 seconds apart from each other

@anmonteiro let me do the first thing and have @mfikes test that

sure

and then you can work off that for your patch

sounds good

@dnolen there’s also another problem here as @mfikes pointed out

we’re shelling out to Node even if :npm-deps is not in the compiler options

in handle-js-modules

fixed here: https://dev.clojure.org/jira/browse/CLJS-2240

@dnolen I have a fix ready for CLJS-2238 using the approach I suggested

I was seeing this problem while developing Lumo too and this fix + CLJS-2240 solves it

reloads are instant now

ok!

let me attach the patch to CLJS-2238

@dnolen patch attached to CLJS-2238

@mfikes can you apply CLJS-2238 on top of current master and report back?

Yes

@anmonteiro oh yeah, ok that’s better, you’re just memoizing the provide computing

crawling the file seq isn’t actually slow

yep

just pushed that to master

@anmonteiro I’m assuming nothing cropped up around my resolve-var refactor

I looked over it and looks good

tried it and didn’t see any weirdness

great

@dnolen CI has also been green which is reassuring https://github.com/mfikes/clojurescript/commits/master

we have tests that cover the resolve-var thing

@anmonteiro Thanks for being kind to my 5-yo computer’s CPUs 🙂

I’ll take that as the patch is working 🙂

👟

Yes it is very fast now

:running:

great. I’m glad you brought it up, I had noticed it yesterday but failed to open a ticket

just got (require '["lodash/array" :as lodash-array]) working in the standard Node.js REPL

about to try that with browser REPL

@anmonteiro hrm, looks like there might be something wrong with maybe-install-npm-deps!?

when I try with an uberjar doesn’t seem to create node_modules

oh …

because of missing package.json

argh

@dnolen I think if you can upgrade to NPM 5 that won’t be an issue

@anmonteiro do you see a problem with just creating it temporarily if it doesn’t exist?

that’s what I was about to do

it’s a real usability bummer otherwise

if it doesn’t exist we could just output one that has {}

not sure if we should cleanup after ourselves or not

yeah that’s what I’m thinking

I think just creating it is fine now that you mention it

@anmonteiro hrm I don’t really under what your gen-user-ns stuff does when it’s handed a form

first glance seems like it couldn’t work at all

I think we made it work for some Figwheel usecase

(gen-user-ns '(require '["lodash/array" :as lodash-array]))

@dnolen there’s certainly a comment here https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L3756-L3758

exception

right but the comment doesn’t make any sense

regarding that case

I agree

since it just doesn’t work

🙂

I don’t really remember why that’s there

well which part? 🙂

ok

I think what’s there assumes src is a string naming a file

yes

so I think gen-user-ns needs to handle sequential? case

why do we need that case?

out of curiosity

because require forms at the REPL

in order to handle modules correctly we need to convert require into ijs by calling parse-ns on result ns* form

and then call handle-js-modules with that

this has to be done before emit on the AST

because if you don’t do that, all the load-lib stuff breaks

this is all simpler in Lumo for the obvious reasons 🙂

obviously

yeah I get it

Node.js REPL + node_modules is already working w/ Uberjar BTW - it’s sweet

this bit is for browser REPL, but of course everyone downstream wins for free

awesome

so close …

@anmonteiro another question what was the rationale for the alias duplicate check

some of these things are really annoying since they prioritize compilation over REPL

like the constant in file check

the alias duplicate check was actually meant for the REPL

where you shouldn’t be able to require something with an alias that is already aliasing another namespace

Clojure does it and I thought we should do that too\

maybe there’s a bug if you’re getting that error?

or are you trying to use the same alias for one that already exists?

@anmonteiro I think maybe you didn’t check for harmless case?

the exact same alias twice?

@dnolen

ClojureScript Node.js REPL server listening on 50913
To quit, type: :cljs/quit
cljs.user=> (require '[clojure.string :as s])
nil
cljs.user=> (require '[clojure.string :as s])
nil
cljs.user=>

looks like I did

but then if I do (require '[clojure.spec.alpha :as s]) it’ll throw an error

I’m sure I specifically checked for that case

this should also be idempotent, across several analysis passes

the problem here is pretty circuitous, one second

@anmonteiro here’s the problem, I’m thinking about disabling that thing and if you want to figure out a way to bring it back you can

short description

at the REPL you don’t know anything - there are no source files

there are no inputs, there are no known requires, no known provides

in order to even know that somebody wants to loads a module you must process the AST at least twice

first time to figure out that you have ns* or ns

(but this is a side effect, you’re going to record incorrect information)

we then preprocess JS modules which updates everything to the real names

we need to parse the ns form again to get the REAL one

but the dupe check fires

there’s lot of gross ways to deal with this but I’m not really interested

one option would be to suppress the dupe check the second time via dynamic var

Maybe will go with that and cleanup later

@dnolen so you’re saying that we may have 2 requires (2 passes, really) that are really trying to load the same namespace but it gets different names?

@anmonteiro it gets different names because don’t know what name Google Closure is going to invent during process-js-modules

that makes total sense

this is already true in build compiles - but it’s less of deal there because process-js-modules happens before you compile anything else

boom, https://github.com/clojure/clojurescript/commit/1d16bc50a33a6e26bebf20b4cac14a2ede373421

still more work to do though 😛

https://dev.clojure.org/jira/browse/CLJS-2241

^ @dnolen this looks like you’re processing a file through Closure under Node.js ?

no browser REPL

when really if you have :npm-deps + :target :nodejs it should just emit a js/require

module$Users$davidnolen$development$clojure$hello-world$node-modules$lodash$array

^ this is a name that Closure would give

we use namespace-name$node$module$lodash_SLASH_array

without any investigation I would say that somewhere we’re not binding :target :nodejs 🙂

e.g. in (:options @env/*compiler*)

@anmonteiro you’re missing what I’m saying

I can look closer in 1 hour or so

this is browser REPL 🙂

oh

there is no Node.js here

no browser REPL

missed a comma there 😛

heh yeah

sorry

this is working and it’s pretty amazing - just need to sort out the corner cases

Node.js REPL is done

awesome

oh yeah I should have read the minimal repro

I mean I think we could argue it’s easier to use Node modules interactively from ClojureScript in a browser than it is in JavaScript

also all this stuff is hot-loading incremental compile

triggered by REPL interactions

Pretty sure it's been said before, but you are all killing it lately - amazingly inspiring to see this much work and love go into ClojureScript!

aaand Node.js require in browser is idempotent

oh you figured out CLJS-2241?

yeah

nice

this is blowing my mind 🙂

I don’t understand your comment though

> need new AST after we know what the modules are

but seems like the commit is removing 1 analyze pass

@dnolen I wonder if this fix breaks something else

I think that 2nd analysis was there for a reason

specifically https://github.com/clojure/clojurescript/commit/8524d7b1ac7c1c418ec394e89322e341070414ca

Did anyone else get any emails from Kyle Pennell at Stackshare about the CircleCI article? I got an automated one from having contributed to CLJS

I did

got one for Om too

me too

Got one too, very spammy I guess

it is pretty clever, to scrape repo contributors and then get their emails from github web or git commits

@anmonteiro I don’t think so, pretty sure vestigial as well

cool. I don’t understand the fix though

how does removing that analyze call fix the issue?

@anmonteiro we didn’t need that one it was suppressing a bunch of stuff

no-warn, no analyze deps, etc.

and it solely fixed the issue?

@anmonteiro yeah

@anmonteiro it’s was trying to analyze the transformed module (which isn’t possible currently)

I agree it might make sense to make that work

but I’m trying to get through the big problems first

you’re just leaving me with more questions 🙂

not that it’s a bad thing

but what do you mean by analyzing the transformed module?

the closure namespace?

Loving the fact that function bodies are not printed now btw

@anmonteiro yes the processed closure namespace

is the fact that they’re printed as #object[add] to maintain compat with Clojure?

more or less

gotta run, bbiab