@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

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

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

instead of need the ambient thing which was always yucky

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

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

Sounds good

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

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

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

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

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)))

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

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

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

Okay

@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

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

@juhoteperi: can’t do anything about those.

but I’m also not concerned about them

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

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

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

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

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

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

@juhoteperi: thanks!

@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.

@dnolen: Did you check the note about analyze ?

@juhoteperi: note where? in the patch?

@dnolen: jira comment

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

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

just the body of analyze

one case for compiler state provided another for without

something like:

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

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

@juhoteperi: I suggesting something way less clever

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

ah

will do that now I’m looking at it

Okay

done

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

that’s how they always worked

hopefully a couple times of that

and people start passing explicit state

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