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

Allowing yup.number() to accept NaN #1330

Closed
jessie-ross opened this issue Apr 8, 2021 · 16 comments
Closed

Allowing yup.number() to accept NaN #1330

jessie-ross opened this issue Apr 8, 2021 · 16 comments

Comments

@jessie-ross
Copy link

The Problem

I can't see a way for a yup.number() schema to accept a NaN value as valid.

yup.number().nullable(true).validateSync(NaN)
// => throws ValidationError: this must be a `number` type, but the final value was: `NaN` (cast from the value `"nan"`).

Should NaN be allowed as a number?

When working with common math libraries (e.g. numpy) it is common for NaN to still be considered a number, it allows further calculations to be made on it without having to check if numbers are there:

NaN + NaN - NaN / NaN * NaN // => NaN

Typescript feels the same:

Screen Shot 2021-04-08 at 10 09 25 pm

In particular I am working on a front-end that performs calculations on numbers that are outputted from numpy. I believe it would be clean to just pass the NaNs through the calculations and would like a way to allow them into my application without needing to write a custom yup test.

Proposed Solution

I propose adding .nanable() equivalent to .nullable() for numbers:

yup.number().nanable().validateSync(NaN)
// => NaN

If you agree on this plan I could try a PR for it.

@X-SLAYER
Copy link

for me i use this solution and it's work

yup.number().transform((value) => (isNaN(value) ? undefined : value)).nullable()

@TheCrazyDev64
Copy link

@X-SLAYER thats more a hack for me. Its a basic feature and needs to be pushed

@jquense jquense closed this as completed Aug 20, 2022
@tchari
Copy link

tchari commented Mar 14, 2023

@jquense why was this closed? What is the resolution/solution for this problem? (i.e. it's still a problem for me.)

@LuisGilGB
Copy link

LuisGilGB commented Mar 15, 2023

Also finding issues with this at 1.0.2 when at 0.32 I wasn't. I'm trying to migrate and the following rule:

Yup.number().nullable().required(ERROR_MESSAGE_FOR_REQUIRED_STUFF)

was working fine and printed the expected message when the input is null, but now I end up seeing the NaN message. Yes, I might solve this issue by using a typeError to make it return the expected message, but it feels like a step back in terms of code expresiveness because the intention is to display a message about the field being required.

@7iomka
Copy link

7iomka commented Mar 23, 2023

Same issue. I have use UI NumberInput component that supports "" (undefined) value as default value, and if input is required I get this error must be a numbertype, but the final value was:NaN(cast from the value"").

@nelsonuprety1
Copy link

transform((value) => (isNaN(value) ? undefined : value)).nullable()

Thanks this worked. I tried setting the default values which didnt worked . It felt like the required() was't being triggered.

@jquense
Copy link
Owner

jquense commented Apr 12, 2023

Just to add a note of closure here. Yup intentionally doesn't allow NaN in same way that it doesn't allow null as a valid object (even tho it is) or InvalidDate as a valid date (even tho it is). Yup is not trying to match the JS definition of what is a thing, but be stricter and catch things that are generally errors. Most of the time NaN is a bug, and it's not really possible to tell if it's "intentional" or the result of trying to parse a number from "some string"

@TheUnlimited64
Copy link

@jquense the thing is a field with "" is converted to NaN. If this field isn't required, then it fails with NaN, even tho it shouldnt

@jquense
Copy link
Owner

jquense commented Apr 13, 2023

An empty string isn't a number which is why it (correctly) fails. Any failed coercion to a number results in NaN not just empty strings. Allowing NaN isn't the right way to deal with that case, transform the value to undefined or null if you want to treat empty strings as empty number values. There are past discussions you can look up for more solutions.

@TheUnlimited64
Copy link

But then the Required flag is totally useless. There should be a elegant way to allow empty fields. Transform is bad imo

@TheUnlimited64
Copy link

Also undefined isnt a number, nan is technically? Why not like the OP proposed like nanable

@LuisGilGB
Copy link

I can accept the definition of NaN stablished in the library, but that still is a technical detail. The main point I see is that the library client wants to achieve two things:

  • Enhance user experience through tailored validation and tailored feedback through useful error messages.
  • Enhance developer experience through code with high expressiveness.

In case NaN values deserve a separate treatment, the number method should allow providing an error message to display in case the input is not a number (basically whatever caught as NaN), what lacks some consistence with the rest of schema generators, or the number schema should have a method to set an error message for NaN values, what feels kind of redundant after calling number(). I can't come up with a 100% perfect solution, but both approaches fill the UX need with a better DX than the use of less expressive ways like transform or test.

@jquense
Copy link
Owner

jquense commented Apr 14, 2023

@LuisGilGB you can handle these cases with typeError if you want to show specific messages for when a value is NaN, it's passed a bunch of metadata about the cast value, the original value, etc.

lacks some consistence with the rest of schema generator

What aspect do you see as inconsistent, the other yup schema act similarly, treating parsing failures as invalid types, e.g failing to parse a date produces a similar InvalidDate instance of Date, that is in fact a valid Date.

@TheUnlimited64 solving for "I have an empty form field that is a number type" is valid thing but doing it via allowing NaN is not the correct solution, it is too broad of a fix that introduces a bunch of other inconsistencies and isn't going to be added. I'd recommend looking for the issue specifically dealing with empty strings and numbers and adding to the conversation there, or adding a custom method to your number schema to do what you want.

Yup isn't trying to solve every case out of the box, but is intentionally flexible and extensible to allow users to change it to suit their needs.

@LuisGilGB
Copy link

What aspect do you see as inconsistent, the other yup schema act similarly, treating parsing failures as invalid types, e.g failing to parse a date produces a similar InvalidDate instance of Date, that is in fact a valid Date.

I mean that making Yup.number() to accept an error message parameter would make it less consistent compared to otger similar methods.

typeError is the kind of thing I think that makes the code to look more low level than needed.

@jquense
Copy link
Owner

jquense commented Apr 14, 2023

@LuisGilGB if your original issue is that null triggers the typeError for non nullable types, then that should be fixed now: #1982 I agree that was inconsistent compared to string and others. What that PR doesn't change is input values that are not null or undefined are still going to be cast to NaN but now it should be clear that NaN represents a parsing failure not null

@LuisGilGB
Copy link

Sorry for replying this late. I got the chance to remove the transform where we were having the type error message and the custom one was displayed successfully with the most recent version, thanks!!

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

9 participants