Fix text input values, and form reset

[reebalazs]

This is a replacement and continuation of #100. It aims at the following fixes:

1. Instead of

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

   we have to use

   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. (Currently it's the wrong way around.)

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.

3. The controlledValue I just introduced is just a helper, it does not do anything other than it makes the code shorter.

4. Meanwhile, material-ui 0.15 has been released, so I had to bump the peer dependencies.

5. Now the additional fix in this PR: The form.reset() needs to be covered too.

   The previous fix b559bd8 already meant to fix this. But it does not cover the use cases when defaultValue or value are changing. This means that it was incompatible with my previous PR Fix text input values #100, which is the reason why I created this new one.

   My solution works by detecting if the value becomes from non-pristine to pristine, which happens when a reset is issued. So this covers the reset usecase as well, together with the defaultValue and value cases, as a consequence it's working well in both the controlled and the uncontrolled use cases, and behaves well with reset as well.

I'm testing this for weeks in a project, and it works well with my use case. Sadly, I had no time to bring this fix upstream until now. However seeing that meanwhile, the recent fix targets to fix reset() in a different way, I'd like to ask everyone involved to test my fix for their use case, making sure that it covers the reset() use case as well.

[mbrookes]

Might just be simpler to do:

return { value: this.controlledValue() };

[mbrookes]

@reebalazs Thanks for resubmitting. A couple of comments, purely code-style difference of opinion. 😄

[mbrookes]

@reebalazs Any update?

[reebalazs]

Hi @mbrookes,

I've updated the code according to your suggestions, I agree that it's better this way.

I've also updated componentWillReceiveProps to make it work more the "expected" way. This is:

• if props.defaultValue changes, it should only update the input, if there are no manual changes
• while, changing props.value will update the input regardless if there are any manual changes.

I also fixed the error in the tests.

However, I noticed another problem, this time with Formsy.Form. It simply uses our props.value as the pristine value of the field, in a hardcoded way. So it does not take our defaultValue into consideration at all. As a consequence, if someone uses defaultValue only, then the second "isChanged" parameter passed to the form's onChange handler, will give a bad result, it will detect the form changed even though we are just on the default value.

So right now this is only correct if we use "value". To make the pristine values working correctly for the case of "defaultValue" in here, I had to modify getPristineValues:

function getPristineValues() {
      return this.inputs.reduce((data, component) => {
        var name = component.props.name;
        // try both defaultValue, and value.
        data[name] = component.props.defaultValue || component.props.value;
        return data;
      }, {});

... But actually the correct solution would be if it used state._pristineValue, which is meant for that purpose, but not used in the end. Then we could update the pristine value on our state together with the value updates, and get done with the problem.

The other solution that has to be considered, is to remove support for defaultValues at all, although despite its usefulness and the fact that mui's TextField supports it, one could live without it, and after all the other fields don't have it either, they just have a value prop. Then it would fall back to working just like, say, FormsySelect works. I am considering this despite the ton of testing I've put into this already. Besides that it would not require the fix on Formsy.Form, it would offer a clearer model, making it easier to explain how it works,

To play around, I have created a demo app on a separate branch. It can be used to manually test what happens if defaultValue, value props are changed on the fly, and how this interacts with the user's manual changes, and form reset. There is also a button to totally drop and recreate the form. This demo does contain the necessary fix in Formsy.Form already (or, a version of it.)

I also believe that either solution is followed in the end, we need to add more tests to actually define how the component has to behave in these cases. I will start to add some soon. This is inevitable, as the interaction with Formsy makes things complicated.

@mbrookes what is your opinion about keeping vs. dropping defaultValue?

[mbrookes]

@reebalazs Thanks for the update, and taking the time to write up your findings and provide a demo app.

changing props.value will update the input regardless if there are any manual changes

Playing with the demo app, this only seems to be true if props.value has updated since the form has had manual changes. Any existing value is ignored. Maybe that isn't an issue?

Regarding defaultValue, that was only added recently. Previously you had to provide a default using value, which seemed to work fine even though not as convenient.

defaultValue was mostly added for consistency with Material-U, as there was some confusion because of its absence. If removing it again is simplest, I'm not totally against that - development on Formsy is slow at best, so getting a change there would probably be difficult.Not to mention defaultValue is specific to Material-UI, while Formsy is more general.

A final option would be to mokey-patch Formsy.Form, or provide a replacement, FormsyForm that extends it as you have done in Main.js.

[reebalazs]

I added tests two weeks ago. I wanted to work on the formsy patch last weekend, but I had to time, so that's still to be done.

If we forget about supporting defaultValue, then the PR is ready (even, the code can be simplified by removing defaultValue). If we continue to support defaultValue, which would be still my preference, then the change in Formsy will be necessary.

A monkey patch is fine from an application, but imo, not a good solution for a library. We can provide a temporary hook that people can call to activate the patch, but the clean solution would be the change in formsy. The required change is very simple, but we need a clear reasoning for why we need it. (How some components, like FormsyText in our case, have a pristine value that is not simply props.value.)

[sgabler]

Hey, thanks for your work on this @reebalazs. I'd be in favour of merging this new functionality now and creating a follow-up pull-request for the defaultValue related changes. What do you guys think?

[sgabler]

@reebalazs @mbrookes Any update on how to approach this pull-request? I am still in favour of creating a follow-up ticket, but obviously I am not as involved as you guys 😉

[reebalazs]

I'd go with merging and creating a follow up ticket. I try to do the followings during the upcoming weekend:

1. Write my suggested changes and submit it as PR to Formsy.
2. Write a local handler that can be called to monkeypatch Formsy, until the PR is accepted. - This is only necessary to fix the pristine values for the use case when someone wants to use defaultValue.
3. Write a followup ticket here, that describes how to move on with this issue.

I hope to finish this till July 5th.

[mbrookes]

@reebalazs Yes, sorry for the delay. Please squash this down a bit, and I'll merge.

[reebalazs]

Squashed. The followup ticket and modifications are coming up soon.

[mbrookes]

@reebalazs Thanks!