Fork me on GitHub
#cljs-dev
<
2016-10-03
>
juhoteperi20:10:45

@anmonteiro @dnolen Did you test circle-color advanced build with older versions of Cljs? I'm seeing Closure error + empty output with 1.7.145 and 1.9.229

anmonteiro20:10:06

@juhoteperi definitely Closure error with 1.7.145

anmonteiro20:10:14

only warnings with your patch

dnolen20:10:43

@juhoteperi I get output with your patch applied, but it doesn’t work

juhoteperi20:10:44

Yes, Closure has changed "constant module$resources$public$js$libs$react assigned a value more than once." from error to warning

anmonteiro20:10:12

but the app doesn’t really work even with externs

dnolen20:10:17

also need to make sure you’re using externs

juhoteperi20:10:31

Oops, I had typo on externs option

juhoteperi20:10:25

Yep, seeing quarter of circle now

anmonteiro20:10:26

@juhoteperi that and the input doesn’t work

anmonteiro20:10:30

it’s what we both had

juhoteperi20:10:20

Problem with input change is very clear, and is caused by the cljs source

juhoteperi20:10:38

(. this -handleColorChange)

juhoteperi20:10:28

this handleColorChange custom method is introduced by cljs code, and not included in externs so calls get rewriten

anmonteiro20:10:38

right that makes sense

dnolen20:10:43

but that doesn’t explain the other issues

juhoteperi20:10:49

cx and cy properties on circle svg component get renamed to ak and bk (or something like that)

juhoteperi20:10:57

other properties work as they are included in some externs

juhoteperi20:10:40

Processed Circle.js: React$$module$resources$public$js$libs$Circle.createElement("circle", {cx:"100px", cy:"100px", r:"100px", fill:this.props.color})

juhoteperi20:10:54

Optimized: ta.createElement("circle", {ak:"100px", bk:"100px", r:"100px", fill:this.props.color})

juhoteperi20:10:17

Is there anything that can be done about this? Other than to add those properties to extern file?

dnolen20:10:33

@juhoteperi: oh because those are props and JSX doesn’t compile to object literals with string properties

dnolen20:10:53

oh but those are legitimate SVG properties no?

juhoteperi20:10:27

I checked and looks like JSX transform tool doesn't have option to output string properties

dnolen20:10:16

sorry what I mean is - isn’t there an externs for SVG?

dnolen20:10:47

(though JSX having a knob for output would also be useful)

juhoteperi20:10:00

Hmm, I don't see specific svg extern file at Closure repo

dnolen20:10:21

in general I guess I don’t see how this would be a problem actually

dnolen20:10:28

props would get renamed consistently

dnolen20:10:54

so JSX behavior is a good thing as long there aren’t string shenanigans somewhere

dnolen20:10:58

in user code

juhoteperi20:10:47

Works with that extern file

juhoteperi20:10:23

React probably passes those properties as is to DOM

anmonteiro20:10:26

wow, great news

dnolen20:10:43

so now I wonder if #js needs something else 😛

juhoteperi20:10:04

hmm? #js emits string properties

dnolen20:10:18

right but that’s not what you want

dnolen20:10:29

since if everything is going through Closure, it will optimize all these properties

dnolen20:10:36

will think about it some more ...

juhoteperi20:10:36

But optimizing #js wouldn't work with anything that is outside of Closure, like browser built-in JS

dnolen20:10:52

@juhoteperi I’m not suggesting breaking #js

dnolen20:10:09

I’m just pointing out that this means for React stuff we just want to emit literals w/o strings

favila20:10:44

I've hurt for this a few times actually

dnolen20:10:59

yeah there’s other cases

favila20:10:33

We use #js for struct-like things but google closure tooling really wants struct to be literals and dict to be strings

favila20:10:48

easy thing is #jso, uses set! for properties instead of aset

favila20:10:06

or something similar

dnolen20:10:07

yes something like that, but should think about the name

favila20:10:19

but reader macro isn't essential, just nice sugar

dnolen20:10:25

ok - @juhoteperi anyways fantastic stuff - I poked around your patch and I couldn’t really think of anything specific

dnolen20:10:36

wrt. things that needed changing

dnolen20:10:49

it’s all implementation details as far as I’m concerned

juhoteperi21:10:35

Great. I'll work through FIXME notes and maybe tune the test.

dnolen21:10:35

@juhoteperi is there something about your patch that you feel doesn’t fall into this assessment?

favila21:10:57

actually now that I think about it, to emit a literal js record (instead of a={};a.prop1=...) from cljs side, need a compiler primitive or at least a js*. Also need new JSValue-like wrapper on the clojure side if clojure macros want to emit the same.

dnolen21:10:45

probably can just make JSValue have a flag

dnolen21:10:34

also I wonder if this should be a per file configuration thing

dnolen21:10:40

so we don’t have to add more syntax

favila21:10:40

that might get confusing. I run into it when I need to pass js value to other adv-compiled js from cljs

favila21:10:07

In that case I need to remember to either use my special anonymous-deftype macro on the calling side, or to use array-access on the callee side

dnolen21:10:14

this needs a ticket and a enumeration of the options

dnolen21:10:21

likely will need to stew on it for a while