Fork me on GitHub
#cljs-dev
<
2017-06-16
>
mfikes00:06:43

Seeing if I can extend CLJS-2085 to cover extend-protocol as well (via CLJS-2090).

mfikes00:06:44

^ CLJS-2090 is not an issue. Closed it.

cgrand11:06:45

In bootstrap, when at the repl, one can’t require a namespace fully created through the repl. The minimal test case is:

cljs.user=> (require ‘cljs.user)
No such namespace: cljs.user, could not locate cljs/user.cljs, cljs/user.cljc, or Closure namespace “cljs.user”

cgrand11:06:40

issueing (swap! cljs.js/*loaded* conj 'cljs.user) beforehand is a workaround

cgrand11:06:49

but it becomes hairy when you want to define macros at the repl

mfikes11:06:05

Or at least a variant of it

rauh12:06:46

How do I run the tests in src/test/clojure/cljs? In particular cljs.compiler-tests. Currently running from the REPL.

cgrand13:06:39

@mfikes thanks I somehow missed it while reviewing the existing issues. Definitely similar. I haven’t traced the code enough to know if they depend on the same codepaths (`cljs.js/*loaded*` is quite specific to bootstrap)

mfikes13:06:18

@cgrand Yeah, fixing the issue everywhere might involve two separate solutions

rauh13:06:23

@mfikes Thanks, hadn't actually seen that. For some reason I was looking for something in ./scripts/

cgrand13:06:43

@mfikes plus (defmacro ...) at the REPL won’t ever (for reasonable values of “ever”) work in “regular” cljs, while in bootstrap....

mfikes13:06:23

@cgrand Right. I documented what I know here: http://blog.fikesfarm.com/posts/2015-09-07-messing-with-macros-at-the-repl.html If you find more, please share 🙂

cgrand13:06:35

yeah, that’s what I discovered by myself too (but you shew me the ropes months ago)

mfikes13:06:00

I’m updating that post: (defmacro ...) now returns the macro var 🙂

cgrand13:06:05

returns true atm for me

cgrand13:06:32

wild idea: when defmacro in a non $macros ns, create the ns on the fly and with a ns obj which has the non-macro ns obj as prototype.

cgrand13:06:47

(or patch name resolution for $macros ns to lookup in the regular ns)

cgrand13:06:54

(the prototype is too hacky)

mfikes13:06:26

I think there is a strong desire to preserve semantics between bootstrap and regular JVM ClojureScript. (Otherwise there is essentially a fork where codebases target either.) We know, for subtle edge cases, this has already occurred already anyway. What I find philosophically interesting is that ClojureScript lets you turn the knob to two extremes: - The AoT compilation model, massive code minimization, DCE, etc., all geared towards the browser use case. - A dynamic self-hosted model, where code size is not as important, More like Clojure and classic Lisp I actually target non-browsers more (with iOS work, and Planck), the first model is not so important to me, and I almost feel like it is an unfortunate premature optimization that could not be avoided

cgrand13:06:56

Inconsistent resolution

cljs.user$macros=> foo
WARNING: Use of undeclared Var cljs.user$macros/foo at line 1
nil
cljs.user$macros=> `foo
cljs.user/foo

mfikes13:06:02

While the above is true, I think it is generally OK and matches what happens in JVM ClojureScript for a lot of code: foo could refer to a Var in the Clojure (maybe it is a Clojure function defined in the same namespace and called during macroexpansion). If you syntax quote, it is oftentimes used to generate code that refers to Vars in the runtime namespace. But, IIRC, cljs.user/foo could resolve to a Var in the macro namespace if one is there. (Corner cases abound.)

mfikes13:06:54

This ticket is related to the above as well https://dev.clojure.org/jira/browse/CLJS-1773

dnolen14:06:27

@rauh going through a bunch of tickets w/ patches today and I see you have a few - your name still isn’t showing up on the Contributor list, you said you sent in your CA right? What name did you use?

rauh14:06:06

@dnolen Yeah I def sent it a few months ago. It should be "Andre Rauh"

rauh14:06:41

Should I get an email confirmation or some sort?

dnolen14:06:53

@rauh no I don’t think so your name should just show up

rauh14:06:21

I can fill it out again if needed.

Alex Miller (Clojure team)14:06:19

@dnolen I can confirm we have Andre's CA

Alex Miller (Clojure team)14:06:59

We got it on May 20 but I think it's been a while since they've been updated

Alex Miller (Clojure team)14:06:22

Usually it happens about once a week but I think we are behind

dnolen14:06:46

@rauh I’ve bumped your JIRA permissions, you should be able to freely edit tickets now

rauh14:06:03

@dnolen Great, thanks!

dnolen15:06:51

If you use ClojureScript + Node.js please try this patch and report on the ticket https://dev.clojure.org/jira/browse/CLJS-1990

dnolen15:06:14

this fix looked good to me but probably should probably test your projects with master https://github.com/clojure/clojurescript/commit/64f126f136270492555b5afe5a6947646a36bdc4

rauh15:06:01

dnolen: Tested master just now on my project. Everything working fine.

dnolen15:06:26

@rauh great, thanks for the report

tmulvaney17:06:18

I forgot all about this patch. Glad its ok

rauh15:06:12

Just added a patch and a few test in https://dev.clojure.org/jira/browse/CLJS-2046 . Would appreciate if somebody could take a quick look/review. (My first real excursion into the analyzer)

rauh15:06:28

(Prerequestite for https://dev.clojure.org/jira/browse/CLJS-2041 which avoids .call(null ...) constructs)

dnolen15:06:54

@mfikes I thought we supported ^:const pattern but I can’t remember where we make this work?

mfikes15:06:14

It affects case

mfikes15:06:11

It also prevents redefinition. (There must be a guard somewhere in the analyzer.)

favila15:06:22

does that work in Clojure?

bronsa15:06:27

I really don't like that cljs case works with const

bronsa15:06:31

no clojure doesn't do that

favila15:06:47

I assumed case with symbol was always literal

bronsa15:06:47

it's an odd difference

bronsa15:06:21

and means you can't really use a case with symbols w/o having knowledge about the surrounding context

mfikes15:06:25

Yeah, it is unfortunately one of the things listed here https://clojurescript.org/about/differences#_special_forms

dnolen15:06:36

@mfikes well let’s set aside case for now 🙂 so we don’t have generalized ^:const support right?

mfikes15:06:30

The only two specific effects of ^:const are re-def and case. Nothing generalized that I can think of.

dnolen15:06:56

so we don’t actually inline the value?

mfikes15:06:14

Ahh. I don’t think so…

mfikes15:06:37

Inlining in Clojure is listed as a difference on the “differences” page.

mfikes15:06:35

Seems like a reasonable enhancement (in line with what people might expect)

dnolen15:06:44

I think the only limitation of Clojure ^:const is that the value must be a EDN literal correct?

bronsa15:06:52

IIRC it has some gotchas around metadata too

mfikes15:06:59

“Clojure Programming”, pg. 200 says “any references to a constant var that aren’t resolved at runtime (as per usual); rather, the value held by the var is retained permanently by the code referring to the var when it is compiled”

dnolen15:06:08

@bronsa like meta on the literal?

bronsa15:06:26

yeah, not at a keyboard rn but I'll check later

dnolen15:06:31

https://dev.clojure.org/jira/browse/CLJS-1965 this is a pretty simple looking enhancement, worth testing out

bronsa15:06:55

nvm the meta thing I remember was a bug that got fixed a few versions ago

dnolen15:06:34

breaking for a bit of lunch, if there’s any ticket + patch you will like to see get pushed through let me know

dnolen15:06:17

k will take a look when I get back

anmonteiro15:06:38

@dnolen I think I’ve solved something similar in Lumo recently, mind if I assign this to myself? https://dev.clojure.org/jira/browse/CLJS-1959

anmonteiro16:06:12

it’s a breaking change for people who’ve started using the shorthands you introduced (e.g. :es5, :es6)

rauh16:06:28

@anmonteiro I think str/replace takes care of that, no?

anmonteiro16:06:00

didn’t see that, sorry

anmonteiro16:06:42

sorry for the noise

dnolen16:06:25

@anmonteiro yeah I was confused by that too 🙂

mfikes16:06:57

A string? assertion is triggering for me with the unit tests (perhaps the recently added one for CLJS-1891, but I haven’t isolated it yet)

dnolen17:06:11

@anmonteiro I think that CLJS-1959 should piggieback on ^:const inlining?

anmonteiro17:06:30

@dnolen I think they’re unrelated and we could achieve it by changing how we bootstrap node

dnolen17:06:30

I don’t know for people who want to optimize the dump there’s better things to do

mfikes17:06:47

@dnolen I don't know on CLJS-1601. I believe Nikita converted Quil to to support bootstrapped and perhaps he came up with a use case where it matters.

mfikes17:06:53

On CLJS-1625, I did have that recent perf optimization https://dev.clojure.org/jira/browse/CLJS-2066 that does smell awfully similar. I'll see if I can confirm whether they ended up being the same.

dnolen17:06:32

@mfikes I’m pretty confident your work fixed 1625, it’s exactly what you covered

dnolen17:06:27

@rauh I’m going to close https://dev.clojure.org/jira/browse/CLJS-2041 unless you can show that CLJS-1630 is covered

dnolen17:06:45

I’m pretty sure we need absolutely need .call for users who want to make JS primitives callable

dnolen17:06:09

I might be wrong, but we should look into that

rauh18:06:59

@dnolen Please don't close yet. I really don't see how .call is needed. I don't understand CLJS-1630.

dnolen18:06:22

hrm sorry I guess that’s a runtime thign

mfikes18:06:33

Ahh, right. Yes the 2^n thing with for loops was at analysis time. It would be interesting if anyone had access to Android Chrome to repro

dnolen18:06:30

https://dev.clojure.org/jira/browse/CLJS-2095, low hanging fruit for someone 🙂 Nashorn runner

dnolen18:06:46

means we could ship a default test runner etc.

mfikes18:06:48

Whatever was going on with CLJS-797, expressions like this:

mfikes18:06:05

(for [a [[[[[[[1]]]]]]]]
  (for [b a]
    (for [c b]
      (for [d c]
        (for [e d]
          (for [f e]
            (for [g f]
			  g)))))))

mfikes18:06:25

Are very expensive; there is a doubling effect in the amount of JavaScript emitted

rauh18:06:51

@dnolen I stand by my opinion that the .call is not needed. I'll comment on the tickets with a longer response. An interesting side finding that might need clarification: 1. If I extend-protocol not-native or native, and specify some IFn, let's say arity 0, at some point and then later at some other point 2. Again extend-protocol the same type, with let's say arity1, then the prototype has both IFn for arity 0 and 1, but the .call method will ONLY be able to handle the latter defined one. So they diverged 😕

rauh18:06:17

No clue what clojure does here.

anmonteiro19:06:13

^ I’m almost certain this covers the Node.js behavior for setting __dirname and __filename

anmonteiro19:06:20

this is something that has been on my mind for a while since you refactored the way we bootstrap Node.js

dnolen19:06:49

@anmonteiro ok that looks great, I tried something like that but I had trouble getting it to work with :npm-deps

dnolen19:06:54

so this works just fine with that?

darwin19:06:28

trying to run clojurescript compiler tests after a while via ./scripts/test and getting Unable to find static field: FUNCTION_PARAMS in class com.google.javascript.jscomp.DiagnosticGroups, compiling:(cljs/closure.clj:101:1)

darwin19:06:45

sounds like some incompatibility with closure compiler

darwin19:06:58

I’ve done ./scripts/bootstrap

darwin19:06:41

current master, macOS 10.12

dnolen19:06:11

@darwin been running tests all day after re-bootstrap, I don’t see that

dnolen19:06:17

make sure your lib/ isn’t dirty

thheller19:06:24

@anmonteiro + lineOffset: 0, should be 1 in your CLJS-1959 patch

thheller19:06:28

you are adding 1 line

thheller19:06:39

otherwise source maps will be off by one

darwin19:06:40

ok, going to start from scratch

thheller19:06:59

oh wait you are not actually adding a line

darwin19:06:49

ok, I’m good, ./scripts/clean and ./scripts/bootstrap fixed it

rauh19:06:32

Looks like clojure removes previously defined arities of the protocol:

(defprotocol Foo (foo [a] [a b]))
(extend-type Keyword Foo (foo [a] 0))
(extend-type Keyword Foo (foo [a b] 1))
(foo :a) ;; Error
Strictly cljs should then clear all other arities for correctness, but I don't think anybody wants that code emitted every time extending multi arity protocols. So I vote to ignore this issue.

rauh19:06:08

@dnolen https://dev.clojure.org/jira/browse/CLJS-2042 should also be low risk. Also applies cleanly

anmonteiro19:06:17

@thheller I’m not adding a new line

anmonteiro19:06:59

oh you said it

anmonteiro20:06:28

@dnolen I think I’m getting some weird failures with :npm-deps, nice catch

anmonteiro20:06:32

investigating

dnolen20:06:59

@anmonteiro yeah I burned a lot of time on it - my experience was that Google Closure compiled modules is fundamentally at odds with Node style CommonJS modules

dnolen20:06:16

this is why I was suggesting magic constants

anmonteiro20:06:20

yeah.. Closure expects global scope to be available

anmonteiro20:06:25

or rather… local scope

dnolen20:06:48

as far as I can tell magic constants completely solve the problem

dnolen20:06:59

ClojureScript knows what those values should be

anmonteiro20:06:00

it might be possible to work around it by using eval instead of vm.runInThisContext but I’m not sure how that would play with source maps

thheller20:06:40

@anmonteiro I wrote this mostly as a reminder for myself, but it works around the issues you are seeing maybe https://github.com/thheller/shadow-build/blob/master/doc/node.md

anmonteiro20:06:08

let me have a look

thheller20:06:10

ie. local vs global scope and such

anmonteiro20:06:21

reading it now

anmonteiro20:06:51

that’s an interesting read, but doesn’t seem like something we could do easily

thheller20:06:40

thats how the final “bootstrap” final looks for a node repl for example

thheller20:06:24

very different from the CLJS bootstrap but it solved all the issues I had so I’m happy with it 😉

dnolen20:06:06

@anmonteiro like I alluded I don’t really see the point

dnolen20:06:24

:npm-deps is just about client stuff and we just want it to work OK with REPL

dnolen20:06:36

if you’re doing real Node.js dev with CLJS you don’t care about :npm-deps

anmonteiro20:06:39

I’m gonna play with it a little bit more if that works for you

anmonteiro20:06:56

but I’ll probably just adopt the magical globals approach 🙂

dnolen20:06:50

I’ve been a bit lazy about organizing tickets around releases, moving forward would be nice to be bit more rigorous about it

dnolen20:06:21

if you’re opening a ticket always set “Affects Version”

dnolen23:06:40

only did very light testing but worth kicking around

mfikes23:06:24

There are already some things in core (like cljs.spec.alpha/MAX_INT) that are marked as such and will benefit :)