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

[FormsyText] Warning: Input elements must be either controlled or uncontrolled #82

Closed
dmhood opened this issue Apr 19, 2016 · 18 comments
Closed
Labels

Comments

@dmhood
Copy link

dmhood commented Apr 19, 2016

Currently the defaultValue prop of the underlying TexField component is set using the "value" prop (https://github.com/mbrookes/formsy-material-ui/blob/master/src/FormsyText.jsx#L34). Because we pass all these props down to textfield (https://github.com/mbrookes/formsy-material-ui/blob/master/src/FormsyText.jsx#L32) this unfortunately makes the field un-editable. Suggest just removing https://github.com/mbrookes/formsy-material-ui/blob/master/src/FormsyText.jsx#L34 altogether.

@mosanger
Copy link

This also produces the following warnings in Chrome, when I try to set the value property, instead of defaultValue:

Warning: Input elements must be either controlled or uncontrolled (specify either the value prop, or the defaultValue prop, but not both). Decide between using a controlled or uncontrolled input element and remove one of these props. More info: https://fb.me/react-controlled-components

Warning: TextField is changing a controlled input of type text to be uncontrolled. Input elements should not switch from controlled to uncontrolled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components

Setting value instead of defaultValue at least makes the field usable, but doesn't set its value initially, which means its value is always empty on mount.

using React 15.0.1 and Material UI 0.15.0-beta1

@keremciu
Copy link

yep, I delete only one line on formsytext

defaultValue: this.props.value

and it works normally now, I can use defaultValue property

@quangdng
Copy link

@keremciu Can you make a pull request for that? It would be great.

@reebalazs
Copy link
Contributor

reebalazs commented Apr 21, 2016

Hi there,

I've created #87 which is a related issue, closing now because the two issues interrelate.

In my original problem I passed in 'value' which I wanted to get passed down as defaultValue to TextField. This makes the field uneditable. I attempted to fix this in PR #86, which makes the field editable again. - But it also breaks the possibility to pass in a value directly. In my use, I don't do that, but reading this issue, I realize it can be a legitimate need.

  1. solution would be to change the api and require to pass in defaultValue if you really want to pass in defaultValue, and value if you want value. This would be, imo, a breaking change if previously the code passed in value.
  2. possibility is to apply my fix and then you loose the ability to pass in value directly, but you can continue to pass in value which would result in passing the defaultValue to TextField really

However if everything is left as it is currently, it is not possible to set a defaultValue, and an intention to do so will actually set value instead of defaultValue, resulting in an uneditable form field. So a change is needed in any case, because passing in an initial value (transformed into defaultValue) is a legitime use case. It broke my codebase, for example, which was the main reason for me to figure out a solution.

I beleive this line in the README:
"Note: for FormsyText you must use value instead of defaultValue to set a default value."
summarizes the problem really. Now I am inclined to solution 1., as it seems to be more universal, even on the price of breaking changes. @mbrookes, what do you think? In both cases, the fix would be simple, but I urge some action, as currently FormsyText is broken.

@reebalazs
Copy link
Contributor

Looking deeper at this, the case seems more and more complex. I am not sure if some recent change in formsy, or the react update has caused the regression that does not allow the value to be changed any more. The only way I can make this work currently, if I start managing the value myself. I have a terribly messed up version that works for my code, but not elegant and workaround-ish. I'll consider another PR once I am able to stabilize things.

@mbrookes
Copy link
Collaborator

@reebalazs - the reason the default value has to be set with value is that formsy-react expects the default value on that prop.

Things may have been broken by the removal in 0.4.0 of a chunk of code to improve performance, (effectively a copy #72 applied manually).

@mbrookes
Copy link
Collaborator

Actually, it's likely this was always an issue, but React didn't complain before 15.0.

Could you try the current master?

@mbrookes mbrookes changed the title Using TextField's defaultValue makes it unuseable [FormsyText] Warning: Input elements must be either controlled or uncontrolled Apr 25, 2016
@mbrookes mbrookes added the bug label Apr 25, 2016
@dmhood
Copy link
Author

dmhood commented Apr 25, 2016

Before https://github.com/mbrookes/formsy-material-ui/pull/72/files the element was manually applying the newly input values to the underlying text element...now that it is removed you cannot use value/defaultValue at all without rending the textfield unchangeable.

The warning about controlled/uncontrolled input elements is a separate (related) issue.

Shouldn't cf19f38 be rolled back until a working solution can be found?

@mbrookes
Copy link
Collaborator

@dmhood You shouldn't be using defaultValue - please see the README.

@dmhood
Copy link
Author

dmhood commented Apr 25, 2016

@mbrookes What I mean is using the value prop to set the default value (https://github.com/mbrookes/formsy-material-ui/blob/master/src/FormsyText.jsx#L34). It "locks" the value to whatever you pass in. There is no way to set the value but still get new user input.

@mbrookes
Copy link
Collaborator

@dmhood - my bad. Missed the most important file from the commit! Try now.

@dmhood
Copy link
Author

dmhood commented Apr 25, 2016

@mbrookes works great w/ the recent commit, thanks!

edit: Leaving it open in case there is any more discussion but feel free to close.

@KatSick
Copy link

KatSick commented Apr 26, 2016

Gentlemen's, is there any workaround to use controlled inputs ?

@mbrookes
Copy link
Collaborator

@dmhood - 0.4.1 released.

@ochervak - not at the moment - controlled use was causing performance problems, but we'll have to revisit at some point, as uncontrolled use is possibly being removed from MUI in the future. Feel free to submit a PR if you can figure out a solution! 👍

@KatSick
Copy link

KatSick commented Apr 26, 2016

thanks

@dmhood
Copy link
Author

dmhood commented Apr 26, 2016

thanks!

@mbrookes
Copy link
Collaborator

I've updated FormsyText on master, so it now supports defaultValue, but also kept backwards compatibility with using value for this purpose. Please check it out and let me know if it's working as expected for you.

@reebalazs
Copy link
Contributor

reebalazs commented May 15, 2016

It took me a while but I tested the last fix out. The fix was definitely going to the right direction, very similar to my previous solution, but better. However it was still not perfect. Here is PR #100 which fixes the remaining problems. (Wohoo, PR 100!)

  1. The correct order of the value + defaultValue selection should be just the opposite of the current one:

    this.props.value || this.props.defaultValue || ''

    This allows the use of both a defaultValue and a value (controlled case), and allows the value to take precedent if it's specified. After this, both value and defaultValue props do perfectly what they are intended to.

  2. Similar to componentWillMount, there is also a need for a componentWillReceiveProps. This covers the case when the form gets reloaded with new values, and allows the defaultValue-s change on the fly, taking care of the pristine values working correctly in this case.

Some other notes are on the PR.

Please test it out and let me know what you think, I am testing this for a while in a project, and it works well.

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

No branches or pull requests

7 participants