Skip to content

Do you guys support signed string hex for jQuery.isNumeric #2781

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

Closed
stevemao opened this issue Dec 16, 2015 · 15 comments
Closed

Do you guys support signed string hex for jQuery.isNumeric #2781

stevemao opened this issue Dec 16, 2015 · 15 comments

Comments

@stevemao
Copy link
Contributor

$.isNumeric(-0xFF)
true
$.isNumeric('-0xFF')
false

Is it the right behaviour?

@dmethvin
Copy link
Member

In the first case, the JS parser converts the hex before execution. In the second, you're expecting the string to be converted at the point of execution. Hex constants aren't supported by parseFloat or implicit string-to-number conversion. So I would say this is the right behavior. If you want to convert or validate hex/binary/octal strings write your own validator.

Edit: parseInt does suport hex, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt

@stevemao stevemao changed the title Do you guys support minus string hex for jQuery.isNumeric Do you guys support signed string hex for jQuery.isNumeric Dec 17, 2015
@stevemao
Copy link
Contributor Author

Thanks @dmethvin , looks like this needs parseInt, parseFloat, isFinite and isNaN combined.

@dmethvin
Copy link
Member

I think this can be fixed in docs. Negative hex constants aren't a really common use case.

@stevemao
Copy link
Contributor Author

Just note: parseInt supprts hex but not binary or octal :(

Also I think ( obj - parseFloat( obj ) + 1 ) >= 0; isn't semantically correct and it's a hack of not using isFinite etc...

I think it could be isNaN ( obj - parseFloat( obj )) because it's really not comparing with 0.

its harder to understand it now as the useful comments are removed in #2663.

@gibson042
Copy link
Member

With respect to the initial question, $.isNumeric('-0xFF') should always be false because +"-0xFF" is NaN. So I think this ticket should be closed, and the isNumeric code (including comments removed by 15ac848) considered separately.

@stevemao
Copy link
Contributor Author

@gibson042 I'm fine if '-0xFF' is not supported but ' +"-0xFF" is NaN' should not be reason. Its just one API that does not recognize signed string hex. as @dmethvin mentioned parseInt does suport hex.

@gibson042
Copy link
Member

The purpose of jQuery.isNumeric is (was?) to indicate that a numeric-cast operation on a value yields meaningful (read: finite) results—parseInt and parseFloat are irrelevant, except for possible implementation utility. Numeric casting of "-0xFF" is not meaningful, therefore jQuery.isNumeric("-0xFF") should return false. Cf. http://code.jquery.com/jquery-1.11.0.js .

Personally, I would be comfortable with an implementation like isFinite( obj - parseFloat( obj ) ), suitably guarded for Symbols¹ or—per https://bugs.jquery.com/ticket/14179 and gh-2662 😞—non-primitives².


¹ e.g., if ( typeof obj !== "number" ) { obj = String( obj ); }
² e.g., var type = jQuery.type( obj ); return ( type === "number" || type === "string" ) && …

@mgol
Copy link
Member

mgol commented Dec 18, 2015

I've looked at the docs and it seems very vague and even incorrect to me. The short description states:

Determines whether its argument is a number

which is simply not true as that would imply the argument has to be of a number type. Later in the text it states:

The $.isNumeric() method checks whether its argument represents a numeric value.

without explaining what "a numeric value" is. Therefore, I can see why no one really knows what this method tries to do. We should determine what's the goal of this function and document it properly, then we can fix the edge cases.

@dmethvin
Copy link
Member

I agree with that @mzgol, it's really something like, "Determines whether the argument (typically a String) can be converted to a valid Number. If the argument is already a Number, returns true unless it is one of the exceptional values like NaN or Infinity (see below)."

Instead of trying to chase edge cases we can just add some clarifying examples to the docs page, these were taken right out of the unit tests. If someone needs input validation beyond that they should implement it themselves.

This is another case where we waded into a huge amount of 💩 for a seemingly trivial utility function that has nothing to do with the DOM!

@timmywil
Copy link
Member

Keep in mind we've already added a note to isNumeric in 3.0 docs. jquery/api.jquery.com@4e65b6d

@mgol
Copy link
Member

mgol commented Dec 18, 2015

@timmywil Good point but my remarks still stand. A note is just a note, kind of additional info whereas I think the main portion of the page almost doesn't contain any concrete information...

@gibson042
Copy link
Member

Since we no longer have any internal uses of jQuery.isNumeric, it can be whatever you want in 3.0—including removed 😈. But historically, its purpose was exactly as I described above: a numeric-cast pre-check. That first changed with 10efa1f, and then again with 15ac848, but neither commit altered the result for any input of a still-valid type.

In its current state, I'd characterize the function's behavior as:

jQuery.isNumeric checks whether a value is a finite number, or a castable string representing one. Non-primitive Number and String objects are treated as their encapsulated primitive values.

If everyone agrees, then I suppose an api.jquery.com PR is in order.

@dmethvin
Copy link
Member

I think that description is fine @gibson042 and would just add another example showing that "-0x42" returns false.

@stevemao
Copy link
Contributor Author

I feel its better to remove this. Some reasons are already mentioned above and I wanna add a few more:

  1. to reduce the size of jQuery core.
  2. $.isNumeric('0b1') would return different values in old and new browsers.
  3. people can easily require different implementations to check numeric values to their needs.

@timmywil
Copy link
Member

Opened docs issue. jquery/api.jquery.com#862

gibson042 added a commit to gibson042/api.jquery.com that referenced this issue Jan 12, 2016
stevemao added a commit to stevemao/jquery that referenced this issue Jan 14, 2016
Add back accidentally deleted comments about the implementation.
Add tests for new number literals.

Ref jquery#2663
Ref jquery#2781
Ref jquery#2780
AurelioDeRosa pushed a commit to jquery/api.jquery.com that referenced this issue Jan 17, 2016
stevemao added a commit to stevemao/jquery that referenced this issue Jan 24, 2016
Add back accidentally deleted comments about the implementation.
Add tests for new number literals.

Ref jquery#2663
Ref jquery#2781
Ref jquery#2780
gibson042 pushed a commit that referenced this issue Jan 25, 2016
Also add back accidentally deleted comments about the implementation.

Fixes gh-2780
Ref gh-2663
Ref gh-2781
Closes gh-2827
@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

5 participants