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

Fixes Number.is* signatures to accept any input. #24436

Merged
merged 1 commit into from
May 30, 2018

Conversation

MicahZoltu
Copy link
Contributor

I skipped straight to a PR rather than opening an issue first because this is such a small change and a PR illustrates the problem well IMO. I can open a separate issue if that is desirable.

These functions are incredibly useful for testing to see if a value is a number that meets certain constraints as they return false for any input that doesn't satisfy the constraints explicitly. Tested in NodeJS and Firefox and both of them work properly when you give a range of values. MDN also indicates that they will return false for any non-number input.

These functions are incredibly useful for testing to see if a value is a number that meets certain constraints as they return false for _any_ input that doesn't satisfy the constraints explicitly.  Tested in NodeJS and Firefox and both of them work properly when you give a range of values.  MDN also indicates that they will return false for any non-number input.
@msftclas
Copy link

msftclas commented May 27, 2018

CLA assistant check
All CLA requirements met.

@j-oliveras
Copy link
Contributor

Already declined, see #4002.

@MicahZoltu
Copy link
Contributor Author

TL;DR: The arguments made in that PR do not apply here, and this is not a duplicate.

@j-oliveras That is for isNaN and isFinite (global methods). This PR is for Number.isNaN and Number.isFinite, which have very well defined behavior for what happens when non-numbers are passed in and, in alignment with the arguments made in #4002, does no implicit conversion. Number.isFinite('4') === false and isFinite('4') === true.

Because of the lack of implicit conversion, there are very meaningful and useful use cases for calling Number.isFinite with a non-number, such as being able to do if (Number.isFinite(unvalidatedUserInput)) instead of if (typeof unvalidatedUserInput === 'number' && Number.isFinite(unvalidatedUserInput))

@j-oliveras
Copy link
Contributor

Sorry, you are right.

@mhegazy mhegazy requested a review from RyanCavanaugh May 30, 2018 00:10
@mhegazy
Copy link
Contributor

mhegazy commented May 30, 2018

@RyanCavanaugh thoughts?

@RyanCavanaugh
Copy link
Member

I think it's compelling to allow e.g. string | number inputs to these functions since we can be sure no weird coercion is taking place.

That said, I'm not a huge fan that you can write e.g. Number.isFinite("oops") as it will always be false - this call would be extremely suspect and we should flag it as such.

Without the ability to declare contravariant parameters or use the assignability relationship for some parameter positions, though, I think the first point outweighs the second.

@kpdonn
Copy link
Contributor

kpdonn commented May 30, 2018

@RyanCavanaugh Already doable:

declare function isFinite<T extends (number extends T ? any : number)>(
    value: T,
): boolean;

declare const maybeNumber: number | string;
declare const definitelyNumber: number;
declare const definitelyNotNumber: string;

isFinite("oops") // Argument of type '"oops"' is not assignable to parameter of type 'number'
isFinite(12345) // allowed

isFinite(maybeNumber) // allowed
isFinite(definitelyNumber) // allowed
isFinite(definitelyNotNumber) // Argument of type 'string' is not assignable to parameter of type 'number'

Playground link

Credit to @jcalz for coming up with the pattern in an old issue #22375 (comment). I have it bookmarked because it's so useful.

@MicahZoltu
Copy link
Contributor Author

Do I understand correctly that what you want is for Number.isFinite(input) to compiler error if the type of input is already constrained to not a number, but you want it to compile successfully if the parameter might be a number as in the following:

function a(x: number|string, y: string, z: any) {
    Number.isFinite(x) // no compiler error
    Number.isFinite(y) // compiler error
    Number.isFinite(z) // no compiler error
}

@MicahZoltu
Copy link
Contributor Author

Also, would it be appropriate for me to change the signatures in this PR to have the return value as value is number? I learned about that after submitting this PR, and it would significantly improve the quality of my code since the static analyzer would be able to make assertions about the variable after the call if I branched based on it.

function a(input: any) {
    if (!Number.isFinite(input)) throw new Error(`${input} must be a finite number.`)
    // `input` is now of type `number` at this point in the code according to the compiler

I can also submit that as a separate PR if that is preferable.

@mhegazy mhegazy merged commit 593b048 into microsoft:master May 30, 2018
@ajafff
Copy link
Contributor

ajafff commented Jun 1, 2018

@MicahZoltu unfortunately making these functions act as typeguard (value is number) doesn't work. Because TypeScript will strip number from the type in the else branch. That's not correct, because you might still deal with a number. See this snippet: https://agentcooper.github.io/typescript-play/#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXywGcBJVDEAcxBgAoAHWKAWwC54pUBPASjYZmYFC8VMiYAjagG4AUDNCRYCCCAzwAbm1ETq8AD7xCGGFlQVZMrIng0ipclVrru3eAG8Z8LxoB0GHABiWAAeIMA03LIAvvAgEIQIHt6+YAAWsACCGDQADJHwAPQF8BipRPAA7lgQECWpMDgV7GowaNhMCFbwAOTq3ULsSBA4UNhm8HQ4pmrakjAyUUA

IIRC there was an issue to add typeguards that only narrow the then-branch, but I can't find it right now.

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
@MicahZoltu MicahZoltu deleted the patch-1 branch November 2, 2018 03:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants