jquense / react-formal

Sophisticated HTML form management for React
http://jquense.github.io/react-formal
MIT License
527 stars 52 forks source link

Empty non required field error #127

Closed sshmyg closed 7 years ago

sshmyg commented 7 years ago

Hi, this issue posted here https://github.com/jquense/yup/issues/66#issuecomment-313032152, but it's Form bug. Empty non required field pass to yup validator empty string, but should pass undefined.

jquense commented 7 years ago

Hi there. This is the job of your form Inputs to return correct values for 'empty' inputs. Native inputs can only have string values which is why they are empty strings vs undefined/null. The default ones included with react-formal do this already. The reason tho this has to be down at the input level is that that is the only place that knows if the empty string is intentional or a non-believers, we can assume that all empty strings would be undefined

sshmyg commented 7 years ago

"The default ones included with react-formal do this already" - no, it's not.

http://jquense.github.io/react-formal/#/getting-started?_k=vcpbfo First form with Date of Birth. This isn't required field. But when we input something and than clear field, we'll get an error about Invalid date and can't submit the form.

jquense commented 7 years ago

That is a bug, but not this one. the problem there is the schema is wrong, it should be nullable() and hasn't been updated. The value returned when you clear it is not an empty string it's null

sshmyg commented 7 years ago

I've tried with nullable(true). And nothing.

jquense commented 7 years ago

change the schema in the example to

dateOfBirth: yup.date()
  .nullable()
  .max(new Date(), "You can't be born in the future!"),

and it works when you clear a date

sshmyg commented 7 years ago

Just fixed an example, http://jquense.github.io/react-formal/#/getting-started?_k=vcpbfo here, that non require fields works as expect.

http://prntscr.com/g1g6mr - let parent = getter(parentPath, value) || {} always as empty string.

jquense commented 7 years ago

can you please post a reproduction of the error in a codesandbox or fiddle, that will be easier to work with

sshmyg commented 7 years ago

https://codesandbox.io/s/Z40AJRjVv Input something, and than clear field.

jquense commented 7 years ago

Is there a reason you are using the text type for the input instead of the number type? In the general the errors are the best as indicating what is wrong, but what's happening is that your string is empty or otherwise is trying to be parsed to a number, since that fails the value becomes NaN, the confusing thing is that when turned into a string to display NaN becomes '' because javascript.

Overall the errors should be a bit more descriptive of the underlying problem, but ultimately if you want a field to be clearable you need to make the schema, nullable so that null is valid. or have your input return undefined on change.

sshmyg commented 7 years ago

No it's not a problem that example contains text type instead of int. Problem is this field NOT require, but if I type something there and after change my mind - clear this field. I'll get an error and can't send form. But this field NON require and I should have ability to send form with empty the field. On form validation yup validator get empty string but should get undefined instead. I fixed it in pull request.

sshmyg commented 7 years ago

Change please example scheme, I could input and than clear field without error.

jquense commented 7 years ago

Yes i've already explained what is wrong with your setup. An empty string is "" is NOT an empty value to yup, it is a string value. If you want to clear an input the input needs to return undefined, OR null with a nullable() schema

https://codesandbox.io/s/Z2zrLK48

The error you are getting has nothing to do with whether the field is required or not, its throwing a TypeError because the input is returning the wrong type. If you use a text type input the input returns the text value, if it can't parse that to a number the value becomes NaN and yup correctly tells you that the value must be a number. same as when you clear the text input and it returns an empty string. A string is not a number so yup complains that the value is the wrong type.

I've noted abovce that the errors are not very descriptive of the underlying problem, we need to improve them.

sshmyg commented 7 years ago

Ok. Thnks. Probably my mistake