Fork me on GitHub
#cljs-dev
<
2017-07-13
>
anmonteiro00:07:53

it’s impressive what we did in the last 12 days though: https://github.com/clojure/clojurescript/compare/r1.9.671...HEAD

anmonteiro00:07:57

since the last release

anmonteiro04:07:57

@dnolen fixed the :rename support for JS modules in CLJS-2217. But please apply CLJS-2220 before because CLJS-2217 depends on the test NS that it introduces

Roman Liutikov11:07:41

Testing master right now. Should code splitting work with :npm-deps?

Roman Liutikov11:07:46

Getting No :out-file for IJavaScript #cljs.closure.JavaScriptFile error

Roman Liutikov11:07:29

works fine under adv optimizations, but not with :none

dnolen12:07:36

@roman01la make something minimal file an issue

dnolen12:07:01

@juhoteperi does :global-exports have the same :rename support issue?

juhoteperi12:07:52

@dnolen I think @anmonteiro's patch fixed global-export also

juhoteperi12:07:06

But it doesn't have a test

dnolen13:07:49

@juhoteperi thanks noted on the issue

dnolen13:07:37

yesterday’s post did pretty well, pretty much double the traffic from the code splitting post

Yehonathan Sharvit13:07:27

David, can you share the post url again?

dnolen13:07:13

one interesting data point is that Quick Start appears to get ~4000 views a month and the engagement numbers are quite high, average time on page is nearly three minutes. I definitely think there is a pretty big engagement opportunity here by providing a “Try It” page with bootstrapped REPL.

richiardiandrea13:07:19

I really like the above idea, bootstrap is one of those things that make people go "Ooooh" 😄

mfikes13:07:44

The replumb library (driving http://clojurescript.io) is probably a good starting point, in terms of maturity, I’d speculate

richiardiandrea14:07:00

@mfikes no I haven't had time to update replumb and it actually failed at its goal

richiardiandrea14:07:24

I don't think many folks needed to have a common lib for that

richiardiandrea14:07:51

the largest bootstrap applications (planck, lumo, klipse) all use custom implementations

mfikes14:07:05

So, if Quick Start had a bootstrapped REPL Try It link, it could be a bespoke one built for it.

dnolen14:07:46

yeah it’s not that hard, I don’t think we need a third party project for this 🙂 Probably we’ll just need some help from Alex Miller about building the ClojureScript we need and getting it copied to the right place when we deploy.

mfikes14:07:19

I think you are right, though, if there were a big Try It! button in the menubar on the Quick Start page (that essentially looks like Get Started! button on the main page), lots of people would immediately get a taste of it 🙂

anmonteiro15:07:49

@dnolen I'm happy to add the global exports tests either with that patch or as a follow-up

anmonteiro15:07:10

I'm more interested in the feedback for CLJS-2220

anmonteiro15:07:46

i.e. do we want to rely on :npm-deps for running runtime tests

anmonteiro15:07:03

I see no problem with it but thought you might have opinions

dnolen15:07:00

@anmonteiro no opinions other than this seems necessary? 🙂

dnolen15:07:44

adding a global exports test to existing patch would be nice

anmonteiro15:07:03

Yesterday's post is featured in this week's Node Weekly that goes out to 37k subscribers: http://nodeweekly.com/issues/196

mfikes16:07:00

woo hoo!!!

anmonteiro16:07:26

@dnolen augmented CLJS-2217 with tests for rename + global exports

anmonteiro16:07:53

(just leaving another note to please apply CLJS-2220 before CLJS-2217)

dnolen17:07:40

@anmonteiro all your patches are applied

anmonteiro17:07:49

@dnolen there’s still a trivial one that enables JSC under test-simple again https://dev.clojure.org/jira/browse/CLJS-2219

dnolen17:07:06

@mfikes sorry looks like CLJS-2198 still needs rebase

anmonteiro17:07:19

I think there are no problems enabling it again, and running test-simple was useful to figure out the module stuff was working before adding externs

mfikes17:07:21

No problem. Will rebase it again

dnolen17:07:47

@mfikes perhaps we should go live with yours tomorrow, @juhoteperi’s on Monday, and I will do a final post on :global-exports on Wednedsay before EuroClojure?

mfikes17:07:57

Sounds good.

mfikes17:07:22

@anmonteiro Dunno if you are aware, unit tests failing

mfikes17:07:32

There is a little ambiguity as to when it started failing: https://github.com/mfikes/clojurescript/commits/master

anmonteiro17:07:06

hrm.. works on my machine™

anmonteiro17:07:39

I know the fix

darwin17:07:14

looking at https://github.com/clojure/clojurescript/wiki/Building-the-compiler, what is the right thing to build on canary? script/build or script/uberjar?

mfikes17:07:14

lein test is also showing things like:

actual: java.lang.IllegalArgumentException: No implementation of method: :-find-sources of protocol: #'cljs.closure/Compilable found for class: cljs.build.api$inputs$reify__6717

dnolen17:07:33

@mfikes you have a dirty target directory

anmonteiro17:07:40

just needed to fix travis.yml

darwin17:07:20

@dnolen ok, thanks, I’m going to add some new options to that script, need to run maven in batch mode and make it less verbose (hitting travis output limits), all will be optional driven by env flags

dnolen17:07:23

@anmonteiro that patch looks unnecessarily large?

dnolen17:07:57

(meaning it doesn’t look like the right patch :)

anmonteiro17:07:12

ran a bad command

anmonteiro17:07:16

@dnolen 1 insertion, 1 deletion now

anmonteiro17:07:18

that’s more like it

mfikes18:07:39

I wonder if somehow including "assert" in native-node-modules is causing the self host tests under node to wig out. Hrm.

mfikes18:07:33

There are lots of assert complaints popping up.

anmonteiro18:07:21

yeah, assert might be a problem hrm

anmonteiro18:07:44

I think we should just remove it from the native-node-modules list

dnolen18:07:01

@anmonteiro I don’t understand why it’s problem?

anmonteiro18:07:33

I think we’re compiling the self-host/ self-parity test runners with :target :nodejs

anmonteiro18:07:52

and assert is somehow taking precedence in resolve?

anmonteiro18:07:06

may actually not be a good assumption

anmonteiro18:07:17

let me repro locally and I’ll have more info

anmonteiro18:07:09

meanwhile CLJS-2223 fixes a warning in self-host

dnolen18:07:14

if that’s true this just sounds like a bug and we should address the root cause

anmonteiro18:07:43

agreed, looking into it

dnolen18:07:11

it may show the module as fn support is broken (we need to guard - should only work if :require’d)

dnolen18:07:44

I also think we may need to move those cases into the final branch

dnolen18:07:58

user defs should always take precendence

anmonteiro18:07:07

that’s definitely the problem btw

dnolen18:07:22

yeah so I think we need to do both of the things I suggested

dnolen18:07:52

and test cases 🙂

dnolen18:07:22

feel free to open a blocker issue for this

anmonteiro18:07:09

@dnolen I don’t know if we can move these to the final branch?

dnolen18:07:20

I don’t see why not

anmonteiro18:07:26

we need to bottom out somewhere

dnolen18:07:37

yes but you have to do this when you bottom out

anmonteiro18:07:38

I think user-defined vars should take precedence, so move those up?

anmonteiro18:07:51

I’ll gather more info and get back to you

dnolen18:07:53

no because core names resolve

dnolen18:07:08

(at least that’s what I recall)

dnolen18:07:14

so it should user -> core -> module fn

anmonteiro18:07:35

I’m talking about the :else branch in resolve-var for context

dnolen18:07:17

in the final branch we fallback to the current ns if not core name

dnolen18:07:27

so if not core name, that’s when we check for module fn case

dnolen18:07:48

then if not, then resolve to current-ns

dnolen18:07:50

(sorry I got my order wrong)

anmonteiro18:07:02

I didn’t see the core-name? call

dnolen18:07:35

heh, it’s funny how much you can remember when you’ve been working on this stuff too long 😛

anmonteiro18:07:37

@dnolen just to confirm, in that last case order should be: 1. core name 2. user-defined (current ns) 3. module

dnolen18:07:52

1. user-defined 2. core name 3. don’t know, current-ns is the current order

dnolen18:07:54

now it should be

anmonteiro18:07:14

oops got those wrong

dnolen18:07:16

1. user-defined 2. core name 3. module fn if require’d 4. don’t know current-ns

mfikes19:07:07

I’ve updated the aget post https://github.com/clojure/clojurescript-site/pull/101 to reflect the latest patch in https://dev.clojure.org/jira/browse/CLJS-2198, set with a tentative date for tomorrow release, etc.

mfikes19:07:11

@dnolen I’ve confirmed (somewhat manually) that the -v11 patch in CLJS-2198 doesn’t cause the build to fail. I suppose if the stuff discussed above is sorted first, I can re-baseline and attach a -v12

mfikes19:07:59

@darwin If working on Canary, just wanted to let you know I sorted the Planck build failures. TL;DR: Planck master should build fine with ClojureScript master.

darwin19:07:48

@mfikes nice, will get to it in a day or two

darwin19:07:35

btw. canary is able to build clojurescript jar, now figuring out how to determine what was actually built and how to “upload” it somewhere: https://travis-ci.org/cljs-oss/canary/builds/253347045

darwin19:07:46

the idea is to have another branch “builds” or similar, which will be used as a storage for all jars, at least for now

dnolen20:07:17

@mfikes array checking landed in master

dnolen20:07:30

I played around with it quite a bit - I must say - very cool

anmonteiro20:07:03

I’m glad we found out the resolve bug before the release

anmonteiro20:07:21

this was actually here before, and it’s a pretty bad bug 😛

dnolen20:07:49

npm install

dnolen20:07:54

suddenly everything stops working

anmonteiro20:07:58

submitting the patch soon, I have fix & test

anmonteiro20:07:17

we don’t ever do just npm install do we?

dnolen20:07:26

no I meant if someone else did that

anmonteiro20:07:27

we’ll always install the required deps

dnolen20:07:37

and happened to get a module w/ a var name

anmonteiro20:07:38

I took it literally 🙂

anmonteiro20:07:14

getting a bunch of aget warnings with master but I think this is expected

mfikes20:07:09

@anmonteiro The only ones that are expected are logs associated with the tests that cause them. (Which we can probably squelch if we mess with js/console)

anmonteiro20:07:24

is cljs.array-access-test supposed to throw a number of errors?

anmonteiro20:07:28

running test-self-parity

mfikes20:07:02

Yes… I think it is too noisy. It goes down a path that calls js/console.warn and maybe we can block those for the duration of that test namespace

mfikes20:07:15

I’ll investigate

anmonteiro20:07:16

I don’t mind

anmonteiro20:07:30

as long as I get 0 errors 0 failures in the end 🙂

mfikes20:07:33

Yeah, but when you see it your immediate reaction is to think the tests failed

anmonteiro21:07:15

found CLJS-2226 from a HN comment 😛

anmonteiro21:07:35

will be useful for orgs to support @org/package-name format

anmonteiro21:07:57

some other people already publishing packages with those names too

dnolen21:07:53

I’m just thinking I should delete this stuff from analyzer? (if it was already)

anmonteiro21:07:57

not off the top of my head

dnolen21:07:19

oh you already addressed it OK

anmonteiro21:07:19

I think @juhoteperi deleted every remain of missing-js-modules

dnolen21:07:37

well alrighty

dnolen21:07:09

I’m sure will find more minor issues but wrt. features I think we’re set

dnolen21:07:31

I’m loathe to really look at any new or other outstanding tickets as this release will undoubtably create some churn

anmonteiro21:07:55

I think CLJS-1955 was also slated for this release

dnolen21:07:57

but if there’s something simple that people want to see will consider

anmonteiro21:07:11

your decision whether it makes it or not

dnolen21:07:14

oh you fixed that!

anmonteiro21:07:25

hah yeah 🙂

anmonteiro21:07:14

@dnolen replaced the patch

anmonteiro22:07:01

@dnolen when are you thinking about releasing the next version?

anmonteiro22:07:15

I’d love to kick the tires on all this stuff during the next week

anmonteiro22:07:22

before releasing

dnolen22:07:27

probably on the eve of EuroClojure?

anmonteiro22:07:44

sounds good to me

dnolen22:07:54

yeah not tomorrow 🙂

anmonteiro22:07:57

I just want the weekend to play with it 🙂

dnolen22:07:59

we have some more posts to write

anmonteiro22:07:08

and bring Lumo up to date with all the new stuff

anmonteiro22:07:36

(and submit any patches that may be needed to make string-requires work there)

dnolen22:07:16

also now that we have a unified :require for non true foreign libs, indexing JS requires and doing basic usage validation is starting to sound compelling

dnolen22:07:28

something for later

mfikes22:07:29

Maybe in a year or two we can solve the React Native Packager problem 🙂

anmonteiro22:07:51

what is the React Native Packager problem?

dnolen22:07:23

we could probably solve that sooner .. ModuleLoader is a thing in Closure, we could provide one for RN

dnolen22:07:35

or least supply the hook for it

mfikes22:07:37

It essentially gets in between you and React Native, causing issues. One was that it translates JavaScript using a Facebook-proprietary include mechanism.

dnolen22:07:53

I think that’s the primary problem

dnolen22:07:00

(but maybe there are others?)

dnolen22:07:08

we also need a basic :npm-deps guide at some point

dnolen22:07:41

oh we have an issue for that I see

mfikes22:07:49

Part of my issue with the React Native Packager is actually just a lack of knowledge about it

mfikes22:07:15

FWIW, I did try :npm-deps with React Native, and got it partially working

mfikes22:07:35

That can all wait for another day 🙂

anmonteiro22:07:09

@mfikes I’d be happy to pair with you to get some of that stuff figured out someday

mfikes22:07:33

Like any of this stuff, it is all ultimately solveable. But re-natal works for now. 🙂

anmonteiro22:07:46

right. introduce as few variables as possible

dnolen22:07:44

feel free to add anything missing that needs updates that will land next week

juhoteperi22:07:33

Btw, re EuroClojure, anyone here going to attend?

juhoteperi22:07:31

Still undecided myself, the timing is quite inconvenient.

dnolen22:07:44

testing that

dnolen22:07:13

too close to ClojuTRE?

juhoteperi22:07:08

It is middle of Finnish summer vacation

juhoteperi22:07:56

I don't have much problem with that myself, but many will have plans with families etc so much less Finns will be attending this year than usual

dnolen22:07:22

@mfikes I have an idea for CLJS-2227

dnolen22:07:29

finally a use for \print-err-fn\

mfikes22:07:54

Hah. Yeah. 🙂 That would do it too

mfikes22:07:28

And, it might be easier for users of the feature to squelch chunks of code on their own.

dnolen22:07:33

yeah this is trivial and way better

dnolen22:07:44

it already works when user calls enable-console-print

dnolen22:07:53

and *print-err-fn* is raw

dnolen22:07:05

so no CLJS printing of args

mfikes22:07:05

It is good we have a week to bang on all of this stuff (it takes a while to polish the corners and get the ergonomics right)

dnolen23:07:22

@mfikes going to suppress all the checked array compile time warnings as well

mfikes23:07:40

Ahh. Cool.

dnolen23:07:43

basically introducing an alias

dnolen23:07:01

(defn checked-aget-alias [& args] (apply checked-aget args))

dnolen23:07:05

so we only test runtime

dnolen23:07:22

which is all we care about with these tests anyway

mfikes23:07:38

I’m digging into using this stuff with core.async, and I’m thinking that core.async might be right and the new diagnostic wrong. The tag list is [objects number]. Need to figure out what objects is.

dnolen23:07:58

right there are these array type alias in Clojure

dnolen23:07:01

we should probably respect that

dnolen23:07:09

^objects is one

mfikes23:07:27

I’ll try a change and see if it quiets core.async down

dnolen23:07:30

^bytes, ^booleans ^strings maybe a few others

mfikes23:07:58

Ahh. Right.

dnolen23:07:36

I can probably fix that while I’m at it

mfikes23:07:49

I’ll try objects locally and let you know

dnolen23:07:55

quieter tests in on mater now

dnolen23:07:15

doing the general fix over here

mfikes23:07:12

Cool. The only remaining warnings I see now appear all be legit. For example: https://github.com/bhauman/lein-figwheel/blob/f772e351a2e2a260cdeca6a4958fb3643507a2f2/support/src/figwheel/client/socket.cljs#L8 causes this diagnostic

WARNING: cljs.core/aget, arguments must be an array followed by numeric indices, got [js string] instead (consider goog.object/get for object access) at line 8 target/ios/figwheel/client/socket.cljs

juhoteperi23:07:00

In this case window is Object but what if it was Array? The compiler can't know what is the type of of js/ var??

dnolen23:07:20

@juhoteperi we allow js stuff to pass

mfikes23:07:32

Did you just add that to the general set of symbols?

mfikes23:07:11

If so, this will trigger simply because the 2nd arg being string and not numeric

dnolen23:07:19

yeah it’s not js here

dnolen23:07:21

it’s string

dnolen23:07:28

@mfikes master updated

mfikes23:07:12

@dnolen Master seems to be inappropriately emitting disgnostics for any now. For example:

WARNING: cljs.core/aget, arguments must be an array followed by numeric indices, got [any number] instead at line 84 target/ios/cljs/core/async/impl/timers.cljs

mfikes23:07:02

@dnolen Master is good. This is with a project using Om Next as well (no static diagnostics triggered). I also tested a Reagent project (no static diagnostics there either).

dnolen23:07:26

@mfikes nice, now we’ll see if people’s tests fail at runtime with those checks enabled 🙂

mfikes23:07:56

Yes, I’m down to a small enough number where I can now go from :warn to :error

mfikes23:07:10

Or just leave it at :warn and keep my eyes peeled.

dnolen23:07:11

might be a good requirement for cljs-oss to build libs with array checking on

mfikes23:07:26

We could be annoying and make it print ^G

mfikes23:07:09

This feature is bound to annoy some people anyway without it beeping.