Fork me on GitHub
#cljs-dev
<
2016-11-07
>
anmonteiro12:11:57

@dnolen the MAX_SAFE_INTEGER here is probably an oversight, or is it on purpose? https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/spec.cljs#L638

anmonteiro12:11:07

because Number.MAX_SAFE_INTEGER is ES6, I think

anmonteiro12:11:31

(you do define a MAX_INT at the beginning of the namespace, so I'm leaning towards oversight)

dnolen12:11:31

@anmonteiro we can’t use ES6

anmonteiro12:11:41

right, so probably a bug

anmonteiro12:11:10

need to change that to the MAX_INT constant defined at the top of the namespace

dnolen12:11:32

right sorry yeah I think just did that one quickly

anmonteiro12:11:34

@dnolen I'm porting the perf tweaks, should I change it as part of this patch or in a separate one?

dnolen12:11:37

and forgot to change it later

dnolen12:11:44

@anmonteiro fine if in the same patch

anmonteiro14:11:44

@dnolen attached a patch to the perf tweaks JIRA ticket: http://dev.clojure.org/jira/browse/CLJS-1768 as noted in the comment, I've pushed a branch with the 4 different commits to make it easier to review: https://github.com/anmonteiro/clojurescript/tree/spec-perf-tweaks

anmonteiro15:11:58

@dnolen the following ticket is assigned to you, are you planning to fix it? http://dev.clojure.org/jira/browse/CLJS-1702

dnolen15:11:24

@anmonteiro if you want to take it go for it

anmonteiro15:11:17

@dnolen as I mentioned in the backlog, I think this is not a bug: http://dev.clojure.org/jira/browse/CLJS-1622

anmonteiro15:11:27

he's trying to use with-redefs with static-fns true

anmonteiro15:11:02

as far as I understand, this is only supported if the function is marked ^:dynamic, correct?

dnolen16:11:41

yes closed

anmonteiro18:11:44

we don't really use it anymore now that require is a thing

anmonteiro18:11:15

probably would mean breaking changes for tooling... hrm

dnolen18:11:42

was about to say yes as long as it doesn’t break anything internally

dnolen18:11:49

externally no one should have been relying on that

anmonteiro18:11:52

it doesn't internally

dnolen18:11:56

if they are - they should expect to break

anmonteiro18:11:01

OK I'm OK with that

anmonteiro18:11:30

I think e.g. Planck implements require through this mechanism

anmonteiro18:11:03

but Planck is tied to a specific ClojureScript version so it can just choose to upgrade whenever

mfikes18:11:16

Right—don't worry about Planck; it will be updated to the new macros instead of REPL specials. Besides, it's ok to break Planck.

mfikes18:11:17

(It's not using public API.)

anmonteiro18:11:39

tested REPLs (including Browser), and ran tests

anmonteiro19:11:09

going to look at http://dev.clojure.org/jira/browse/CLJS-1666 next ("Flag to optionally disable transit analysis cache encoding"). what should this compiler option be called?

dnolen19:11:51

:cache-analysis-format, should support 2 values :edn & :transit

anmonteiro19:11:47

@dnolen OK sounds good. default is the current impl? i.e. :transit if the lib is in the classpath

dnolen21:11:25

@anmonteiro probably want to provide :edn as default for the 3 argument case no? that and add an explicit case for :edn, and a throwing case for unknown format?

anmonteiro21:11:36

@dnolen why should we default to :edn in the 3 argument case?

anmonteiro21:11:58

I thought we wanted to default to Transit

dnolen21:11:05

@anmonteiro hrm sorry you’re right but I think we need this to be less implicit then

dnolen21:11:21

that is the logic for choosing the format shouldn’t be hard coded into the final arity

dnolen21:11:25

but before

dnolen21:11:37

then you have the option to fully control the beahvior

anmonteiro21:11:51

right, so we're forced to change the API, I believe

dnolen21:11:06

and I think we still want to throw if we can’t determine the format

anmonteiro21:11:16

yeah, I have no arguments against that

dnolen21:11:23

@anmonteiro yes so you’ll have to touch a couple of other things

dnolen21:11:32

but I’d rather we just fix it

anmonteiro21:11:36

no problem touching other stuff, just wanted to make sure

anmonteiro21:11:50

thanks for the feedback, incorporating it now

thheller21:11:48

@anmonteiro @dnolen added a comment to http://dev.clojure.org/jira/browse/CLJS-1666 ... just my 2 cents, gotta sleep now. be back tomorrow.