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

New private function Util._checkNumber instead of checking over isNaN #7768

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johnd0e
Copy link
Collaborator

@johnd0e johnd0e commented Nov 11, 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/

  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).

Ref: #7761, #7128.


Questions:

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/

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).
Copy link
Member

@IvanSanchez IvanSanchez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@IvanSanchez
Copy link
Member

Should we have common method instead of copy-paste (LatLng: stricter arguments validation #7765, Point: arguments validation #7767, this, and potentially a lot of other cases)?

IMO, yes. But if it's going to be a common method/function, I'd rather name it without the leading underscore, and put some docstrings.

Comment on lines +247 to +249
// keep it private intentionally, because such function implementation
// heavily depends on it's particular application and unable to suit all the cases
export function _checkNumber(a) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// keep it private intentionally, because such function implementation
// heavily depends on it's particular application and unable to suit all the cases
export function _checkNumber(a) {
// @function checkNumber(data: Number|String): Number
// Makes sure that passed `data` is finite number, and returns it.
// If string is passed - converts it to number trying to avoid coercion pitfalls.
// Throws error in case of any failure.
export function checkNumber(a) {

@IvanSanchez
But I'm not sure, because fixing it in public API

  • .. makes impossible future internal changes. E.g. what if later we'll decide to throw on strings like '1e9'
  • .. opens the door to misusing (remember using Browser.touch)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanSanchez what do you think about that points? I personally would like to have a public function so it can be easy used in plugins. Not everyone should reinvent the wheel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mourner what do you think is better? A public or private function for checking if the input is a number.

@Falke-Design Falke-Design added this to the 1.8 milestone Nov 25, 2021
@Falke-Design Falke-Design modified the milestones: 1.8.0, 1.8.1 Dec 17, 2021
@Falke-Design Falke-Design moved this from To do to In progress in Validation & Checks Apr 15, 2022
@Falke-Design
Copy link
Member

The open questions is should we make the checkNumber function public or private? I'm for public.

The concerns against public was:

.. makes impossible future internal changes. E.g. what if later we'll decide to throw on strings like '1e9'
.. opens the door to misusing (remember using Browser.touch)

@mourner @IvanSanchez how shall we proceed? It would be good if we decide that. Around 5 PRs and 8 issues are waiting on that decision.

@jonkoops
Copy link
Collaborator

I'd argue to make it private, if it creates added value for plugins devs we can always make it public in the future. Let it prove itself first.

@Falke-Design
Copy link
Member

@mourner what do you think?

@Falke-Design Falke-Design removed this from the 1.9.0 milestone May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Validation & Checks
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants