From d287376fa252a5c05f550a6b0ef01a6ee8ce294b Mon Sep 17 00:00:00 2001 From: dannyfreeman Date: Sun, 6 Mar 2022 10:22:15 -0500 Subject: [PATCH] Allow subscribe to be safely called from any context The `warn-when-not-reactive` warnings are no longer valid if we bypass the subscription cache when not in a reagent component, so they've been removed. This should address issue #753 This change probably requires some documentation cleanup --- src/re_frame/subs.cljc | 52 ++++++++++++++----------------- test/re_frame/subs_test.cljs | 59 +++++++++++++++++++++++++----------- 2 files changed, 64 insertions(+), 47 deletions(-) diff --git a/src/re_frame/subs.cljc b/src/re_frame/subs.cljc index 86149d149..a172a2741 100644 --- a/src/re_frame/subs.cljc +++ b/src/re_frame/subs.cljc @@ -35,25 +35,29 @@ (defn cache-and-return "cache the reaction r" [query-v dynv r] - (let [cache-key [query-v dynv]] - ;; when this reaction is no longer being used, remove it from the cache - (add-on-dispose! r #(trace/with-trace {:operation (first-in-vector query-v) - :op-type :sub/dispose - :tags {:query-v query-v - :reaction (reagent-id r)}} - (swap! query->reaction - (fn [query-cache] - (if (and (contains? query-cache cache-key) (identical? r (get query-cache cache-key))) - (dissoc query-cache cache-key) - query-cache))))) - ;; cache this reaction, so it can be used to deduplicate other, later "=" subscriptions - (swap! query->reaction (fn [query-cache] - (when debug-enabled? - (when (contains? query-cache cache-key) - (console :warn "re-frame: Adding a new subscription to the cache while there is an existing subscription in the cache" cache-key))) - (assoc query-cache cache-key r))) - (trace/merge-trace! {:tags {:reaction (reagent-id r)}}) - r)) ;; return the actual reaction + ;; Only cache the reaction when subscribe is called from a reactive context + ;; where a reagent component will be able to purge the reaction from the subscription cache + ;; when the component is dismounted. + (when (reactive?) + (let [cache-key [query-v dynv]] + ;; when this reaction is no longer being used, remove it from the cache + (add-on-dispose! r #(trace/with-trace {:operation (first-in-vector query-v) + :op-type :sub/dispose + :tags {:query-v query-v + :reaction (reagent-id r)}} + (swap! query->reaction + (fn [query-cache] + (if (and (contains? query-cache cache-key) (identical? r (get query-cache cache-key))) + (dissoc query-cache cache-key) + query-cache))))) + ;; cache this reaction, so it can be used to deduplicate other, later "=" subscriptions + (swap! query->reaction (fn [query-cache] + (when debug-enabled? + (when (contains? query-cache cache-key) + (console :warn "re-frame: Adding a new subscription to the cache while there is an existing subscription in the cache" cache-key))) + (assoc query-cache cache-key r))) + (trace/merge-trace! {:tags {:reaction (reagent-id r)}}))) + r) ;; Always return the actual reaction, even if it was not cached. (defn cache-lookup ([query-v] @@ -63,17 +67,8 @@ ;; -- subscribe --------------------------------------------------------------- -(defn warn-when-not-reactive - [] - (when (and debug-enabled? (not (reactive?))) - (console :warn - "re-frame: Subscribe was called outside of a reactive context.\n" - "See: https://day8.github.io/re-frame/FAQs/UseASubscriptionInAJsEvent/\n" - "https://day8.github.io/re-frame/FAQs/UseASubscriptionInAnEventHandler/"))) - (defn subscribe ([query] - (warn-when-not-reactive) (trace/with-trace {:operation (first-in-vector query) :op-type :sub/create :tags {:query-v query}} @@ -92,7 +87,6 @@ (cache-and-return query [] (handler-fn app-db query))))))) ([query dynv] - (warn-when-not-reactive) (trace/with-trace {:operation (first-in-vector query) :op-type :sub/create :tags {:query-v query diff --git a/test/re_frame/subs_test.cljs b/test/re_frame/subs_test.cljs index 2efa7ccde..4f5d21330 100644 --- a/test/re_frame/subs_test.cljs +++ b/test/re_frame/subs_test.cljs @@ -3,6 +3,7 @@ [reagent.ratom :as r :refer-macros [reaction]] [re-frame.subs :as subs] [re-frame.db :as db] + [re-frame.interop] [re-frame.core :as re-frame])) (test/use-fixtures :each {:before (fn [] (subs/clear-all-handlers!))}) @@ -87,16 +88,37 @@ (swap! side-effect-atom inc) (reaction @db))) - (let [test-sub (subs/subscribe [:side-effecting-handler])] - (reset! db/app-db :test) - (is (= :test @test-sub)) - (is (= @side-effect-atom 1)) - (subs/subscribe [:side-effecting-handler]) ;; this should be handled by cache - (is (= @side-effect-atom 1)) - (subs/subscribe [:side-effecting-handler :a]) ;; should fire again because of the param - (is (= @side-effect-atom 2)) - (subs/subscribe [:side-effecting-handler :a]) ;; this should be handled by cache - (is (= @side-effect-atom 2)))) + (testing "Inside of a reactive context" + (with-redefs [re-frame.interop/reactive? (constantly true)] + (let [test-sub (subs/subscribe [:side-effecting-handler])] + (reset! db/app-db :test) + (is (= :test @test-sub)) + (is (= @side-effect-atom 1)) + (subs/subscribe [:side-effecting-handler]) ;; this should be handled by cache + (is (= @side-effect-atom 1)) + (subs/subscribe [:side-effecting-handler :a]) ;; should fire again because of the param + (is (= @side-effect-atom 2)) + (subs/subscribe [:side-effecting-handler :a]) ;; this should be handled by cache + (is (= @side-effect-atom 2))))) + + (subs/clear-subscription-cache!) + (reset! side-effect-atom 0) + + (testing "Outside a reactive context" + (let [test-sub (subs/subscribe [:side-effecting-handler])] + (is (= :test @test-sub)) + (is (= @side-effect-atom 1) + "Subscribed once") + (subs/subscribe [:side-effecting-handler]) + (is (= @side-effect-atom 2) + "Subscribed twice and the cache was never populated.") + (with-redefs [re-frame.interop/reactive? (constantly true)] + (subs/subscribe [:side-effecting-handler :a]) + (is (= @side-effect-atom 3) + "Fires again because of the param, but this time the cache is populated")) + (subs/subscribe [:side-effecting-handler :a]) + (is (= @side-effect-atom 3) + "The cache was already populated, so the cached reaction is used.")))) ;============== test clear-subscription-cache! ================ @@ -105,14 +127,15 @@ :clear-subscription-cache! (fn clear-subs-cache [db _] 1)) - (testing "cold cache" - (is (nil? (subs/cache-lookup [:clear-subscription-cache!])))) - (testing "cache miss" - (is (= 1 @(subs/subscribe [:clear-subscription-cache!]))) - (is (some? (subs/cache-lookup [:clear-subscription-cache!])))) - (testing "clearing" - (subs/clear-subscription-cache!) - (is (nil? (subs/cache-lookup [:clear-subscription-cache!]))))) + (with-redefs [re-frame.interop/reactive? (constantly true)] + (testing "cold cache" + (is (nil? (subs/cache-lookup [:clear-subscription-cache!])))) + (testing "cache miss" + (is (= 1 @(subs/subscribe [:clear-subscription-cache!]))) + (is (some? (subs/cache-lookup [:clear-subscription-cache!])))) + (testing "clearing" + (subs/clear-subscription-cache!) + (is (nil? (subs/cache-lookup [:clear-subscription-cache!])))))) ;============== test register-pure macros ================