Input not controlled when value and on-change specified.

FreekPaans commented 7 years ago

Hi, when I create a component like this

(html [:div [:input {:type "text"
                     :value "hello world"
                     :on-change (fn [e] (println e))}]])

The input becomes an uncontrolled component, while I would expect it to be a controlled one. When I create it manually:

(.createElement
  js/React "div" nil
  (.createElement js/React "input"
    #js {:type "text"
         :value "hello world too"
         :onChange (fn [e] (println e))}))

It is controlled.

What am I doing wrong?

Thanks!

r0man commented 7 years ago

@FreekPaans Nothing, I think you found a bug in sablono. :)

FreekPaans commented 7 years ago

That could also be it :) Do you need help fixing it?

r0man commented 7 years ago

I guess the problem is in wrap-form-element over here: https://github.com/r0man/sablono/blob/master/src/sablono/interpreter.cljc#L29 I don't have time to look into this at the moment, so if you want to look into this, go for it!

FreekPaans commented 7 years ago

Ok, I'm looking into it but having troubles running the tests:

lein doo node nodejs once

;; ======================================================================
;; Testing with Node:

module.js:471
    throw err;
    ^

Error: Cannot find module 'react'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at /home/freek/code/sablono/target/nodejs/out/react-dom.inc.js:16:24
    at Object.<anonymous> (/home/freek/code/sablono/target/nodejs/out/react-dom.inc.js:40:3)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
Subprocess failed

Maybe you can give me a quick pointer?

r0man commented 7 years ago

@FreekPaans Looks like the node dependencies are not installed. Run lein deps once before running the tests. Not sure why lein doo doesn't fetch them. I would have expected that.

lein deps
sablono@0.7.8-SNAPSHOT /home/roman/workspace/sablono
├─┬ react@15.3.1 
│ ├─┬ fbjs@0.8.9 
│ │ ├── core-js@1.2.7 
│ │ ├─┬ isomorphic-fetch@2.2.1 
│ │ │ ├─┬ node-fetch@1.6.3 
│ │ │ │ ├─┬ encoding@0.1.12 
│ │ │ │ │ └── iconv-lite@0.4.15 
│ │ │ │ └── is-stream@1.1.0 
│ │ │ └── whatwg-fetch@2.0.2 
│ │ ├─┬ promise@7.1.1 
│ │ │ └── asap@2.0.5 
│ │ ├── setimmediate@1.0.5 
│ │ └── ua-parser-js@0.7.12 
│ ├─┬ loose-envify@1.3.1 
│ │ └── js-tokens@3.0.1 
│ └── object-assign@4.1.1 
└── react-dom@15.3.1

FreekPaans commented 7 years ago

@r0man that did the trick!

FreekPaans commented 7 years ago

@r0man so I looked into it a little bit, but I'm not sure what's going on in wrap-form-element with :componentWillReceiveProps and :onChange. It seems :onChange hard codes :value to the event value. Also (don't know if this is related) :onChange is only set as the onChange handler after the first :componentWillReceiveProps. Don't know what the idea is here.

Will look more into it later.

r0man commented 7 years ago

@FreekPaans Yes, this is a bit tricky. I think having an on-change handler that is not updating the value of the component, and the way we work around an IE bug are in conflict here. I think most people use a controlled input and update the input value via the onChange listener.

To make it even more tricky, the whole wrap-form-element was introduced to work around some issue with the cursor position in input elements. That was so long ago, I hardly remember the details anymore.This code really needs an investigation what is still needed. I just didn't had the time to do so :)

r0man commented 7 years ago

@FreekPaans The IE bug should be fixed in the next version of React. Then we can remove the workaround code and this should behave as expected. There's also a similar workaround near the end of the comments: https://github.com/facebook/react/issues/7027 Not sure if this complexity is worth implementing, though.

FreekPaans commented 7 years ago

Looked into it a bit more, but don't really know how to move forward. The design around wrap-form-element seems pretty deliberate, but I can't really wrap my head around it :) It's not that big of a problem for me at this moment, so maybe wait for the next react version and revisit this code?

DjebbZ commented 7 years ago

Any advance on this ?

r0man commented 7 years ago

@DjebbZ No, not yet. I haven't investigated if the IE fix is already in the latest version of React, or if this will ship with version 16.

jmlsf commented 6 years ago

I was trying to track down why my refs weren't working and came across this issue. I want to share what I've learned in case it helps anybody, but take my analysis with a grain of salt because I could be totally wrong.

The wrap-form-element was initially introduced in the following commit to om with the commit message "form element wrapper so we can get the correct behavior even in the presence of requestAnimationFrame":

https://github.com/omcljs/om/blob/c1ebe9dbc750b290eea13843ba7f4836230e9180/src/om/dom.cljs

It was imported into Sablono here as a fix for a bug where the cursor moves to the end of the input on each update:

https://github.com/r0man/sablono/issues/10

The basic gist of the problem can be seen by reading this monster thread from react:

https://github.com/facebook/react/issues/955

I think this is the problem: the state of a controlled input must be updated when a user hits a key and only then. Otherwise, the component will rerender and move the cursor around and if you type fast enough you'll drop characters since the subsequent events will not reflect the previous update. I think Om batches updates with requestAnimationFrame or some other mechanism. So I think the whole point here is to bypass that mechanism with real javascript event handler. (For the record, I don't understand the cursor-resetting issue, unless the entire dom node were being replaced, though the letter-dropping issue makes sense to me.) Later on, someone added a fix for an IE11 bug related to the on-change handler.

If I'm right, I wonder if this wrapper is needed in for libraries that don't do whatever om is doing. (Like rum, for example.)

gfredericks commented 6 years ago

This smells related:

I'm having trouble with advanced compilation and have tracked it down to this code:

  (js/ReactDOM.render

   ;; this gives an error about some minified variable being undefined
   (html [:input {:type "text"
                  :value ""}])

   #_ ;; this works fine
   (js/React.createElement
    "input"
    #js {:type "text"
         :value ""})

   (.getElementById js/document "app"))

r0man commented 6 years ago

@gfredericks I can't reproduce this. Your example works for me. Do you have the proper externs for React in place?

gfredericks commented 6 years ago

No, I'm using :infer-externs true

r0man commented 6 years ago

@gfredericks Then this might be the problem? sablono itself doesn't have any annotations that could be picked up by :infer-externs. Did you try :pseudo-names true to find out which variable/fn is undefined?

gfredericks commented 6 years ago

I didn't realize I needed to provide the externs; I'll look into that.

I also didn't know about :pseudo-names true, so thanks for pointing that out. In this case it doesn't seem too helpful, as it only refers to $fn$$.

gfredericks commented 6 years ago

Adding the externs file seems to fix it. Thanks again!

DjebbZ commented 6 years ago

My current workaround is to re-render the component when componentDidMount. In rum (what I use) it's like this :

{:did-mount (fn [state]
              (let [comp (:rum/react-component state)]
                (rum/request-render comp)
                state))}