day8 / re-frame-debux

A fork of debux for tracing re-frame code (for eventual consumption by re-frame-10x)
Eclipse Public License 1.0
43 stars 7 forks source link

Investigate different strategies for avoiding debux tracing in production builds #21

Closed danielcompton closed 4 years ago

danielcompton commented 6 years ago

Currently we use two different JARs with the same API to enable tracing, day8.re-frame/tracing, and day8.re-frame/tracing-stubs. The tracing-stubs library delegates to cljs.core/fn to avoid causing any impact when compiling code when tracing isn't desired, like a production build.

This works ok for Leiningen and Boot, but causes problems for shadow-cljs, where this kind of classpath switching isn't supported.

If a ClojureScript project is compiled with advanced optimizations and includes the tracing JAR, Closure dead code elimination from closure defines should remove all of the extra macroexpansions. I've verified this for the functions themselves, but need to double check if there is any extra code elsewhere which is not removed.

So strictly speaking, it is not necessary to use the tracing-stubs JAR, but it does have two benefits:

Nevertheless, it could be good to investigate alternatives:

@thheller, do you have any ideas?

Full disclosure, we're unlikely to be doing much work on 10x or this project for the next little while, but I'd still be keen to hear people's thoughts now, while they're fresh.

thheller commented 6 years ago

The :external-config is just convention, it is not a part of CLJS. CLJS only has :compiler-options and it is ok to use those (and nest a :external-config in there). Many things do.

It is discouraged to use those because it interferes with caching in CLJS. shadow-cljs invalidates caches when :tooling-config or :external-config change to support this convention. It is perfectly fine to do this in shadow-cljs. CLJS builds (figwheel, lein-cljsbuild or cljs.main) do not cache :advanced builds at all so it is also fine to do it there. Just need to do the usual lein clean when changing the settings during development.

CLJS added globally-shared AOT cache recently which is a nightmare to invalidate. This is the reason doing any side-effects in macros is "very discouraged", it is easier to invalidate "pure" code. Doesn't change the fact that many things still want to do this. CLJS still has a lot of other caching issues to be sorted out so I don't expect the :aot-cache to be used by many until it works properly. Any CLJS user probably is used to the occasional lein clean by now but that would not invalidate the shared AOT cache.

Either way since the main concern here is that :advanced builds should not include the tracing code it is totally safe to use the compiler options for this. There is no cache to worry about in CLJS :advanced and shadow-cljs invalidates properly if you use the :external-config or :tooling-config. It also doesn't share cache between dev and release builds anyways.

PS: I'm fairly certain CLJS wouldn't even invalidate caches if you changed the stubs. The reason the stubs work is because there is no caching at all in optimized builds. Pretty sure compiling :none once and then adding the stubs would not recompile anything.

superstructor commented 4 years ago

As mentioned earlier, this works OK for Leiningen including when using shadow-cljs within a Leiningen build; e.g. re-frame-template.

thheller commented 4 years ago

I actually added a feature to shadow-cljs a little while ago which is intended to solve this problem in a slightly different manner that does not involve changing the actual classpath. I didn't document it because it is non-standard and I'm not sure it is actually a good idea but let me break it down a little.

The problem with all of this is the the trace macros have dependencies. It is easy enough to just make the macros do nothing unless a given setting is present. The macros could do this at compile-time so they wouldn't even generate any more code than usual if disabled. The issue however is that the tracing ns has CLJS-side dependencies.

https://github.com/day8/re-frame-debux/blob/866b7928c45b0d7d8819efe81b344ae03ffce29a/src/day8/re_frame/tracing.cljc#L1-L7

Conditional requires are currently not supported by CLJS and that is an area I haven't found a nice generic solution to. There are some but they are mostly just bad hacks. So while it is easy to make adjustments to the macro so they don't generate anything, it is not easy to get rid of the :require. This is not a problem during development but production builds don't really want all this extra code and most of it also can't be removed by the Closure Compiler.

So the stubs come in handy as they just replace everything with a no-op and don't have any outside dependencies.

shadow-cljs however does not support classpath profiles (mostly because of server-mode) and therefor you have to use project.clj or deps.edn with their profiles/aliases. I don't actually think that this is very user-friendly and I strongly believe that names should be unique. I really don't like if the meaning of names changes based on what I have on the classpath.

So the solution I came up with is simple aliasing. The facilities for this were already in shadow-cljs because of the clojure.* -> cljs.* aliasing so I just had add a couple of lines to make this accessible via the build config.

Long story short: Instead of having a separate tracing-stubs package that just have two namespaces in the actual tracing package.

I'll call them day8.re-frame.tracing and day8.re-frame.tracing-active for now. The .tracing is just the "stubbed" out no-op version. If the user wants to "activate" tracing they can do so via the build config:

{...
 :builds
 {:app
  {:target :browser
   :modules {:main ...}
   :dev {:build-options {:ns-aliases {day8.re-frame.tracing day8.re-frame-tracing-active}}}}} 

During development this would make shadow-cljs use the day8.re-frame-tracing-active namespace whenever someone uses/requires the day8.re-frame.tracing namespace.

This gets you the same result as swapping the classpath without actually swapping the classpath.

I'm not convinced this is actually a good idea though. And regular CLJS doesn't support the configurable aliases so this approach would only be available in shadow-cljs. It is available though and unlikely to be removed given that some core features rely on it.

superstructor commented 4 years ago

Thanks for the comprehensive information @thheller. That is a better solution and one I can add to re-frame-template given it already unapologetic-ally entirely depends on shadow-cljs.

As for re-frame-debux it may require a different project/repo, or just shadow-cljs-specific namespaces. I'll have a think about that one.

superstructor commented 4 years ago

:ns-aliases for shadow-cljs is supported since 0.5.5 and the README has a corresponding section on how to use it so this issue can now be closed.