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

Empty values are lost #31

Open
martijnvermaat opened this issue May 5, 2015 · 3 comments
Open

Empty values are lost #31

martijnvermaat opened this issue May 5, 2015 · 3 comments

Comments

@martijnvermaat
Copy link
Contributor

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

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

odf commented May 5, 2015

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

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?

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

2 participants