Fork me on GitHub
#cljs-dev
<
2015-07-28
>
juhoteperi14:07:15

@dnolen: Other than with-bindings and the var, do you think the approach in cljs-1367 is correct? Wrapping implementation calls in namespaces where the api call takes in opts argument

dnolen14:07:45

@juhoteperi: yes, but I actually want to do another thing

dnolen14:07:58

which is for all those functions to to have an additional arity that takes the compiler state

dnolen14:07:10

instead of need the ambient thing which was always yucky

dnolen14:07:26

this how cljs.js works now and we should just adopt this approach for uniformity

dnolen14:07:42

it won’t break previous users if it’s an additional arity, where the state is the first argument

dnolen14:07:24

(defn foo ([] (foo env/*compiler*)) ([state] …))

dnolen14:07:45

@juhoteperi: other than that yes that’s the right idea

juhoteperi14:07:17

I think this additional state argument is probably best solved in another ticket and patch?

dnolen14:07:45

@juhoteperi: could do, but just simpler to just fix it all in one go.

juhoteperi14:07:20

Instead of with-bindings I would use macro or function:

(defmacro with-api-opts [opts & [body]]
  `(binding [ana/*cljs-warning-handlers* (:warning-handlers ~opts ana/*cljs-warning-handlers*)]
     ~@body))

(defn with-api-opts [opts f]
  (binding [ana/*cljs-warning-handlers* (:warning-handlers opts ana/*cljs-warning-handlers*)]
     (f)))

juhoteperi14:07:43

This would also make it easy to extend it later if other "api-opts" are added

dnolen14:07:13

@juhoteperi: do not with anything with-x thing at all

dnolen14:07:34

you must put the bindings inline, it’ll be tedious but then the api won’t be goofy

bensu15:07:42

@dnolen @juhoteperi I am all for the "pass the state as an argument". This is what I ended up doing to hide the analyzer from the rest of the code in a doc generator: https://github.com/funcool/codeina/blob/master/src/codeina/reader/clojurescript.clj#L76

juhoteperi15:07:18

build and watch btw. already take the compiler-env as argument, but it's not the first argument

dnolen15:07:56

@juhoteperi: can’t do anything about those.

dnolen15:07:13

but I’m also not concerned about them

dnolen15:07:24

it’s the ambient weird things I want to get rid of over time.

juhoteperi16:07:15

Would it be okay to make cljs.analyzer/default-warning-handler public and expose it in api? If :warning-handlers overwrites the warnings handlers, I need some way to access the default handler

juhoteperi17:07:15

Compiler-env is necessary for cljs.analyze/analyze, right? So it makes sense if it throws error if it isn't passed compiler-env or dynamic var isn't set

dnolen17:07:57

@juhoteperi: if analyze didn’t throw before it’s not going to throw now

dnolen17:07:37

@juhoteperi: there should be a function to return the default warning handler sure in the api ns

juhoteperi17:07:20

Okay. I'll attach a patch to Jira for comments (it still needs fix for analyze)

dnolen21:07:17

@juhoteperi: patch looks good, I think we probably want to put warning handlers into the compiler state along with the other queryable things, but we can do that later.

juhoteperi21:07:53

@dnolen: Did you check the note about analyze simple_smile?

dnolen21:07:35

@juhoteperi: note where? in the patch?

dnolen21:07:27

@juhoteperi: ah no, we should probably dupe the code and have two code paths, one for state nil and not nil

juhoteperi21:07:07

@dnolen: Dupe which part of the code? with-compiler-env or the call to implementation from every api call?

dnolen21:07:22

just the body of analyze

dnolen21:07:32

one case for compiler state provided another for without

juhoteperi21:07:03

something like:

(defn analyze* [...] (binding [...] ...))
(defn analyze ([...] (analyze* ) ([state ...] (analyze* ...)

juhoteperi21:07:16

Or I could just copy the option binding to the other arity

dnolen21:07:54

@juhoteperi: I suggesting something way less clever simple_smile

dnolen21:07:18

(if state (env/with-compiler-env …) (binding […] …))

dnolen21:07:42

will do that now simple_smile I’m looking at it

juhoteperi21:07:12

I noticed another thing in analyzer.api ns, the calls which directly access compiler-env will throw an error when trying to deref nil

dnolen21:07:41

that’s how they always worked

dnolen21:07:50

hopefully a couple times of that

dnolen21:07:56

and people start passing explicit state

dnolen22:07:48

@juhoteperi: thanks much for working on this btw, a big improvement