From 7d5bf06c7814dd8134f1d5919e25945efe414dd8 Mon Sep 17 00:00:00 2001 From: Andrew Nichols Date: Sun, 28 Oct 2018 08:47:42 -0400 Subject: [PATCH] Avoid running after/enrich without a db effect Previously both standard interceptors acted upon the `db` *coffect* if no *effect* was available. Per #453 and related discussion, there is little reason in general to do this, since this context means that the `app-db` should not change. Both functions are changed to explicitly check for the presence of the `db` effect. If it is not present in the context, the interceptor function is not run. The previous functionality around `nil` and `false` values for the `db` effect is preserved: the interceptors *are* run in this case. Tests are added to verify the behavior of both functions in the case of non-presence, falsey and truthy values. The relevant docstrings are tweaked to accomodate this change. Closes #453. --- CHANGES.md | 3 ++ src/re_frame/std_interceptors.cljc | 27 +++++------ test/re_frame/interceptor_test.cljs | 72 ++++++++++++++++------------- 3 files changed, 54 insertions(+), 48 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 538fb6c79..1710541f4 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,8 @@ ## Unchanged +#### Fixed + + - `after` and `enrich` interceptors now no longer run if there is no `db` effect, rather than running against the `db` coffect. [#453](https://github.com/Day8/re-frame/issues/453) ## 0.10.6 (2018-09-03) diff --git a/src/re_frame/std_interceptors.cljc b/src/re_frame/std_interceptors.cljc index 619cf3319..1e04acb89 100644 --- a/src/re_frame/std_interceptors.cljc +++ b/src/re_frame/std_interceptors.cljc @@ -240,7 +240,7 @@ return a modified `db`. Unlike the `after` interceptor which is only about side effects, `enrich` - expects `f` to process and alter the given `db` coeffect in some useful way, + expects `f` to process and alter the given `db` effect in some useful way, contributing to the derived data, flowing vibe. Example Use: @@ -283,12 +283,11 @@ :id :enrich :after (fn enrich-after [context] - (let [event (get-coeffect context :event) - db (if (contains? (:effects context) :db) - (get-effect context :db) ;; If no db effect is returned, we provide the original coeffect. - (get-coeffect context :db))] - (->> (f db event) - (assoc-effect context :db)))))) + (let [event (get-coeffect context :event) + db-fx? (contains? (:effects context) :db) + db (get-effect context :db)] + (cond-> context + db-fx? (assoc-effect :db (f db event))))))) @@ -296,8 +295,7 @@ "returns an interceptor which runs a given function `f` in the `:after` position, presumably for side effects. - `f` is called with two arguments: the `:effects` value for `:db` - (or the `coeffect` value of db if no db effect is returned) and the event. + `f` is called with two arguments: the `:effects` value for `:db` and the event. Its return value is ignored, so `f` can only side-effect. Examples use can be seen in the /examples/todomvc: @@ -308,12 +306,11 @@ :id :after :after (fn after-after [context] - (let [db (if (contains? (:effects context) :db) - (get-in context [:effects :db]) - (get-in context [:coeffects :db])) - event (get-in context [:coeffects :event])] - (f db event) ;; call f for side effects - context)))) ;; context is unchanged + (let [event (get-coeffect context :event) + db-fx? (contains? (:effects context) :db) + db (get-effect context :db)] + (when db-fx? (f db event))) ;; call f for side effects + context))) ;; context is unchanged (defn on-changes diff --git a/test/re_frame/interceptor_test.cljs b/test/re_frame/interceptor_test.cljs index 7ea902b3b..dd4780008 100644 --- a/test/re_frame/interceptor_test.cljs +++ b/test/re_frame/interceptor_test.cljs @@ -136,42 +136,48 @@ ((:after change-i)) (get-effect :db ::not-found)))))) +(defn- intercept-with-db-effect + "Used to test if interceptors run given a db effect." + [interceptor-f db-fx add?] + (cond-> (context [:a :b] [(interceptor-f)] {:a 1}) + add? (assoc-effect :db db-fx) + true (interceptor/invoke-interceptors :before) + true interceptor/change-direction + true (interceptor/invoke-interceptors :after))) + (deftest test-after - (testing "when no db effect is returned" - (let [after-db-val (atom nil)] - (-> (context [:a :b] - [(after (fn [db] (reset! after-db-val db)))] - {:a 1}) - (interceptor/invoke-interceptors :before) - interceptor/change-direction - (interceptor/invoke-interceptors :after)) - (is (= @after-db-val {:a 1})))) - (testing "when a false db effect is returned" - (let [after-db-val (atom :not-reset)] - (-> (context [:a :b] - [(after (fn [db] (reset! after-db-val db)))] - {:a 2}) - (assoc-effect :db nil) - (interceptor/invoke-interceptors :before) - interceptor/change-direction - (interceptor/invoke-interceptors :after)) - (is (= @after-db-val nil)))) - (testing "when a nil db effect is returned" - (let [after-db-val (atom :not-reset)] - (-> (context [:a :b] - [(after (fn [db] (reset! after-db-val db)))] - {:a 3}) - (assoc-effect :db false) - (interceptor/invoke-interceptors :before) - interceptor/change-direction - (interceptor/invoke-interceptors :after)) - (is (= @after-db-val false))))) + (let [after-db-val (atom nil) + run-after (partial intercept-with-db-effect + #(after (fn [db] (reset! after-db-val db))))] + (testing "when no db effect is returned" + (reset! after-db-val :not-reset) + (run-after nil false) + (is (= @after-db-val :not-reset))) + (testing "when a nil db effect is returned" + (reset! after-db-val :not-reset) + (run-after nil true) + (is (= @after-db-val nil))) + (testing "when a false db effect is returned" + (reset! after-db-val :not-reset) + (run-after false true) + (is (= @after-db-val false))) + (testing "when a truthy db effect is returned" + (reset! after-db-val :not-reset) + (run-after :reset true) + (is (= @after-db-val :reset))))) (deftest test-enrich - (testing "when no db effect is returned" - (let [ctx (context [] [] {:a 1})] - (is (= ::not-found (get-effect ctx :db ::not-found))) - (-> ctx (:after (enrich (fn [db] (is (= db {:a 1}))))))))) + (let [run-enrich (partial intercept-with-db-effect + #(enrich (fn [db] (assoc db :enriched :added))))] + (testing "when no db effect is returned" + (let [ctx (run-enrich nil false)] + (is (= (-> ctx (get-effect :db) :enriched) nil)))) + (testing "when nil db effect is returned" + (let [ctx (run-enrich nil true)] + (is (= (-> ctx (get-effect :db) :enriched) :added)))) + (testing "when truthy db effect is returned" + (let [ctx (run-enrich {} true)] + (is (= (-> ctx (get-effect :db) :enriched) :added)))))) (deftest test-update-coeffect (let [context {:effects {:db {:a 1}}