Don't warn about reloading handlers when initiated by figwheel #204
Comments
|
As far as implementation goes, would it be possible to do an (optional) equality check when overwriting handlers? |
|
@smahood Sadly, it won't work. When figwheel triggers the reload of a namespace, it will create all new functions and they won't test |
|
I have this setup for development: (require '[re-frame.loggers :as rf.log])
(def warn (js/console.warn.bind js/console))
(rf.log/set-loggers!
{:warn (fn [& args]
(cond
(= "re-frame: overwriting " (first args)) nil
:else (apply warn args)))})I want to leave the normal logger operational so that any other warnings that pop up during reload (such as deprecation notices) don't get swallowed. |
When figwheel reloads code, it causes handlers to be re-registered which leads to a warning for each handler. This leads to noisy logs. Figwheel has a :before-jsload key which can be called before it reloads application code. This patch adds a *warn-on-overwrite* dynamic var, and shows how to use it in the example project. One side effect of using this is that you will never get a warning for duplicate handlers when Figwheel is reloading. It may be possible to track how many handlers were reloaded in a particular Figwheel reload, but this would get very complex. In any case, you will still get warnings every time you refresh the browser, which should be good enough for the relatively rare case of creating duplicate handlers. Fixes #204
When figwheel reloads code, it causes handlers to be re-registered which leads to a warning for each handler. This leads to noisy logs. Figwheel has a :before-jsload key which can be called before it reloads application code. This patch adds a *warn-on-overwrite* dynamic var, and shows how to use it in the example project. One side effect of using this is that you will never get a warning for duplicate handlers when Figwheel is reloading. It may be possible to track how many handlers were reloaded in a particular Figwheel reload, but this would get very complex. In any case, you will still get warnings every time you refresh the browser, which should be good enough for the relatively rare case of creating duplicate handlers. Fixes #204
|
@mattly your snippet works well, thanks. AFAIK you don't need the |
|
I just got around this. Solution: clear handlers before Figwheel reloads. (ns ^:figwheel-hooks cat.core ; Tell Figwheel to look for hooks in this ns
(:require
;; ...
[re-frame.registrar]
))
;; Then, remove all registered handlers before loading code
(defn ^:before-load cleanup []
(re-frame.registrar/clear-handlers))I like this solution because it will notify me if I remove code generating a handler and depend on it, in addition to notifying me on actual duplicate handlers. |
|
However if you reload only some (but not all) of your namespaces, some
handlers will be lost, correct? This is how Figwheel works.
…On Wed 10. Oct 2018 at 17:41, Teodor Heggelund ***@***.***> wrote:
I just got around this. Solution: clear handlers before Figwheel reloads.
(ns ^:figwheel-hooks cat.core ; Tell Figwheel to look for hooks in this ns
(:require
;; ...
[re-frame.registrar]
))
;; Then, remove all registered handlers before loading code
(defn ^:before-load cleanup []
(re-frame.registrar/clear-handlers))
I like this solution because it will notify me if I remove code generating
a handler and depend on it, in addition to notifying me on actual duplicate
handlers.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#204 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGfWKaye2PFHAGrBPub0csAn23KxTKcks5ujhUwgaJpZM4Jq3TE>
.
|
|
No, as of (defn mount []
(reagent/render [category-view]
(js/document.getElementById "app")))
(defn ^:before-load cleanup []
(re-frame.registrar/clear-handlers))
(defn ^:after-load re-render []
(mount))
(defonce start-up (do
(rf/dispatch [:initialize-db])
(mount)
true)) |
|
As explained above, this is adding a lot of noise for every reload. This is significant in reload-heavy workflows. Arguably warning about reloads is not a good default. After all, registering a subscription is much like defining a function (or a defmethod). In Clojure, redefining a function is something you're expected to do, and won't issue a warning. Clearing the registry is not a good option for the same reason that the defmulti registry is not cleared when you re-run If there's interest in changing the behavior, I'm happy to prepare a patch @mike-thompson-day8 For the moment, I'm using this hackity hack to monkey-patch the warning away: |
|
@pesterhazy I've looked at this a bit at #413 and was never totally happy with the approach, as it requires everybody to add more config to their project. Before opening up a PR, would you be able to describe your approach? I haven't talked to Mike about this recently, but last time we talked he was keen for there to be a way to catch multiple handlers on the initial page load. Here are a few of my ideas on how you could do this, though none of them feel elegant:
Interested to hear your thoughts 🤔. |
|
@danielcompton, thanks for the thoughtful response AIUI the requirements as explained by you and @mike-thompson-day8 are:
My approach would be
I've created a demo repo with inlined re-frame source code for simpler modifiction. Let me know what you think |
That's basically the approach taken above
I think you usually want asserts and warnings off in prod (that's how it is today). Config options are always a bit clunky. I also think that approach 1 is a bit better because it still gives you warnings on initial load.
That seems fragile and too magical to me |
|
We considered:
OutcomeI have changed re-frame so that it will no longer warn about duplicates after initial page load. This happens automatically without calling any function in main.
|
|
These warnings still occur on |
There may be a way to tell re-frame when figwheel is reloading code so that we can ignore the warnings for overwriting handlers. This would reduce the noise in the console, and make it more likely we notice actual cases of this happening.
The text was updated successfully, but these errors were encountered: