reagent-project / reagent-forms

Bootstrap form components for Reagent
339 stars 78 forks source link

Using the datepicker with re-frame #138

Closed madstap closed 6 years ago

madstap commented 6 years ago

I having some issues using the datepicker together with re-frame.

The arity of :update! seems wrong.

The docs want me to do :update! (fn [id val] (re-frame/dispatch [id val])), but it looks like it should have the arity [id save-fn val]. Is this simple oversight in the docs?

What should be the arity of save-fn? [old-value-if-exists new-value]?

Also, is the datepicker the only field that uses :update!?

The difference between using save-fn and in-fn/out-fn

When would I use save-fn over in-fn/out-fn ? From the examples it looks like they serve roughly the same purpose.

in-fn/out-fn seems broken

(Maybe this should go in its own issue?)

When I try using this I get the warning Warning: Unknown props `inFn`, `outFn` on <input> tag. Remove these props from the element. and the functions are never called.

yogthos commented 6 years ago

That looks like it might just be a type in the docs regarding :update. The difference between save and update is basically same as with reset! and swap! on atoms. The save function simply sets the new value, while update passes the existing value to the update function, and sets the result. This is needed in order to support for save-fn in the datepicker described here.

I think @smogg can speak to in-fn/out-fn behavior better as he authored the pr. The warnings you're seeing should be fixed, that was just a matter of cleaning out custom attributes before passing the component to React.

madstap commented 6 years ago

Here's a repro of the issue I'm seeing with in/out-fn.


(ns foo.bar
  (:require
   [re-frame.core :as rf]
   [reagent.core :as r]
   [reagent-forms.core :as rforms]))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; vanilla reagent

(def state (r/atom {:foo "foobar"}))

(defn form* []
  [:div
   [:p (pr-str @state)]
   [rforms/bind-fields
    [:input.form-control {:field :text
                          :id :foo
                          :in-fn (fn [x] (prn "in " x) (subs x 3))
                          :out-fn (fn [x] (prn "out" x) (str "foo" x))}]
    state]])

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; re-frame

(rf/reg-event-db :foo/bar
  (fn [db [id v]]
    (assoc db id v)))

(rf/reg-sub :foo/bar
  (fn [db [id]]
    (get db id)))

(defn form []
  [:div
   [:p (pr-str @(rf/subscribe [:foo/bar]))]
   [rforms/bind-fields
    [:input.form-control {:field :text
                          :id :foo/bar
                          :in-fn (fn [x] (when x (prn "in " x) (subs x 3)))
                          :out-fn (fn [x] (when x (prn "out" x) (str "foo" x)))}]
    {:get (fn [id] @(rf/subscribe [id]))
     :save! (fn [id val] (rf/dispatch [id val]))
     :update! (fn [id f val] (rf/dispatch [id (f val)]))}]])

The reagent version works like I would expect, but in the re-frame version the in and out fns never get called and the string is saved as-is, without prepending "foo".

madstap commented 6 years ago

Ok, so the behavior I'm seeing comes from here, where the re-frame version has wrap-fns? set to false. If I set this to true it seemingly behaves like I expect it to.

yogthos commented 6 years ago

Yeah I think the re-frame support needs a bit more work. I've also noticed that the :doc key gets initialized in a way that's incompatible with a couple of fields where they try to deref it. It needs to be a subscription to the entire document state.

madstap commented 6 years ago

To make save-fn in re-frame work like the vanilla reagent version, I needed to do the following.

(defn date []
  [:div
   [:p (pr-str @(rf/subscribe [:foo/quux]))]
   [rforms/bind-fields
    [:input.form-control {:field :datepicker
                          :id :foo/quux
                          :date-format (fn [date]
                                         (str date))
                          :save-fn (fn [current-date {:keys [year month day]}]
                                     (if current-date
                                       (doto (js/Date.)
                                         (.setFullYear year)
                                         (.setMonth (dec month))
                                         (.setDate day)
                                         (.setHours (.getHours current-date))
                                         (.setMinutes (.getMinutes current-date)))
                                       (js/Date. year (dec month) day)))}]
    {:get (fn [id] @(rf/subscribe [id]))
     :save! (fn [id val] (rf/dispatch [id val]))
     :update! (fn [id f new-val]
                (let [old-val @(rf/subscribe [id])]
                  (rf/dispatch [id (f old-val new-val)])))}]])

Which works fine, but I'd expect the :update! function to be a pure function with the argument list [id f old new].

yogthos commented 6 years ago

Sure, that would be a pretty simple tweak to make.

smogg commented 6 years ago

Apologies for the delay but I just got back from vacation. Thanks for starting the issue @madstap, you're right - wrap-fns shouldn't be skipped. I missed the fact it matters for the things you've mentioned. I'll add a PR fixing it in a few and I'll look into update-fn! when I have more time. I tested re-frame with what's in page.cljs, but it looks like this dev file doesn't cover all the features of the project. I'll try to create similar dev environment for testing re-frame compatibility (maybe using devcards?).

yogthos commented 6 years ago

Devcards might be a nice way to do this. Just adding more test cases on the page is probably fine for now though.

yogthos commented 6 years ago

I pushed out a new version with the fixes, @madstap let us know if that looks better

madstap commented 6 years ago

Tried it out, seems to work.

There's still the issue of the :update! function having the arity [id f new-val] where I would expect it to be [id f old-val new-val]. Is there consensus on this being better? In any case the docs are wrong.

smogg commented 6 years ago

I agree your proposed arity is better. I'll try to get to it this week.

yogthos commented 6 years ago

Pushed out a new version that should work with datepicker as expected. For now, the :update! function API is the same, so the re-frame events maps should looks as follows:

(def event-fns
  {:get (fn [path] @(re-frame/subscribe [:value path]))
   :save! (fn [path value] (re-frame/dispatch [:set-value path value]))
   :update! (fn [path save-fn value]
              (re-frame/dispatch [:set-value path
                                  (save-fn @(re-frame/subscribe [:value path]) value)]))
   :doc (fn [] @(re-frame/subscribe [:doc]))})
madstap commented 6 years ago

I tried the new version. I'm unsure about the semantics of the doc function in the context of the (maybe wrong?) way I'm using the lib though. Instead of calling bind-fields on an entire form, I call it in each component I create. This way I can use the values from each field elsewhere in the form. This makes it straightforward to build things like master-detail forms (#12)

(ns foo.bar
  (:require
   [reagent-forms.core :as rforms]
   [re-frame.core :as rf]))

(rf/reg-event-db :form/set
  [rf/trim-v]
  (fn [db [f-id id v]]
    (assoc-in db [:form/data f-id id] v)))

(rf/reg-sub :form/data
  (fn [db [_ form-id id]]
    (get-in db (remove nil? [:form/data form-id id]))))

(defn make-event-fns [form-id]
  {:get (fn [[id]] @(rf/subscribe [:form/data form-id id]))
   :save! (fn [[id] val] (rf/dispatch [:form/set form-id id val]))
   :update! (fn [[id] f new-val]
              (let [old-val @(rf/subscribe [:form/data form-id id])]
                (rf/dispatch [:form/set form-id id (f old-val new-val)])))
   :doc (fn [] @(rf/subscribe [:form/data form-id]))})

(defn number-input [{[form-id id] :ids}]
  [rforms/bind-fields [:input {:field :numeric, :id id}] (make-event-fns form-id)])

(defn foo-form []
  ;; This is conceptually one form called ::foo with
  ;; :account/saving and :account/checking fields.
  [:div
   [number-input {:ids [::foo :account/savings]}]
   [number-input {:ids [::foo :account/checking]}]
   ;; I can use the values of each field inside the form, which I wouldn't
   ;; be able to do if this was enclosed by a single bind-fields
   [:div (str "Sum: " (+ @(rf/subscribe [:form/data ::foo :account/savings])
                         @(rf/subscribe [:form/data ::foo :account/checking])))]

   ;; I can also access the whole form
   [:div (str "Debug: " (pr-str @(rf/subscribe [:form/data ::foo])))]])

So each field is its own reagent-forms form, but the :doc function will return a map containing all the fields in the conceptual form.

yogthos commented 6 years ago

Your approach seems perfectly fine to me. Creating multiple bindings for individual components gives you a lot of flexibility.

The :doc function is used to provide the state of the entire document. Places where that's useful is when behavior of one field depends on the state of another field. For example, only show part of the form if a particular checkbox is checked off. A silly example might be something like the following:

[:div
 [:input {:field :text
         :id :person.name.first
         :validator (fn [doc]
                      (when (= "Bob" (-> doc :person :name :first))
                        ["error"]))}]
 [:input {:field :text
         :id :person.name.last
         :set-attributes (fn [doc attrs]
                           (assoc attrs :disabled (= "Bob" (-> doc :person :name :first))))}]]

The second field is disabled when the first field has value Bob, the :set-attributes function needs access to the document to check the other field in this case.