Fork me on GitHub
#cljs-dev
<
2016-10-23
>
wilkerlucio03:10:17

hey, since I updated the Om to 1.9.293 I was having some compilation issues, I was able to narrow it down to this code:

wilkerlucio03:10:43

trying to compile this gives me the error:

wilkerlucio03:10:51

but, only when the dependency [com.cognitect/transit-clj "0.8.290"] is added to the project

wilkerlucio03:10:11

I put up a repository with the minimal case here: https://github.com/wilkerlucio/cljs-compilation-fail

wilkerlucio03:10:17

can someone please try to reproduce the compilation issue to confirm is not just something local here?

Yehonathan Sharvit04:10:41

FWIW I know that adding transit as a dep is causing the analysis cache files to be in json format instead of edn

jrheard07:10:19

@wilkerlucio i cloned that repo, ran that command, got the same output

wilkerlucio10:10:02

thanks for the reproduction @jrheard, so seems something weird is going on there

Yehonathan Sharvit12:10:36

I’m working on a port of core.match to be bootstrapped compatible

Yehonathan Sharvit14:10:57

all the tests from the wiki page of core.match https://github.com/clojure/core.match/wiki/Basic-usage are passing

Yehonathan Sharvit14:10:43

of self-hosted core.match

Yehonathan Sharvit14:10:30

renamed in to match.cljc and make a branch for :cljs

Yehonathan Sharvit14:10:58

Should I submit a patch? where?

Yehonathan Sharvit14:10:06

clojurescript JIRA or core.match JIRA?

dnolen14:10:07

the project to which it belongs

dnolen14:10:16

if it’s a ClojureScript patch, ClojureScript

dnolen14:10:27

if it’s a core.match patch, core.match

Yehonathan Sharvit14:10:20

It’s a core.match path

Yehonathan Sharvit14:10:09

I cannot find the guideline for creating a patch

Yehonathan Sharvit14:10:48

Here is the JIRA ticket for core.match self-host compatible http://dev.clojure.org/jira/browse/MATCH-116

Yehonathan Sharvit14:10:12

@dnolen @mfikes @anmonteiro your code review would be much appreciated

dnolen14:10:36

@viebel yes, but I will get to it when I get to it

dnolen14:10:47

it’s no going to be this week - off to EuroClojure and ReactiveConf

Yehonathan Sharvit14:10:12

Good luck and happy talk 🙂cljs

wilkerlucio17:10:32

@dnolen about the CLJS-1832, I really believe it's a major because all it takes is having Om (that depends on transit-clj) and a destructuring with #js to break it (and currently untangled.client.data-fetch has that, making impossible to use with latest clojurescript), can you tell me why you consider it a minor issue?

dnolen17:10:33

@wilkerlucio it’s just not a deal breaker sorry

dnolen17:10:47

major issues should be things that prevent stuff from working at all usually

dnolen17:10:57

core data structures, hashing, file leaks, etc.

dnolen17:10:26

in general ask here first before marking anything as major

wilkerlucio17:10:43

well, the update made previous working code into a code that doens't compile at all

dnolen17:10:53

it doesn’t mean regressions are major

dnolen17:10:59

major is about how big of a problem it is

dnolen17:10:11

you can work around this trivially with js-obj

wilkerlucio17:10:29

I can if it's my code, but what about when it's on a dependency?

dnolen17:10:29

dependencies can get fixed

dnolen17:10:39

major issues are stuff that effect almost everyone

dnolen17:10:11

occasionally small regression get in - we can’t go around marking all of these as major

anmonteiro17:10:15

@wilkerlucio FWIW you might get away in your case by specifically depending on transit-clj 0.8.288, instead of 0.8.290, have you tried that?

dnolen17:10:16

dependencies not-withstanding

wilkerlucio17:10:59

@anmonteiro I didn't yet, I'll try

anmonteiro17:10:18

this seems more like a regression in transit-java than in the CLJS compiler nevertheless

anmonteiro17:10:38

related to the recent mutex fix for ClojureScript

dnolen17:10:40

that’s another thing

dnolen17:10:54

the stack trace wasn't particularly informative

wilkerlucio17:10:12

@anmonteiro same thing happens with 0.8.288

dnolen17:10:20

another thing is there’s a fix that subsumes all of this stuff

dnolen17:10:33

which is a knob for disabling transit encoding - since no doubt other things will arise

dnolen17:10:42

there’s already a ticket for that and anyone can work in it

anmonteiro17:10:30

@wilkerlucio hrm sorry, you might still be getting a wrong transit-java dep

anmonteiro17:10:49

make sure you’re on transit-java 0.8.313

anmonteiro17:10:00

or rather anything below 0.8.316

wilkerlucio17:10:13

@dnolen thanks for the clarifications, the nob to disable transit encoding would be great

dnolen17:10:43

anyone can feel free to take it

anmonteiro17:10:25

that’s the one

wilkerlucio17:10:10

can you tell me where I should start looking to fix this one? I never digged into cljs compilation code before

dnolen17:10:44

@wilkerlucio it’s super simple, the most annoying part is just having to change some signatures

dnolen17:10:59

see cljs.analyzer/write-cache-file

dnolen17:10:06

and all its' callers

dnolen17:10:14

i think there’s only a 2 places

dnolen17:10:33

in these locations we should be checking build options to see what we should be writing

dnolen17:10:49

the code was written a bit hastily and may need some refactoring to make this work

wilkerlucio17:10:54

@dnolen ok, thanks, I'll check it out

wilkerlucio18:10:21

@dnolen the write-cache-file name is right? I can't find it anywhere on the entire project https://github.com/clojure/clojurescript/search?utf8=%E2%9C%93&amp;q=write-cache-file

dnolen18:10:47

@wilkerlucio sorry write-analysis-cache