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.

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

Using the datepicker with re-frame #138

Closed
madstap opened this issue Mar 12, 2018 · 14 comments
Closed

Using the datepicker with re-frame #138

madstap opened this issue Mar 12, 2018 · 14 comments

Comments

@madstap
Copy link
Contributor

madstap commented Mar 12, 2018

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
Copy link
Member

yogthos commented Mar 12, 2018

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
Copy link
Contributor Author

madstap commented Mar 13, 2018

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
Copy link
Contributor Author

madstap commented Mar 13, 2018

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
Copy link
Member

yogthos commented Mar 13, 2018

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
Copy link
Contributor Author

madstap commented Mar 13, 2018

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
Copy link
Member

yogthos commented Mar 13, 2018

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

@smogg
Copy link
Collaborator

smogg commented Mar 14, 2018

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
Copy link
Member

yogthos commented Mar 14, 2018

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

@yogthos
Copy link
Member

yogthos commented Mar 14, 2018

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

@madstap
Copy link
Contributor Author

madstap commented Mar 14, 2018

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
Copy link
Collaborator

smogg commented Mar 14, 2018

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

@yogthos
Copy link
Member

yogthos commented Mar 17, 2018

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
Copy link
Contributor Author

madstap commented Mar 18, 2018

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.

  • Is it a problem if the doc function returns a map with extra keys?
  • Why is the doc function needed if you can get the same data through the :get function?

@yogthos
Copy link
Member

yogthos commented Mar 18, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants