Fork me on GitHub
#cljs-dev
<
2016-11-08
>
anmonteiro09:11:27

@thheller: I don't see what the problem is with respect to the current behavior

anmonteiro09:11:54

It doesn't change anything, just makes it customizable...

thheller09:11:18

@anmonteiro yeah but why make it customizable

thheller09:11:44

writing faulty analysis data that cannot be re-created to the same state helps no one

thheller09:11:04

IMHO it would be better to just skip the cache if it fails

thheller09:11:15

with a little warning that caching failed

anmonteiro09:11:20

@thheller well people may want the analysis cache to be written out in EDN even if Transit is in the classpath

thheller09:11:29

why though?

thheller09:11:35

they shouldn't even care

thheller09:11:57

just one more thing to care about that might cause problems

thheller09:11:06

I'm not against :edn itself

thheller09:11:16

that is well and good if transit is not available

thheller09:11:40

the problem I have with the toggle is that the user may run into an error

thheller09:11:58

by "accidentally" having transit on the classpath

thheller09:11:27

so the user needs to search for a solution to an error he would not otherwise see

thheller09:11:44

but the underlying problem is still the same ... there is un-readable data in the cache

thheller09:11:51

well not un-readable ... just useless

thheller09:11:34

#object[Thing "something"] when coming from cache vs. an actual Thing instance

anmonteiro09:11:02

right but that's solved by toggling the switch to :edn, isn't it?

anmonteiro09:11:10

then the analysis is written and read as EDN 🙂

thheller09:11:12

no you are missing my point

niwinz09:11:28

yes, but using edn not makes ⁠⁠⁠⁠#object[Thing "something"]⁠⁠⁠⁠ usable

thheller09:11:28

the problem is EDN being too lax

niwinz09:11:38

it just hides the problem 😉

niwinz09:11:03

I completly agree with @thheller arguments

thheller09:11:16

right now it doesn't really matter since nobody is using this data, but it is still an error

anmonteiro09:11:34

how come nobody is using this data?

anmonteiro09:11:52

Transit support has been there for a while

thheller09:11:53

well not that I'm aware of, ppl actually might

thheller09:11:15

the problem AFAICT is the :arglists

thheller09:11:52

cache wants to write arglists .. say (defn my-fn [{:keys [thing] :or {thing #js {}}])

thheller09:11:10

transit failed before since when it gets this it is

thheller09:11:29

[{:keys [thing] :or {thing (JSValue. {})}]

niwinz09:11:52

the problem is not use edn or transit, the problem is write in analysis cache data that is useless. When transit is used, an exception is raised, when edn is used, it is silently written to cache (but it useless, becasue when that cache is readed again, previously written bad data it is useless).

niwinz09:11:38

and @thheller proposal is just does not write analysis cache when it is not posible to encode valid data... (I'm right?)

thheller09:11:46

exactly since EDN has a "default" writer it doesn't error out as transit does

anmonteiro09:11:14

oh OK I get it

thheller09:11:16

it is just very hard to detect when using EDN

thheller09:11:25

since its default writer

anmonteiro09:11:30

@thheller so IMHO you're conflating things...

anmonteiro09:11:46

because the problem you're referring to exists today, and has existed for a while

anmonteiro09:11:14

so why mention it as part of this ticket at all?!

niwinz09:11:19

and the proposal is to solve that instead just workaround it allowing select edn...

anmonteiro09:11:38

I think they are two different things

anmonteiro09:11:52

people should be able to choose to write the cache as edn vs transit even if there's the EDN problem

thheller09:11:56

@anmonteiro because the ticket introduces an additional compiler option that is not useful

thheller09:11:18

why should they? if transit is available it is the better option so use it

niwinz09:11:23

hmm @thheller I agree here with @anmonteiro , the option is useful

thheller09:11:31

why would they ever not want to?

niwinz09:11:18

my +1 to have the option, and my +1 to solve the problem of writing useless cache

niwinz09:11:09

solving the second issue will allow use transit in all cases, because cache that can't be encoded will just gracefully not written to cache instead crash...

thheller09:11:04

I'll open a ticket for the broken EDN case

thheller09:11:18

still interested why you think the option is useful

thheller09:11:50

basically the only way to encounter this issue is via data literals

thheller09:11:14

well maybe metadata, not sure if that is written to cache

thheller10:11:10

@anmonteiro @niwinz opened http://dev.clojure.org/jira/browse/CLJS-1843 so the option can be discussion separately

anmonteiro13:11:38

@dnolen before I attach it to JIRA, how's this one based on your feedback yesterday? https://github.com/anmonteiro/clojurescript/commit/2b7e40705bcbd92ed3ee1e607ac10c4dad4fdc2d

anmonteiro13:11:26

- added validation for the :cache-analysis-format option - explicit inference of the cache file format via the new function cache-analysis-ext

dnolen13:11:41

@anmonteiro looks good enough to me

anmonteiro13:11:45

and we didn't need to change the API

anmonteiro13:11:19

@dnolen attaching it to JIRA then, thanks