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

isNaN usage not covered by tests #7761

Open
johnd0e opened this issue Nov 9, 2021 · 4 comments
Open

isNaN usage not covered by tests #7761

johnd0e opened this issue Nov 9, 2021 · 4 comments

Comments

@johnd0e
Copy link
Collaborator

johnd0e commented Nov 9, 2021

There are some (~5) occurrences of isNaN usage in code, and no single test to understand their real purpose.
And that function is known to be not so obvious: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/isNaN
It's not straightforward to make sure that value is number, because boolean and empty string also coercing to numbers.

So I am not sure that our current usage is correct.
And more - instead of coercing numeric strings - I'd prefer throwing errors

Thus, we need to refactor that parts and cover them by tests.

@johnd0e
Copy link
Collaborator Author

johnd0e commented Nov 9, 2021

Ref: #7128 (comment)

@johnd0e
Copy link
Collaborator Author

johnd0e commented Nov 10, 2021

  1. isNaN itself is not reliable way to check if value can be converted to number. There a lot of discussions, and I just point to these unit tests: https://run.plnkr.co/plunks/93FPpacuIcXqqKMecLdk/
    So I propose to implement isNumeric like here: LatLng: stricter arguments validation #7765
  2. Just making sure that the value is convertible to number is not enough. Instead we should actually convert the value to number as early as possible (otherwise '5'+5 = '55').
    There are some places in code vulnerable to this sort of bugs, and a number of open issues (too lazy to link them right now).

@itsyourOOP
Copy link

@johnd0e

Is the issue still open? Because if it is, I'd like to try if that's fine.

@Falke-Design
Copy link
Member

@itsyourOOP I think this not a good issue to start, it is currently not clearly how we want to further with the number check in Leaflet.

A easy issue to start would be: #7787

@Falke-Design Falke-Design removed the good first issue Good for newcomers label Apr 15, 2022
@Falke-Design Falke-Design added this to To do in Validation & Checks via automation May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants