Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow subscribe to be safely called from any context #754

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow subscribe to be safely called from any context #754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 23 additions & 29 deletions src/re_frame/subs.cljc
View file
Open in desktop
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
Open in desktop
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 "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! ================

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