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

Input not controlled when value and on-change specified. #148

Open
FreekPaans opened this issue Feb 2, 2017 · 30 comments
Open

Input not controlled when value and on-change specified. #148

FreekPaans opened this issue Feb 2, 2017 · 30 comments

Comments

@FreekPaans
Copy link

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

r0man commented Feb 3, 2017

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

@FreekPaans
Copy link
Author

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

@r0man
Copy link
Owner

r0man commented Feb 3, 2017

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

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

r0man commented Feb 3, 2017

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

@r0man that did the trick!

@FreekPaans
Copy link
Author

@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
Copy link
Owner

r0man commented Feb 4, 2017

@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.

See also 579f3d7 for the issue with IE and facebook/react#7027

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

r0man commented Feb 5, 2017

@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: facebook/react#7027
Not sure if this complexity is worth implementing, though.

@FreekPaans
Copy link
Author

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

DjebbZ commented May 9, 2017

Any advance on this ?

@r0man
Copy link
Owner

r0man commented May 14, 2017

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

jmlsf commented Jan 11, 2018

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:

#10

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

facebook/react#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
Copy link

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

r0man commented Mar 22, 2018

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

@gfredericks
Copy link

No, I'm using :infer-externs true

@r0man
Copy link
Owner

r0man commented Mar 22, 2018

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

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

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

DjebbZ added a commit to DjebbZ/sablono that referenced this issue Apr 10, 2018
I had to change some config in project.clj otherwise Fighweel wouldn't
run.
@DjebbZ
Copy link

DjebbZ commented Apr 10, 2018

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))}

@pedrorgirardi
Copy link

pedrorgirardi commented Aug 21, 2019

Hi @r0man, could I help in any way to get this fixed?

@r0man
Copy link
Owner

r0man commented Aug 21, 2019

@pedrorgirardi Yes, if you have a good idea how to fix this, and can test this in multiple browsers I would love to get help on this. I don't have time to look into this myself at the moment.

@pedrorgirardi
Copy link

Ah, nice! Let me try then. :)

@pedrorgirardi
Copy link

Hi @r0man , may I ask why we need the wrappers? The React issue was fixed awhile ago so I tried to not use wrappers at all.

I created this https://github.com/pedrorgirardi/sablono-issue-148 which uses my fork of Sablono without wrappers.

Maybe the wrappers are no longer needed since the React issue was fixed? What am I missing? 😃

@r0man
Copy link
Owner

r0man commented Sep 9, 2019

@pedrorgirardi If I remember correctly this was needed for libraries like om that render on requestAnimationFrame. I'm afraid I don't remember all the details. Do you have a link to the React issue that was fixed?

@pedrorgirardi
Copy link

@r0man facebook/react#7027

What you reckon?

@piranha
Copy link

piranha commented Sep 11, 2019

This seems like an unrelated bug. This thing inside sablono is definitely support for requestAnimationFrame-based rendering.

@pedrorgirardi
Copy link

pedrorgirardi commented Sep 11, 2019

@piranha it's a bit confusing because it seems there are two "issues": 1) wrap controlled input component to support requestAnimationFrame-based rendering as you said; 2) workaround an IE specific issue which seems to have been fixed here facebook/react#7027 .

@r0man
Copy link
Owner

r0man commented Sep 17, 2019

@pedrorgirardi As @piranha mentioned I think we should keep the wrapped inputs to support libs that use RAF for rendering. I'm afraid I don't have time to look into this myself.

@pedrorgirardi
Copy link

Thanks, @r0man and @piranha for the clarification.

I noticed Reagent wraps inputs so I want to have a look at how it's done and hopefully learn something which could potentially be used here.

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

7 participants