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

Unclear error message when checking NaN against the number type #231

Open
ghost opened this issue Jan 16, 2022 · 7 comments
Open

Unclear error message when checking NaN against the number type #231

ghost opened this issue Jan 16, 2022 · 7 comments

Comments

@ghost
Copy link

ghost commented Jan 16, 2022

When testing NaN against ow.number, the following error message is emitted:

ArgumentError: Expected argument to be of type `number` but received type `number`
at ow (node_modules/ow/dist/index.js:29:28) 
{
  validationErrors: Map(1) {
    'number' => Set(1) {
      'Expected argument to be of type `number` but received type `number`'
    }
  }
}

This behaviour can be verified by running this two line snippet in the Node REPL:

const { default: ow } = require('ow')
ow(NaN, ow.number)

I think that this error message would be quite confusing for those who encounter it. I see two possible ways to solve this problem:

  • Change the error message in the NaN case to be something along the lines of: Expected argument to be of type `number`, but received `NaN`.
  • Add specific ow.number.nan and ow.number.not.nan checks.
@sindresorhus
Copy link
Owner

I agree. That should be improved. I think we should do both suggestions.

@panva
Copy link
Contributor

panva commented Feb 21, 2022

@sindresorhus would you prefer the first suggestion to be a breaking change in @sindresorhus/is or a special case added to ow?

@sindresorhus
Copy link
Owner

would you prefer the first suggestion to be a breaking change in @sindresorhus/is or a special case added to ow?

How would you fix it in @sindresorhus/is?

@panva
Copy link
Contributor

panva commented Feb 21, 2022

@sindresorhus change https://github.com/sindresorhus/is/blob/v4.4.0/source/index.ts#L112-L113

to

		case 'number':
			return Number.isNaN(value) ? 'NaN' : 'number';

@sindresorhus
Copy link
Owner

sindresorhus commented Feb 21, 2022

Ah yes. Let's fix it in @sindresorhus/is, but it might take a while before we can do a major version there, so better to fix it in both places, actually.

@panva
Copy link
Contributor

panva commented Feb 21, 2022

Ok, ignore #232 then.

@sindresorhus
Copy link
Owner

See comment update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants