Merge pull request metosin#438 from metosin/frontend-fixes

Frontend fixes
cheolhee · Oct 19, 2020 · f2f7d3a · f2f7d3a
2 parents ff647f3 + 845240d
commit f2f7d3a
Show file tree

Hide file tree

Showing 6 changed files with 74 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,11 @@ We use [Break Versioning][breakver]. The version numbers follow a `<major>.<mino
 
 ## 0.5.7 (2020-10-18)
 
+### `reitit-frontend`
+
+- `reitit.frontend.easy/start!` now correctly removes old event listeners
+when called repeatedly (e.g. with hot code reload workflow)
+([#438](https://github.com/metosin/reitit/pull/438))
 * updated deps:
 
 ```clj

diff --git a/examples/frontend-re-frame/project.clj b/examples/frontend-re-frame/project.clj
@@ -33,7 +33,7 @@
   {:builds
    [{:id           "dev"
      :source-paths ["src/cljs"]
-     :figwheel     {:on-jsload "frontend-re-frame.core/mount-root"}
+     :figwheel     true
      :compiler     {:main                 frontend-re-frame.core
                     :output-to            "resources/public/js/compiled/app.js"
                     :output-dir           "resources/public/js/compiled/out"

diff --git a/examples/frontend-re-frame/resources/public/index.html b/examples/frontend-re-frame/resources/public/index.html
@@ -2,12 +2,9 @@
 <html lang="en">
   <head>
     <meta charset='utf-8'>
-
-
   </head>
   <body>
     <div id="app"></div>
     <script src="js/compiled/app.js"></script>
-    <script>frontend_re_frame.core.init();</script>
   </body>
 </html>
diff --git a/examples/frontend-re-frame/src/cljs/frontend_re_frame/core.cljs b/examples/frontend-re-frame/src/cljs/frontend_re_frame/core.cljs
@@ -9,30 +9,27 @@
 
 ;;; Events ;;;
 
-(re-frame/reg-event-db
- ::initialize-db
- (fn [_ _]
-   {:current-route nil}))
-
-(re-frame/reg-event-fx
- ::navigate
- (fn [db [_ & route]]
-   ;; See `navigate` effect in routes.cljs
-   {::navigate! route}))
-
-(re-frame/reg-event-db
- ::navigated
- (fn [db [_ new-match]]
-   (let [old-match   (:current-route db)
-         controllers (rfc/apply-controllers (:controllers old-match) new-match)]
-     (assoc db :current-route (assoc new-match :controllers controllers)))))
+(re-frame/reg-event-db ::initialize-db
+  (fn [db _]
+    (if db
+      db
+      {:current-route nil})))
+
+(re-frame/reg-event-fx ::push-state
+  (fn [db [_ & route]]
+    {:push-state route}))
+
+(re-frame/reg-event-db ::navigated
+  (fn [db [_ new-match]]
+    (let [old-match   (:current-route db)
+          controllers (rfc/apply-controllers (:controllers old-match) new-match)]
+      (assoc db :current-route (assoc new-match :controllers controllers)))))
 
 ;;; Subscriptions ;;;
 
-(re-frame/reg-sub
- ::current-route
- (fn [db]
-   (:current-route db)))
+(re-frame/reg-sub ::current-route
+  (fn [db]
+    (:current-route db)))
 
 ;;; Views ;;;
 
@@ -41,7 +38,7 @@
    [:h1 "This is home page"]
    [:button
     ;; Dispatch navigate event that triggers a (side)effect.
-    {:on-click #(re-frame/dispatch [::navigate ::sub-page2])}
+    {:on-click #(re-frame/dispatch [::push-state ::sub-page2])}
     "Go to sub-page 2"]])
 
 (defn sub-page1 []
@@ -55,10 +52,10 @@
 ;;; Effects ;;;
 
 ;; Triggering navigation from events.
-(re-frame/reg-fx
- ::navigate!
- (fn [route]
-   (apply rfe/push-state route)))
+
+(re-frame/reg-fx :push-state
+  (fn [route]
+    (apply rfe/push-state route)))
 
 ;;; Routes ;;;
 
@@ -104,15 +101,15 @@
 
 (def router
   (rf/router
-   routes
-   {:data {:coercion rss/coercion}}))
+    routes
+    {:data {:coercion rss/coercion}}))
 
 (defn init-routes! []
   (js/console.log "initializing routes")
   (rfe/start!
-   router
-   on-navigate
-   {:use-fragment true}))
+    router
+    on-navigate
+    {:use-fragment true}))
 
 (defn nav [{:keys [router current-route]}]
   [:ul
@@ -141,13 +138,12 @@
     (enable-console-print!)
     (println "dev mode")))
 
-(defn mount-root []
+(defn init []
   (re-frame/clear-subscription-cache!)
+  (re-frame/dispatch-sync [::initialize-db])
+  (dev-setup)
   (init-routes!) ;; Reset routes on figwheel reload
   (reagent/render [router-component {:router router}]
                   (.getElementById js/document "app")))
 
-(defn ^:export init []
-  (re-frame/dispatch-sync [::initialize-db])
-  (dev-setup)
-  (mount-root))
+(init)
diff --git a/modules/reitit-frontend/src/reitit/frontend/history.cljs b/modules/reitit-frontend/src/reitit/frontend/history.cljs
@@ -26,11 +26,14 @@
                     (let [path (-get-path this)]
                       (when (or (= goog.events.EventType.POPSTATE (.-type e))
                                 (not= @last-fragment path))
-                        (-on-navigate this path))))]
+                        (-on-navigate this path))))
+          ;; rfe start! uses first on-navigate call to store the
+          ;; instance so it has to see the instance with listeners.
+          this (assoc this
+                      :popstate-listener (gevents/listen js/window goog.events.EventType.POPSTATE handler false)
+                      :hashchange-listener (gevents/listen js/window goog.events.EventType.HASHCHANGE handler false))]
       (-on-navigate this (-get-path this))
-      (assoc this
-             :popstate-listener (gevents/listen js/window goog.events.EventType.POPSTATE handler false)
-             :hashchange-listener (gevents/listen js/window goog.events.EventType.HASHCHANGE handler false))))
+      this))
   (-stop [this]
     (gevents/unlistenByKey popstate-listener)
     (gevents/unlistenByKey hashchange-listener)
@@ -115,11 +118,12 @@
                                                       (when (.hasFragment uri)
                                                         (str "#" (.getFragment uri))))]
                                         (.pushState js/window.history nil "" path)
-                                        (-on-navigate this path))))))]
+                                        (-on-navigate this path))))))
+          this (assoc this
+                      :listen-key (gevents/listen js/window goog.events.EventType.POPSTATE handler false)
+                      :click-listen-key (gevents/listen js/document goog.events.EventType.CLICK ignore-anchor-click))]
       (-on-navigate this (-get-path this))
-      (assoc this
-             :listen-key (gevents/listen js/window goog.events.EventType.POPSTATE handler false)
-             :click-listen-key (gevents/listen js/document goog.events.EventType.CLICK ignore-anchor-click))))
+      this))
   (-on-navigate [this path]
     (on-navigate (rf/match-by-path router path) this))
   (-stop [this]

diff --git a/test/cljs/reitit/frontend/easy_test.cljs b/test/cljs/reitit/frontend/easy_test.cljs
@@ -12,6 +12,8 @@
                        ["foo" ::foo]
                        ["bar/:id" ::bar]]))
 
+;; TODO: Only tests fragment history, also test HTML5?
+
 (deftest easy-history-routing-test
   (when browser
     (gevents/removeAll js/window goog.events.EventType.POPSTATE)
@@ -24,7 +26,8 @@
                     (fn on-navigate [match history]
                       (let [url (rfh/-get-path history)]
                         (case (swap! n inc)
-                          1 (do (is (= "/" url)
+                          1 (do (is (some? (:popstate-listener history)))
+                                (is (= "/" url)
                                     "start at root")
                                 (rfe/push-state ::foo))
                           2 (do (is (= "/foo" url)
@@ -41,7 +44,24 @@
                                 (.back js/window.history))
                           6 (do (is (= "/" url)
                                     "go back after replace state")
-                                (rfh/stop! @rfe/history)
-                                (done))
-                          (is false "extra event"))))
+
+                                ;; Reset to ensure old event listeners aren't called
+                                (rfe/start! router
+                                            (fn on-navigate [match history]
+                                              (let [url (rfh/-get-path history)]
+                                                (case (swap! n inc)
+                                                  7 (do (is (= "/" url)
+                                                            "start at root")
+                                                        (rfe/push-state ::foo))
+                                                  8 (do (is (= "/foo" url)
+                                                            "push-state")
+                                                        (rfh/stop! @rfe/history)
+                                                        (done))
+                                                  (do
+                                                    (is false (str "extra event 2" {:n @n, :url url}))
+                                                    (done)))))
+                                            {:use-fragment true}))
+                          (do
+                            (is false (str "extra event 1" {:n @n, :url url}))
+                            (done)))))
                     {:use-fragment true})))))