Permalink
11 comments
on commit
sign in to comment.
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Switch to using String.prototype.trim from String.trim as it's more-w…
…idely available.
- Loading branch information
Showing
1 changed file
with
3 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't believe how much time we have spent discussing string trim .... And ... I still can't stop ?
What strikes me above is that if String.prototype.trim is available then using the native trim boils down .. to erm, actually using it :
function( text ) {
return text.trim() ;
} :
// Otherwise use our own trimming functionality
Above will also behave properly, and in the case of text , not being a string, the above will/should throw TypeError.
--DBJ
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really prefer to not throw an exception - we generally try to be forgiving of user input (trying to handle null/undefined gracefully, for example) and I'd like to continue that trend.
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
function( text ) {
return ! text ? "" : text.toString().trim() ;
} :
// Otherwise use our own trimming functionality
-- DBJ
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
function( text ) {
return text && text.toString().trim() || "";
} :
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course none of those solutions handle trimming false or 0 correctly.
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok true, but why you want to trim non-strings ?
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't before. 1.3.2 had:
return (text || "").replace( ...
If false should be trimmed to "false", why not null/undefined -> "null"/"undefined"?
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. That's what I was thinking.
Personal I think trim should only be executed on Strings, but EcmaScript says anything can be trimmed (even null).
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bug fix from what was in 1.3.2, we're now implementing string trimming in accordance with the built-in string trimming - and this includes trimming non-strings (which is why we do .toString() in the first place). In the built-in string trimming working against null/undefined is equivalent to trimming the global object which is highly counter-intuitive - so we return an empty string instead (which a much more intuitive result and more inline with the rest of the jQuery API).
ba8938d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer "null" and "undefined" which are more informative on debug.