Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot turn of "m" shortcut in production #5

Open
claj opened this issue May 31, 2017 · 6 comments
Open

Cannot turn of "m" shortcut in production #5

claj opened this issue May 31, 2017 · 6 comments

Comments

@claj
Copy link

claj commented May 31, 2017

It's not clear at all from the documentation how to disable the "m" short cut key in production.

Is the correct way to set :whitelist to empty string?

@claj claj changed the title Cannot turn of "m" short cut in production Cannot turn of "m" shortcut in production May 31, 2017
@rauhs
Copy link
Owner

rauhs commented May 31, 2017

Do you use klang in production? I mean: The CLJS code with overlay in production? IMO you shouldn't since it's code that you don't really want/need your users to see.

The typical use case is to set klang.logger-fn to your own CLJS function in production. There you could log to the console or send errors to your server.

whitelist is by default the empty string. The logic if a log calls actually emits code is this:

(if-some [res (if (and (empty? whitelist)
                       (empty? blacklist))
                default-emit?
                (and
                  ;; Need to check the blacklist first since it takes priority
                  ;; and otherwise won't get evaluated due to lazyness of and
                  (or (empty? blacklist)
                      (not (ns-match? ns-type blacklist)))
                  (or (empty? whitelist)
                      (ns-match? ns-type whitelist))))]
  res
  default-emit?)

For production I set default-emit to false and specifically whitelist the ones I want to keep:

   :default-emit? false
   :whitelist "*/(ERRO|FATA|WARN|CRIT)"

Similarly, you can do the opposite, say default-emit? to true and blacklist any INFO/DEBG (etc).

Does that answer your questions?

@claj
Copy link
Author

claj commented May 31, 2017

Thanks for your quick reply!

Just to clarify, what we want is to avoid is that the log-viewer can be opened in the production client. We will try out your suggestion, thanks a lot.

Consider this more of a clarification of the documenation (probably in the readme, rather than functionality of the library).

We'll be back, thanks again

@claj
Copy link
Author

claj commented May 31, 2017

We do get klang working in production, but we did confuse the format of edn-file.

If you (read-string "{:logger-fn 'klang.core/log!}"), logger-fn becomes a Cons (a quote) which gives very confusing error messages. When writing a non qouted symbol name like {:logger-fn 'klang.core/log!} in the repl/source file, it seems to work as expected.

It's also not really clear to us whether the prod log function need to be an ^:export-ed var or not. We also get compilation warnings like WARNING: Use of undeclared Var our.logger/prod-logger at every log statement. I don't know if we can avoid that somehow.

An example working prod-klang.edn with correct quoting and such would be very helpful.

We still have problems feeding figwheel and our cljsbuild task with different :jvm-opts, so right now Klang doesn't work in our development work. This is obviously possible to fix, but it was not apparent how it can be done.

@rauhs
Copy link
Owner

rauhs commented May 31, 2017

I see, maybe I'll make it more robust and accept a (quote some-symbol), seems like a common thing to add.

No, you don't need to export your log functions. The macro will just expand to the function call. It'll all get properly minimized. So in production this:

(info! "hi")

Will be translated into something like:

(your-ns.logger/log! "the.ns.in.which.the.log.is" "INFO" "hi")

So the "undeclared var" is and issue I also had. I agree. The only real good solution is to add another source path for your production build, like env/prod/src and in there create a evn/prod/src/klang/core.cljs and in there create a log! function which is your production logger. This will overwrite my klang.core namespace and you'll have it all properly required by CLJS. THe other (annoying) solution is to require your production logger namespace in every singe file in which you log.

Does that make sense?

@claj
Copy link
Author

claj commented Jun 1, 2017

It does make sense, thank you!

Regarding accepting (quote some-symbol) I just want to make this doesn't affect the runtime performance, ie that the more promiscuous lookup is done entirely during compile time.

Regarding the compile time warnings they are certainly annoying... The workaround with an extra klang/core.cljs do work, though.

@rauhs
Copy link
Owner

rauhs commented Jun 1, 2017

Try 0.5.7. It'll automatically pluck out the symbol in case you use 'foo.bar/log. Performance isn't affected, in fact I improved it by avoiding re-compiling the blacklist/whitelist regexes now.

FWIW, (read-string "foo") will return a symbol. So IMO this is already documented, I'll add an example to the README.

The workaround with overwriting klang.core is -I think- the only solution. It's not perfect but gets the job done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants