AppliedMathematicsANU / plexus-form

A dynamic form component for react using JSON-Schema.
MIT License
134 stars 31 forks source link

Empty values are lost #31

Open martijnvermaat opened 9 years ago

martijnvermaat commented 9 years ago

Due to calling plexus-objective.prune, fields with empty input fields are not returned in the output.

I think this is quite inconvenient when you are updating existing values. Once something is set, the user cannot remove it anymore. I'm not sure if it would be better to return empty strings or null/undefined, but I guess for most use cases any would do.

Another strange behaviour originating from this is I think a quite typical controlled form setup. Say you provide a values prop which is updated on submit from the form output (and submitOnChange is set). Now if there is only one (string) input field, this works until you try to delete the last remaining character in it. What (I think) happens is that the DOM value becomes the empty string, which is pruned, leading to null for the entire form output (also by pruning), which in turn makes that the values prop is ignored and the Form component state holds on to the deleted charachter which is then also rendered again.

This is also related to #7

martijnvermaat commented 9 years ago

As for the first issue, I imagine it would often be possible to completely replace all existing values instead of trying to merge them with the output from plexus-form. It's probably harder to change this behaviour from plexus-form.

As for the second issue, here's a simple test setup: https://gist.github.com/martijnvermaat/292ecd6e912a3edd89c1

npm install
npm run dev

Enter some text in the form and try to remove it again. The last character cannot be removed. I think this is due to this line in prune.

Please let me know if I'm wrong, but I think this is quite a typical setup for a controlled form. Whenever you provide the values props they should probably be updated via submitOnChange.

odf commented 9 years ago

Thanks for reminding me of this issue! I thought I had partially fixed this at some point, but maybe I was just thinking about the fix.

Prune is so aggressive in order to make auto-shrinking of arrays with complex entries work. A post-processing step on the result of the prune call might be a good idea. Maybe this can be relatively simple - just make sure the value has a non-null top-level object with all its properties as given in the schema present (which gets slightly non-trivial when we have a oneOf there, but doable).

It would be incorrect to return an empty string when a numerical field is empty, as would be returning 0, so in those cases I think null is the best value (I think it is usually a bad idea to explicitly set a value to undefined, but maybe that's just me). I'm inclined to do the same for string fields for consistency, but if anyone has a good reason for using empty strings, I can be convinced.

martijnvermaat commented 9 years ago

I think I briefly played around with returning null, but validation failed when passing those back into the form (e.g., a value of type string was expected).

I was thinking a bit more about how to work with the values prop. My reasoning for using it in combination with submitOnChange to update this prop on every change was that otherwise every render would destroy changes by the user (much like the React docs describe working with controlled components, as opposed to using defaultValue). And renders could be triggered at any time, unrelated to the form component.

However, you could also prevent a render using shouldComponentUpdate (return false unless you want the form to be reset to the original values prop). This would save you from having to keep track of the form values and using submitOnChange.

Do you have any thoughts on what is the most obvious approach with using the values prop?