jquense / yup

Dead simple Object schema validation
MIT License
22.86k stars 932 forks source link

Number doesn't allow NaN value #2242

Closed thany closed 1 month ago

thany commented 2 months ago

Describe the bug To make number input appear empty initially, I have to make the default value NaN. An empty string is quite common, but it doesn't satisfy the typings, so it can't be used. I also can't use null or undefined because that will make a field uncontrolled and switch to being controlled once filled in, which is not allowed.

To Reproduce

https://codesandbox.io/p/sandbox/yup-test-case-gg1g1

Expected behavior The value NaN, despite what it means, is absolutely a valid value for the type number. Yup makes it forcibly invalid, using this line: https://github.com/jquense/yup/blob/5a22c16dbba610050e85f123d389ddacaa92a0ad/src/number.ts#L40

This will produce a typeError on number fields having NaN as value. There is no other value I can put, to make a field appear empty and also satisfy typings.

I could of course catch the typeError error and make it show a "field is required" message, but this will break for number fields that are optionally required, where the field, and its error message, become invisible to the user. In such case, the field will keep its default value (NaN), be invisible, and assume that it always validates.

Long story short: NaN should be allowed for the number type. Leave it to schema implementation to explicitly disallow the NaN value (perhaps include it in nonNullable() or required()), and don't hardcode it into Yup to make it invalid.

Platform (please complete the following information):

thany commented 2 months ago

Btw, I tried overriding the check method using this documentation but it seems overriding such built-in function isn't allowed. Also typescript complains about it and the function is never called, so that won't be a viable workaround.

I'll keep looking for a workaround. Hopefully in the mean time, this bug can be fixed for real.

thany commented 2 months ago

Viable workaround, meaning not a proper solution:

  foo: number().default(NaN).when([], {
    is: () => someCondition,
    then: schema => schema.nonNullable().integer().min(1).max(999999).required(),
    otherwise: schema => schema.transform(v => (Number.isNaN(v) ? null : v)).nullable(),
  }),

This will allow NaN values in the otherwise part of the validation. Again, not great that I have to transform them to null, because now I can't protect against actual null values, potentially, with a different error.

jquense commented 1 month ago

I'm not going to allow NaN, it's nearly always indicative of an error, in the same way InvalidDate is actually a Date but isn't a helpful value. If you want an empty value convert '' to null or undefined

thany commented 1 month ago

I can't convert it to a non-number, because the underlying interface requires a number type. The schema has to match the interface, that's how you set up the typings. If I have to allow null or undefined, it would become an allowable value, which it shouldn't be.

NaN is indeed indicative of an error, which is precisely why it should be possible to validate against it!