Uncontrolled input if `:onChange` is specified

[jonase]

If an input uses the :onChange callback it is uncontrolled even if :value is specified. Here's a small reproducible test:

(ns om-controlled.core
  (:require [om.core :as om :include-macros true]
            [om.dom :as dom :include-macros true]))

(enable-console-print!)

(def app-state (atom {}))

(defn controlled-input [{:keys [text]}]
  (reify
    om/IRender
    (render [this]
      (dom/input #js {:type "text" :value text}))))

(defn uncontrolled-input [{:keys [text]}]
  (reify
    om/IRender
    (render [this]
      (dom/input #js {:type "text" :value text :onChange identity}))))

(defn todo [data owner]
  (reify
    om/IRender
    (render [this]
      (dom/ul nil
              (dom/li nil (dom/input #js {:type "text" :value "controlled"}))
              (dom/li nil (om/build controlled-input {:text "controlled"}))
              (dom/li nil (om/build uncontrolled-input {:text "uncontrolled"}))))))

(om/root
  todo
  app-state
  {:target (. js/document (getElementById "app"))})

In React all three inputs are controlled (http://jsfiddle.net/69z2wepo/220/)

[GetContented]

Please excuse my lack of knowledge, but isn't that because you're using render instead of render-state?

[jonase]

@JulianLeviston Sorry, I don't see what Render vs. RenderState has to do with this. Could you explain what you mean?

@swannodette Looking at the source it seems that this behaviour is intentional. What is the rationale for this? It means that you can't do standard React things like

(dom/input #js {:type "text" :value (:text app)
                      :onChange #(let [new-value (-> % .-target .-value)]
                                   (when (< (count (:text app)) 10)
                                     (om/transact! app :text (fn [old-value]
                                                               new-value))))})

[GetContented]

Apologies. I shouldn't have commented. I clearly don't understand the problem.

[swannodette]

@jonase the patching of form elements is necessary to avoid issues that arise out of async rendering. Better solution ideas welcome.

[jonase]

@swannodette Thanks! I would like to understand this problem more deeply.

• With async rendering I suppose you mean rendering with requestAnimationFrame?
• What are the "issues that arise"? How do they manifest and how does the current implementation mitigate them?
• If these issues are insurmountable, would you consider exposing a configuration option to turn of async rendering (per root perhaps)? It might be benefitial to trade off some performance for a more functional model in some (many?) cases.

[swannodette]

No flag for disabling async rendering. Use React DOM elements directly if you want to observe the issues. In particular text inputs behave badly.

[jonase]

@swannodette After some searching I found https://github.com/facebook/react/pull/1759 where the issue with controlled components and rAF is discussed. I also found this statement from one of the React developers[1]

Let me repeat myself: rAF batching is not supported. It is not a priority for us because it does not solve any real problems that I know of and makes things much harder to reason about.

so it seems that making rAF work seamlessly with the React model is currently not a priority.

[1] https://groups.google.com/d/msg/reactjs/LkTihnf6Ey8/FgFvvf33GckJ

[jonase]

I played around with this some more and I found that even with the current implementation, the following

(def app-state (atom {:text "foo"}))

(defn my-input [app]
  (reify
    om/IRender
    (render [this]
      (dom/input
        #js {:type "text"
             :value (.toUpperCase (:text app))
             :onChange #(om/transact! app
                                      :text
                                      (constantly (-> % .-target .-value)))}))))

(om/root
  my-input
  app-state
  {:target (. js/document (getElementById "app"))})

will cause the cursor to jump to the last position and this seems to be one of the symptoms of async rendering. Note that if I replace (.toUpperCase (:text app)) with (:text app) the cursor stays in the correct position.

Even more interesting is the fact that, even thou the implementation is completely different, Reagent behaves in exactly the same way as Om in this case:

(defn my-input [on-change text]
  [:div [:input {:type "text"
                 :value (.toUpperCase text)
                 :on-change #(on-change (-> % .-target .-value))}]])

(def text (reagent/atom "foo"))

(defn main-page []
  [my-input #(reset! text %) @text])

Here also, the cursor will jump to the last position and if I replace (.toUpperCase text) with text the cursor stays put.

[swannodette]

Sorry I've been a bit busy with cljs.test and test.check work. In order to get controlled input behavior you must:

• Use set-state! to set component local state
• The value of the controlled input must come from component local state

That is, the contract of onChange invocation is that it must update component local state, and force a re-render. That's the only way to get controlled behavior.

This is implicit in the tutorial but perhaps could be better documented as a hard requirement until a better solution is provided for.

[swannodette]

This is not something that needs fixing.