Dispose of subscriptions that were created after restore fn

Fixes #223

CHANGES.md

@@ -16,6 +16,7 @@
 - Add `re-frame.loggers/get-loggers` function to well, you know.
 - Added `clear-subscription-cache!` function. This should be used when hot reloading code to ensure that any bad subscriptions that cause rendering exceptions are removed. See [reagent-project/reagent#272](https://github.com/reagent-project/reagent/issues/272) for more details.
 - Added experimental tracing features. These are subject to change and remain undocumented at the moment. By default they are disabled, and will be completely compiled out by advanced optimisations. To enable them, set a [`:closure-defines`](https://www.martinklepsch.org/posts/parameterizing-clojurescript-builds.html) key to `{"re_frame.trace.trace_enabled_QMARK_" true}`
+- [#223](https://github.com/Day8/re-frame/issues/223) When using `make-restore-fn`, dispose of any subscriptions that were created after the restore function was created.
 
 #### Fixes
 

src/re_frame/core.cljc

@@ -12,7 +12,8 @@
     [re-frame.interceptor      :as interceptor]
     [re-frame.std-interceptors :as std-interceptors :refer [db-handler->interceptor
                                                              fx-handler->interceptor
-                                                             ctx-handler->interceptor]]))
+                                                             ctx-handler->interceptor]]
+    [clojure.set               :as set]))
 
 
 ;; --  dispatch
@@ -134,14 +135,14 @@
     (fn []
 			;; call `dispose!` on all current subscriptions which
 			;; didn't originally exist.
-			#_(->> subs/query->reaction
-					 vals
-					 (remove (set (vals subs-cache)))   ;;
-					 (map interop/dispose!)
-					 (doall))
+      (let [original-subs (set (vals subs-cache))
+            current-subs  (set (vals @subs/query->reaction))]
+        (doseq [sub (set/difference current-subs original-subs)]
+          (interop/dispose! sub)))
 
-			;; reset the atoms
-      (reset! subs/query->reaction subs-cache)
+      ;; Reset the atoms
+      ;; We don't need to reset subs/query->reaction, as
+      ;; disposing of the subs removes them from the cache anyway
       (reset! registrar/kind->id->handler handlers)
       (reset! db/app-db app-db)
       nil)))

test/re-frame/restore_test.cljs

@@ -0,0 +1,74 @@
+(ns re-frame.restore-test
+  (:require [cljs.test :refer-macros [is deftest async use-fixtures testing]]
+            [re-frame.core :refer [make-restore-fn reg-sub subscribe]]
+            [re-frame.subs :as subs]))
+
+;; TODO: future tests in this area could check DB state and registrations are being correctly restored.
+
+(use-fixtures :each {:before subs/clear-all-handlers!})
+
+(defn one? [x] (= 1 x))
+(defn two? [x] (= 2 x))
+
+(defn register-test-subs []
+  (reg-sub
+    :test-sub
+    (fn [db ev]
+      (:test-sub db)))
+
+  (reg-sub
+    :test-sub2
+    (fn [db ev]
+      (:test-sub2 db))))
+
+(deftest make-restore-fn-test
+  (testing "no existing subs, then making one subscription"
+    (register-test-subs)
+    (let [original-subs @subs/query->reaction
+          restore-fn    (make-restore-fn)]
+      (is (zero? (count original-subs)))
+      @(subscribe [:test-sub])
+      (is (one? (count @subs/query->reaction)))
+      (is (contains? @subs/query->reaction [[:test-sub] []]))
+      (restore-fn)
+      (is (zero? (count @subs/query->reaction))))))
+
+(deftest make-restore-fn-test2
+  (testing "existing subs, making more subscriptions"
+    (register-test-subs)
+    @(subscribe [:test-sub])
+    (let [original-subs @subs/query->reaction
+          restore-fn    (make-restore-fn)]
+      (is (one? (count original-subs)))
+      @(subscribe [:test-sub2])
+      (is (contains? @subs/query->reaction [[:test-sub2] []]))
+      (is (two? (count @subs/query->reaction)))
+      (restore-fn)
+      (is (not (contains? @subs/query->reaction [[:test-sub2] []])))
+      (is (one? (count @subs/query->reaction))))))
+
+(deftest make-restore-fn-test3
+  (testing "existing subs, making more subscriptions with different params on same subscriptions"
+    (register-test-subs)
+    @(subscribe [:test-sub])
+    (let [original-subs @subs/query->reaction
+          restore-fn    (make-restore-fn)]
+      (is (one? (count original-subs)))
+      @(subscribe [:test-sub :extra :params])
+      (is (two? (count @subs/query->reaction)))
+      (restore-fn)
+      (is (one? (count @subs/query->reaction))))))
+
+(deftest nested-restores
+  (testing "running nested restores"
+    (register-test-subs)
+    (let [restore-fn-1 (make-restore-fn)
+          _            @(subscribe [:test-sub])
+          _            (is (one? (count @subs/query->reaction)))
+          restore-fn-2 (make-restore-fn)]
+      @(subscribe [:test-sub2])
+      (is (two? (count @subs/query->reaction)))
+      (restore-fn-2)
+      (is (one? (count @subs/query->reaction)))
+      (restore-fn-1)
+      (is (zero? (count @subs/query->reaction))))))

test/re-frame/test_runner.cljs

@@ -7,7 +7,8 @@
     [re-frame.interceptor-test]
     [re-frame.subs-test]
     [re-frame.fx-test]
-    [re-frame.trace-test]))
+    [re-frame.trace-test]
+    [re-frame.restore-test]))
 
 (enable-console-print!)
 
@@ -21,7 +22,8 @@
     're-frame.interceptor-test
     're-frame.subs-test
     're-frame.fx-test
-    're-frame.trace-test))
+    're-frame.trace-test
+    're-frame.restore-test))
 
 ;; ---- KARMA  -----------------------------------------------------------------
 
@@ -31,4 +33,5 @@
     're-frame.interceptor-test
     're-frame.subs-test
     're-frame.fx-test
-    're-frame.trace-test))
+    're-frame.trace-test
+    're-frame.restore-test))