thheller / shadow-cljs

ClojureScript compilation made easy
https://github.com/thheller/shadow-cljs
Eclipse Public License 1.0
2.23k stars 173 forks source link

`:on-click` -like functions being lobotomized #1187

Closed enspritz closed 2 weeks ago

enspritz commented 2 weeks ago

Hello

Currently investigating moving from a Leiningen-based cljsbuild to shadow-cljs build for an existing codebase. I noticed that basically all reagent.core/create-class components that have event handlers like :on-click, such as:

(defn my-novel-component [a b]
  (reagent.core/create-class
    {:reagent-render (fn [_ _]
      [:button {:on-click action-fn}]}))

Compiled with shadow-cljs with :advanced optimizations produce event handlers whose function bodies are empty. Inspecting in a browser shows the event handler as for example:

function sD() {}

There are about 20 such sites across this code base, all appear to be similarly afflicted. All other code appears to be functioning as expected.

I experimented with a variety of changes to the event handlers to see if I could provoke a change in the compiled output. The following had no effect; an empty function remained the registered event handler for the element:

And these changes caused the browser dev tools inspector to show that no event handlers are registered on the rendered element:

All libs are latest versions (shadow-cljs 2.29.9, re-frame 1.4.3, reagent 1.2.0) except react and react-dom which are pinned at 17.0.2.

thheller commented 2 weeks ago

It is pretty much impossible that shadow-cljs removes code in the way you are describing.

:advanced removes dead code yes, so if this code is never actually used it would be removed entirely. It does not leave empty functions. It certainly has no special treatment of anything reagent. Assuming that your lein setup used :advanced previously then there is no difference in regard to how shadow-cljs handles this. If you previously only used :simple it is possible that this is externs related. Do you get warnings during compilation?

In the snippet above you use action-fn but never show how that is actually defined.

You can try using npx shadow-cljs release <your-build> --pseudo-names to help with debugging. It will use semi-predictable names in the generated code, so that at least you can figure out what the code originally was.

enspritz commented 2 weeks ago

Thank you for your confirmation, and yes, it is my sentiment too regarding optimizations, but of course you have much greater knowledge in this area. I have been monitoring compiler output (a bit of a past-time), and was quite surprised to see this behavior.

Do you get warnings during compilation?

Yes, but they are (by my judgement) entirely unrelated to the problem. Mainly (reagent.dom/dom-node) deprecation warnings at usage sites distant from this problem, and the always-present :redef error in no/en/core.cljc.

In the snippet above you use action-fn but never show how that is actually defined.

action-fn is variously as:

(let [action-fn #(.stopPropogation %)] ...
; or  
(let [action-fn (fn [_] (re-frame.core/dispatch [::garble-grok :bleep :bloop]))] ...
; or even 
(defn a-component [action-fn] 
  (reagent.core/create-class
    {:reagent-render ... 
      {:on-click action-fn}...)
enspritz commented 2 weeks ago

Thanks for the compiler option hint; I tried it out but to no avail. Also changed the :optimization level to :simple, no change. For that reason, I now realize that my initial assumption that this was DCE or some other optimization was an assumption; changing the title of this issue to reflect that.

thheller commented 2 weeks ago

This is extremely hard to debug if you give me random snippets that are neither syntactically nor semantically correct.

If you want any kind of help I need a full snippet, or even better a reproducible example.

enspritz commented 2 weeks ago

This build configuration is quite straight forward to me, no tricks.

{:builds
 {:abc
  {:compiler-options {:closure-output-charset "US-ASCII"
                      :externs                ["closure-externs/misc-externs.js"]
                      :infer-externs          :auto
                      :language-out           :ecmascript5-strict
                      :output-wrapper         true
                      :source-map             true}
   :js-options       {:resolve {"filesaverjs" {:target :file
                                               :file   "node_modules/file-saver/dist/FileSaver.js"}
                                "jszip"       {:target :file
                                               :file   "node_modules/jszip/dist/jszip.js"}
                                "screenfull"  {:target :file
                                               :file   "node_modules/screenfull/index.js"}}}
   :modules          {:core    {:entries [vivid.abc.all-namespaces]
                                :prepend "/**\n * @license Copyright ...\n */"}}
   :output-dir       "src/main/resources/js"
   :target           :browser}}

 :deps  true}
enspritz commented 2 weeks ago

Yes I understand, I'm working on that now.

thheller commented 2 weeks ago

What is the point of those configured :resolve options? They shouldn't be necessary?

enspritz commented 2 weeks ago

What is the point of those configured :resolve options? They shouldn't be necessary?

Slowly porting the configuration over from lein / cljsbuild to shadow-cljs. Got stuck at this point, and haven't attended to those yet. Moments ago, I did remove them and that had no effect in terms of this issue ..

This is extremely hard to debug if you give me random snippets that are neither syntactically nor semantically correct. If you want any kind of help I need a full snippet, or even better a reproducible example.

It's not my intention to use you and your time as a troubleshooting resource. I know none of us will appreciate me barfing the ~2 pages of actual code here, so I attempted to get the essence across. I might not have succeeded on that point..

A reproducible example, on the other hand, is just what the situation calls for. In particular I'm not convinced about how Reagent's create-class is being used.

However, I think I'm going to walk the whole thing back; the build products from the prior build are shippable, and both of our time is precious ;) Let's let this one go..