Skip to content

Commit

Permalink
Allow subscribe to be safely called from any context
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@dannyfreeman
dannyfreeman committed Mar 6, 2022
1 parent 69cf395 commit 742be81
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 47 deletions.
52 changes: 23 additions & 29 deletions src/re_frame/subs.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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}}
Expand All @@ -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
Expand Down
59 changes: 41 additions & 18 deletions test/re_frame/subs_test.cljs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!))})
Expand Down Expand Up @@ -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 "Instead 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! ================

Expand All @@ -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 ================

Expand Down

0 comments on commit 742be81

Please sign in to comment.