Fork me on GitHub
#cljs-dev
<
2016-07-22
>
dnolen12:07:37

@rohit: the analysis cache isn’t completely about being valid EDN since we want to capture the fn signature for the benefit of users

dnolen12:07:16

@rohit I’m still collecting information about whether the Transit cache is worth these kinds of complications - so I’ll continue to keep an eye about how many issues like this people are encountering

rohit12:07:07

@dnolen: ah got it. thanks a lot. really appreciate it!

martinklepsch12:07:18

@dnolen: will you integrate @rohit's patch in the meantime? (@rohit did you open a ticket by any chance?)

dnolen12:07:30

@martinklepsch: already applied to master

martinklepsch12:07:01

We could use a Github integration in this channel, yay, nay?

rohit12:07:45

or maybe a separate channel- cljs-dev-jira?

rohit12:07:56

right now this channel is pretty clean

martinklepsch12:07:24

Yeah, maybe. Although a jira integration probably doesn't fit into our "free Slack" integration limit ...

rohit13:07:12

i think it’ll require permissions from both jira admin and slack admin. https://clojurians.slack.com/apps/A0F7YS3MZ-jira

anmonteiro16:07:10

under simple optimizations, cljs.pprint/compile-format [1] compiles to: EDIT: see snippet below this results in an error (again, under simple optimizations. works fine with :none and :advanced). I don’t know if this is a problem in Clojurescript or the Closure Compiler itself [1]: https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/pprint.cljs#L2672-L2690

rohit18:07:33

@anmonteiro: the line you are highlighting looks pretty innocuous in cljs (https://github.com/clojure/clojurescript/blob/c3899acf797eb6779c53b313f5606c5018360b83/src/main/cljs/cljs/pprint.cljs#L2684). Could you share a usage which doesn’t work under :simple optimization?

anmonteiro18:07:57

@rohit: exactly my thoughts. but the javascript it compiles is invalid

anmonteiro18:07:21

I’ve found this while trying to compile some devcards under simple optimizations

dnolen19:07:33

@anmonteiro: seems weird is there something suspicious looking in the CLJS code?

anmonteiro19:07:07

@dnolen: it’s really weird and I still haven’t found a reliable way to reproduce 100% of the time

darwin20:07:04

@anmonteiro: just wondering if your devcards setup does not create multiple javascript contexts (e.g via iframes or webworkers) to isolate individual devcards. I saw one tricky case where :none and :advanced builds worked, but :simple was broken just because I was crossing js-context boundary (e.g. calling pr-str on a data structure created in a different js-context).

darwin20:07:31

ah, nevermind, forget my message, it was a different problem, :none and :simple worked, but :advanced did not

anmonteiro20:07:09

managed to repro 100% without Devcards

anmonteiro20:07:11

compiling just the following with {:optimizations :simple, :pseudo-names true} will cause the issue

(ns foo.core
  (:require [cljs.pprint :as pprint]))

anmonteiro20:07:02

what triggers the issue is setting :pseudo-names to true

anmonteiro20:07:20

but I’m not even sure if simple compilation + pseudo-names is a valid combo?

dnolen21:07:47

ah hrm I may have seen something like that before

dnolen21:07:03

a problem where the generated pseudo name creates a clash with a ClojureScript name because I don’t think Closure is looking for it

dnolen21:07:07

and we use a similar delimiter

dnolen21:07:13

and yes I would argue that :pseudo-names is only for debugging advanced/simple compilation issues

anmonteiro21:07:38

@dnolen: this does happen with simple compilation

anmonteiro21:07:30

@dnolen: so could the problem in cljs.pprint/compile-format be the usage of the dynamic var?

anmonteiro21:07:41

that Closure compiler thinks it can overwrite, or something?

dnolen21:07:10

@anmonteiro: I don’t think so

dnolen21:07:21

I’m suggesting that we just have name generation clash here

dnolen21:07:31

Closure produces an existing name that we already created

anmonteiro21:07:46

@dnolen: and how can that be avoided?

dnolen21:07:01

well in the general case I don’t see how it can

anmonteiro21:07:21

OK I think I get it

anmonteiro21:07:48

@dnolen: I managed to get it to work using a simple hack, though

anmonteiro21:07:19

IIRC, I was mutating (and assigning) the variable twice, so that Closure would generate another name for it

anmonteiro21:07:36

I can see how undesirable that is, though

dnolen21:07:25

@anmonteiro: my guess could be wrong of course and there’s something weird here about pseudo-names and how dynamic bindings work that I don’t understand

anmonteiro21:07:29

@dnolen: So far I was unable to find any related issues in the Closure Compiler repo, which leads me to believe that we might be doing something wrong

darwin21:07:42

dnolen: have been working on support for deftype/defrecord in cljs-devtools, discovered some inconsistencies along the way. for example type->str does not work for defrecords and I don’t see reason why it should not implement .-cljs$lang$ctorStr like deftype does, I had to resort to use cljs$lang$ctorPrWriter[1], nothing is really blocking me, because I have workarounds, but I would like to propose some patches which would make my implementation more maintainable https://github.com/binaryage/cljs-devtools/commit/99d1d61398f14614d95b6fee632dbf00300d84b2

darwin21:07:16

also noticed that IIterable protocol probably should be listed in fast-path-protocols

dnolen21:07:57

@darwin ok for IIterable but seems like signficantly less big deal

dnolen21:07:18

@darwin I don’t recall why there is a discrepancy about the ctorStr thing, probably just an oversight

darwin21:07:35

that’s what I thought, I will open a JIRA with a patch if preferred

dnolen21:07:52

@darwin yes go ahead for both things

darwin21:07:19

also I would like to add some names for anonymous functions here and there, for example type constructors

dnolen21:07:26

@anmonteiro: that’s also possible but would be easier if we could get a smaller thing that exhibits the problem

dnolen21:07:48

@darwin I tried something like that at some point and it ended up being more trouble than it’s worth

dnolen21:07:27

feel free to take a look into it again but my feeling was it was a rabbit hole - but who knows, maybe you’ll be able to do it simpler w/o issue 🙂

dnolen21:07:47

I was tackling it along with some other things so maybe it just wasn’t important enough for me to sort out

darwin21:07:51

interesting, those names should be properly munged, so I would not expect an issue, will try

dnolen21:07:15

@darwin in my experience so far anything to do name generation is a big pain in the ass

darwin21:07:21

and speaking about demunge, it is not perfect, I had to write some wrappers around it, to handle dollars and to reverse js-reserved transformation

dnolen21:07:24

and never as simple as it seems

darwin21:07:50

maybe something to think about to move to core, if we wanted to make it more robust

dnolen21:07:27

yes munge/demunge pair is useful

dnolen21:07:36

would like to see that get a proper treatment + a bunch of tests

darwin21:07:01

FYI, this[1] is what I use currently, not well tested, will run with it for next devtools release and see what problems people discover, then I can spend some time implementing something like this in cljs.core https://github.com/binaryage/cljs-devtools/blob/99d1d61398f14614d95b6fee632dbf00300d84b2/src/lib/devtools/munging.cljs#L150-L176