Fix two possible bugs:

mike-thompson-day8 · mike-thompson-day8 · commit a1824c40de5c · 2016-08-18T17:07:53.000+10:00
1.  if an event triggered two rules, and if both rules have halt? true, then two halt events get dispatched
  2.  if two flows were used concurrently, AND they didn't supply an `:id`, then they would tread on each other's toes.
diff --git a/README.md b/README.md
@@ -13,7 +13,7 @@
 
 ## Async Control Flow In re-frame
 
-Herein a re-frame [Effects Handler](https://github.com/Day8/re-frame/wiki/Effectful-Event-Handlers), keyed `:async-flow`,
+Herein a re-frame [Effects Handler](https://github.com/Day8/re-frame/docs), keyed `:async-flow`,
 which wrangles async tasks.  It manages control flow at app boot time.
 
 ## Quick Start Guide
diff --git a/src/day8/re_frame/async_flow_fx.cljs b/src/day8/re_frame/async_flow_fx.cljs
@@ -4,7 +4,6 @@
     [clojure.set :as set]
     [day8.re-frame.forward-events-fx]))
 
-(def default-id  :async/flow)
 
 (defn seen-all-of?
   [required-events seen-events]
@@ -24,47 +23,43 @@
        (filterv (fn [task] ((:when task) (:events task) now-seen-events)))))
 
 
+(def map-when->fn {:seen?        seen-all-of?
+									 :seen-both?   seen-all-of?
+									 :seen-all-of? seen-all-of?
+									 :seen-any-of? seen-any-of?})
+
+(defn when->fn
+	[when-kw]
+	(if-let [when-fn (map-when->fn when-kw)]
+		when-fn
+		(re-frame/console :error  "async-flow: got bad value for :when - " when-kw)))
+
 (defn massage-rules
   "Massage the supplied rules as follows:
     - replace `:when` keyword value with a function implementing the predicate
     - ensure that only `:dispatch` or `:dispatch-n` is provided
-    - add a unique :id, if one not already present
-    - add halt event when :halt? present"
-  [flow-id rules]
-  (let [halt-event  [flow-id :halt-flow]
-        when->fn {:seen?        seen-all-of?
-                  :seen-both?   seen-all-of?
-                  :seen-all-of? seen-all-of?
-                  :seen-any-of? seen-any-of?}
-        add-halt (fn [tasks halt?]
-                   ; when rule represents stop, add `:halt-flow` as last event
-                   (if halt? (concat tasks [halt-event]) tasks))]
-    (->> rules
-         (map-indexed (fn [index {:keys [id when events dispatch dispatch-n halt?]}]
-                        (let [when-as-fn (when->fn when)
-                              _  (assert (cond [(some? dispatch) (some? dispatch-n)]
-                                               [false false] true ; either or both can be nil
-                                               [true false]  true ; only dispatch provided
-                                               [false true]  true ; only dispatch-n provided
-                                               false) "async-flow: rule can only specify one of :dispatch :dispatch-n")
-                              _  (assert (some? when-as-fn) (str "async-flow: found bad value for :when: " when))
-                              tasks (-> (cond
-                                          dispatch   (list dispatch)
-                                          dispatch-n dispatch-n
-                                          :else      nil)
-                                        (add-halt halt?))]
-                          {:id         (or id index)
-                           :when       when-as-fn
-                           :events     (if (coll? events) (set events) #{events})
-                           :dispatch-n tasks}))))))
-
-
-;; -- Create Event Handler
+    - add a unique :id, if one not already present"
+  [rules]
+	(->> rules
+			 (map-indexed (fn [index {:as rule :keys [id when events dispatch dispatch-n halt?]}]
+											{:id         (or id index)
+											 :halt?      (or halt? false)
+											 :when       (when->fn when)
+											 :events     (if (coll? events) (set events) (hash-set events))
+											 :dispatch-n (cond
+																		 dispatch-n (if dispatch
+																									(re-frame/console :error "async-flow: rule can only specify one of :dispatch and :dispatch-n. Got both: " rule)
+																									dispatch-n)
+																		 dispatch   (list dispatch)
+																		 :else      '())}))))
+
+
+;; -- Event Handler
 
 (defn make-flow-event-handler
-  "given a flow definitiion, returns an event handler which implements this definition"
+  "Given a flow definitiion, returns an event handler which implements this definition"
   [{:keys [id db-path rules first-dispatch]}]
-  (let [id          (or id default-id)
+  (let [
         ;; Subject to db-path, state is either stored in app-db or in a local atom
         ;; Two pieces of state are maintained:
         ;;  - the set of seen events
@@ -81,15 +76,16 @@
                       (fn [db] (get-in db db-path))
                       (fn [_] @local-store))
 
-        rules       (massage-rules id rules)]       ;; all of the events refered to in the rules
+        rules       (massage-rules rules)]       ;; all of the events refered to in the rules
 
     ;; Return an event handler which will manage the flow.
     ;; This event handler will receive 3 kinds of events:
     ;;   (dispatch [:id :setup])
     ;;   (dispatch [:id :halt-flow])
     ;;   (dispatch [:id [:forwarded :event :vector]])
     ;;
-    ;; This event handler returns a map of effects.
+    ;; This event handler returns a map of effects - it expects to be registered using
+		;; reg-event-fx
     ;;
     (fn async-flow-event-hander
       [{:keys [db]} event-v]
@@ -113,31 +109,44 @@
                     :forward-events           {:unregister id}
                     :deregister-event-handler id}
 
-        ;; Here we are managig the flow.
-        ;; A new event has been forwarded to this handler. What does it mean?
-        ;;  1. does this new event mean we need to dispatch another?
+        ;; Here we are managing the flow.
+        ;; A new event has been forwarded, so work out what should happen:
+        ;;  1. does this new event mean we should dispatch another?
         ;;  2. remember this event has happened
         (let [[_ [forwarded-event-id & args]] event-v
               {:keys [seen-events rules-fired]} (get-state db)
               new-seen-events (conj seen-events forwarded-event-id)
               ready-rules     (startable-rules rules new-seen-events rules-fired)
+							add-halt?       (some :halt? ready-rules)
               ready-rules-ids (->> ready-rules (map :id) set)
-              new-rules-fired (set/union rules-fired ready-rules-ids)]
+              new-rules-fired (set/union rules-fired ready-rules-ids)
+							new-dispatches  (cond-> (mapcat :dispatch-n ready-rules)
+																			add-halt? vec
+																			add-halt? (conj [id :halt-flow]))]
           (merge
             {:db       (set-state db new-seen-events new-rules-fired)}
-            (when (seq ready-rules) {:dispatch-n (mapcat :dispatch-n ready-rules)})))))))
+            (when (seq new-dispatches) {:dispatch-n new-dispatches})))))))
 
 
-;; -- Register effects handler with re-frame
+(defn- ensure-has-id
+	"Ensure `flow` has an id.
+	Return a vector of [id flow]"
+	[flow]
+	(if-let [id (:id flow)]
+		[id flow]
+		(let [new-id (keyword (str "async-flow/" (gensym "id-")))]
+			[new-id (assoc flow :id new-id)])))
 
-(defn flow->handler
-  [{:as flow :keys [id] :or {id default-id}}]
-  (re-frame/reg-event-fx
-    id                                ;; add debug middleware if dp-path set ???  XXX
-    (make-flow-event-handler flow))
-  (re-frame/console :log "starting async-flow:" id)
-  (re-frame/dispatch [id :setup]))
 
+;; -- Effect handler
+
+
+(defn flow->handler
+	"Action the given flow effect"
+  [flow]
+	(let [[id flow']  (ensure-has-id flow)]
+		(re-frame/reg-event-fx id (make-flow-event-handler flow'))   ;; register event handler
+		(re-frame/dispatch [id :setup])))                            ;; kicks things off
 
 (re-frame/reg-fx
   :async-flow
diff --git a/test/day8/re_frame/async_flow_fx_test.cljs b/test/day8/re_frame/async_flow_fx_test.cljs
@@ -36,28 +36,29 @@
 
 
 (deftest test-massage-rules
-  (is (= (core/massage-rules :my-id [{:when :seen? :events :1 :dispatch [:2]}])
-         (list {:id 0 :when core/seen-all-of? :events #{:1} :dispatch-n (list [:2])})))
+  (is (= (core/massage-rules [{:when :seen? :events :1 :dispatch [:2]}])
+         (list {:id 0 :when core/seen-all-of? :events #{:1} :halt? false :dispatch-n (list [:2])})))
 
-  (is (= (core/massage-rules :my-id [{:when :seen-both? :events [:1 :2] :halt? true}])
-         (list {:id 0 :when core/seen-all-of? :events #{:1 :2} :dispatch-n (list [:my-id :halt-flow])})))
+  (is (= (core/massage-rules [{:when :seen-both? :events [:1 :2] :halt? true}])
+         (list {:id 0 :when core/seen-all-of? :events #{:1 :2} :halt? true :dispatch-n '()})))
 
-  (is (= (core/massage-rules :my-id [{:when :seen-any-of? :events #{:1 :2} :dispatch [:2] :halt? true}])
-         (list {:id 0 :when core/seen-any-of? :events #{:1 :2} :dispatch-n (list [:2] [:my-id :halt-flow])}))))
+  (is (= (core/massage-rules [{:when :seen-any-of? :events #{:1 :2} :dispatch [:2] :halt? true}])
+         (list {:id 0 :when core/seen-any-of? :events #{:1 :2} :halt? true :dispatch-n (list [:2])}))))
 
 
 (deftest test-setup
-  (let [flow       {:first-dispatch [:1]
+  (let [flow       {:id             :some-id
+										:first-dispatch [:1]
                     :rules          [
                                      {:when :seen? :events :1 :dispatch [:2]}
                                      {:when :seen? :events :3 :halt? true}]}
         handler-fn (core/make-flow-event-handler flow)]
     (is (= (handler-fn {:db {}} [:dummy-id :setup])
            {:db             {}
             :dispatch       [:1]
-            :forward-events {:register     core/default-id
+            :forward-events {:register     :some-id
                               :events      #{:1 :3}
-                              :dispatch-to [core/default-id]}}))))
+                              :dispatch-to [:some-id]}}))))
 
 (deftest test-forwarding
   (let [flow {:first-dispatch [:start]
@@ -76,7 +77,7 @@
            {:db {:p {:seen-events #{:33 :no}
                      :rules-fired #{}}}}))
 
-    ;; new event should not cause a new dispatch because task is already started  (:id 0 is in ::rules-fired)
+    ;; new event should not cause a new dispatch because task is already started  (:id 0 is in :rules-fired)
     (is (= (handler-fn
              {:db {:p {:seen-events #{:1}
                        :rules-fired #{0}}}}
@@ -109,13 +110,14 @@
 
 
 (deftest test-halt1
-  (let [flow {:first-dispatch [:1]
+  (let [flow {:id :some-id
+							:first-dispatch [:1]
               :rules []}
         handler-fn   (core/make-flow-event-handler flow)]
     (is (= (handler-fn {:db {}} [:dummy-id :halt-flow])
            { ;; :db {}
-            :deregister-event-handler core/default-id
-            :forward-events           {:unregister core/default-id}}))))
+            :deregister-event-handler :some-id
+            :forward-events           {:unregister :some-id}}))))
 
 
 ;; Aggh. I don't have dissoc-in available to make this work.
