clj-commons / citrus

State management library for Rum
Eclipse Public License 1.0
274 stars 21 forks source link

Updating state with same values does not trigger render #30

Closed nijssels closed 6 years ago

nijssels commented 6 years ago

Currently, when a controller returns state that is exactly the same as the previous state, a re-render does not occur, because the state-change callback is not executed. The logic being: why re-render when there are no changes?

However, I currently have a case where re-render should definitely occur, even if new state is the same as the previous state. I will explain the usecase later, but for now a code sample to demonstrate the problem:

(def state {:controller "42"})

(defmulti controller (fn [event] event))

(defmethod controller :set [_ _ current]
  {:state current})

(def reconciler
  (citrus/reconciler
   {:state (atom state)
    :controllers {:controller controller}}))

(def subscription (citrus/subscription reconciler [:controller]))

(rum/defc Editor < rum/reactive [r]
  (let [text (rum/react subscription)]
    [:input {:type "text" :value text 
                          :on-change #(citrus/dispatch-sync! r :controller :set)}]))

(rum/mount (Editor reconciler)
           (. js/document (getElementById "app")))

The code sample contains a simplified example of the issue I am experiencing: I want to prevent the end-user from entering invalid data in an editbox. In case of the sample the behaviour should be that the user can never change the value in the textbox from "42" to something else.

However, if you run the sample, you will see that it's definitely possible to change the value in the textbox from 42 to something else.

The sample seems pretty nonsensical, but in a more reallife example: imagine you want the user to only be allowed to enter certain characters (numerical, or legal email adres characters for instance). Then you want to be able to revert to the old value.

The solution that currently works for me is changing the following lines of code in Citrus:

;; in citrus.reconciler [41 - 47]
  IWatchable
  (-add-watch [this key callback]
    (add-watch state (list this key)
      (fn [_ _ oldv newv]
        (callback key this oldv newv)))
        ;was
        ;(when (not= oldv newv)
        ;  (callback key this oldv newv))))
    this)

;; in citrus.cursor [24 - 32]
  IWatchable
  (-add-watch [this key callback]
    (add-watch ref (list this key)
      (fn [_ _ oldv newv]
        (let [old (reducer (get-in oldv path))
              new (reducer (get-in newv path))]
          (callback key this old new))))
          ;was
          ;(when (not= old new)
          ;  (callback key this old new)))))
    this)

Happy to provide a PR if okay with the solution.

nijssels commented 6 years ago

Anybody? This is in my opinion a major bug with a minor risk fix.

roman01la commented 6 years ago

I don't think it's a major issue. Perhaps it makes sense to not perform eq check in Citrus at all and let users do this via rum.core/static mixin where subscription is used.

For now you can force re-render manually:

(rum/defcs Editor <
  rum/reactive
  [state r]
  (let [text (rum/react subscription)]
    [:input {:value text 
             :on-change #(rum/request-render (:rum/react-component state))}]))