…totype.toString.call). Fixes #3330.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -438,18 +438,22 @@ jQuery.extend({ | ||
| // Since version 1.3, DOM methods and functions like alert | ||
| // aren't supported. They return false on IE (#2968). | ||
| isFunction: function( obj ) { | ||
| return jQuery.type(obj) === "function"; | ||
| }, | ||
|
|
||
| isArray: function( obj ) { | ||
| return jQuery.type(obj) === "array"; | ||
| }, | ||
|
|
||
| type: function( obj ) { | ||
| return toString.call(obj).slice(8, -1).toLowerCase(); | ||
| }, | ||
|
|
||
| isPlainObject: function( obj ) { | ||
| // Must be an Object. | ||
| // Because of IE, we also have to check the presence of the constructor property. | ||
| // Make sure that DOM nodes and window objects don't pass through, as well | ||
| if ( !obj || jQuery.type(obj) !== "object" || obj.nodeType || obj.setInterval ) { | ||
| return false; | ||
| } | ||
|
|
||
| @@ -595,9 +599,9 @@ jQuery.extend({ | ||
| // The extra typeof function check is to prevent crashes | ||
| // in Safari 2 (See: #3039) | ||
| // Tweaked logic slightly to handle Blackberry 4.7 RegExp issues #6930 | ||
| var type = jQuery.type(array); | ||
|
|
||
| if ( array.length == null || type === "string" || type === "function" || type === "regexp" || (typeof type !== "function" && array.setInterval) ) { | ||
jeresig
Author
Member
|
||
| push.call( ret, array ); | ||
| } else { | ||
| jQuery.merge( ret, array ); | ||
38 comments
on commit 94f35d0
There was a problem hiding this comment.
How do you intend to document the new jQuery.type. Something like "Acquiring type of any not-undefined / not-null / not-host values" ?
Since it is not only an internal utility function, wouldn't it be better to make it more generic like the one already proposed (http://gist.github.com/305222) that is also more consistent on different browsers (forum discussion) ?
At least you would support undefined/null values:
return obj == null ? obj + '' : toString.call(obj).slice(8, -1).toLowerCase();
Also, I would suggest to use the native Array.isArray when it exists:
isArray: Array.isArray || function( obj ) { ...
There was a problem hiding this comment.
It's not technically its type it's the [[Class]].
ES5 Page 33:
The value of the [[Class]] internal property is defined by this specification for every kind of built-in object. The value of the [[Class]] internal property of a host object may be any String value except one of "Arguments", "Array", "Boolean", "Date", "Error", "Function", "JSON", "Math", "Number", "Object", "RegExp", and "String". The value of a [[Class]] internal property is used internally to distinguish different kinds of objects. Note that this specification does not provide any means for a program to access that value except through Object.prototype.toString (see 15.2.4.2).
See also:
http://github.com/jdalton/fusejs/blob/master/src/lang/object.js#L245-250
There was a problem hiding this comment.
As it is, I have to suppose that the intention was to provide an more cross-browser typeof (note the .toLowerCase() at the end).
Retrieving [[Class]], in this case, is only an implementation detail.
EDIT:
The commit message suggests that the intention of jQuery.type was simply to retrieve [[Class]], but why than it returns the string in lower case, and the function is called "type" instead of something like "classOf"?
There was a problem hiding this comment.
It would definitely be useful to have null return 'null' and undefined return 'undefined'. Note that there is already a jQuery.type extension in the plugins repo with similar intent, but I don't know how commonly used it might be. http://plugins.jquery.com/project/type
There was a problem hiding this comment.
How is citing anything other than spec helpful? The fact that Object.prototype.toString() is used is a clear indication of [[Class]]. I am not guessing at John's intentions I am telling you how it is in black and white, in the exact words of the specification. If you call it type the specification defines the types as
8 Types ................................................................................................................................................... 28
8.1 The Undefined Type ............................................................................................................................ 28
8.2 The Null Type ...................................................................................................................................... 28
8.3 The Boolean Type ............................................................................................................................... 28
8.4 The String Type .................................................................................................................................. 28
8.5 The Number Type ............................................................................................................................... 29
8.6 The Object Type ................................................................................................................................. 30
Following spec is more than an academic exercise. It helps devs understand the language and functionality of the API. It also help when defining methods to follow the precedents of the specification so that eventually jQuery could use the native versions of such methods. (ex: Array#forEach and Array#map)
There was a problem hiding this comment.
LOL... John, when I showed you http://gist.github.com/280043, you said I was mental!
There was a problem hiding this comment.
I have a 'solution' to the issue with $.type, rename it to $.what. Yes, it is more ambiguous, however it doesn't spread misinformation and it's easy to grasp. As well it fits nicely into jQuery's short, direct API.
There was a problem hiding this comment.
Sorry gf3 but... LOLWAT?!?
There was a problem hiding this comment.
John: "Additionally checking the setInterval property on the NodeList throws an exception in the older WebKits - thus we use that check to our advantage."
NodeList' objects don't have asetInterval` property. An attempt to get a property off a NodeList can cause the browser to crash, not throw an exception.
In Safari 2, given an Element not in the document, getting a nodeList off that element (e.g. myDiv.childNodes) and then attempting to access a non-existing property such as myDiv.childNodes.setInterval, will crash the browser. I can't remember if the element had to be removed from the document or not. NEver bothered me that much, as I tend not to do such things on my own, so the details are not that interesting (and so forgettable).
There was a problem hiding this comment.
I agree with jdalton -- className would be better at a method name than type, and toLowerCase is a bit dangerous (who says all class names are capitalized, and that there are no conflicts between LoserClass and loserClass?) as well as unnecessary overhead.
/be
There was a problem hiding this comment.
Why not just use the type checking function from Raphael?
http://github.com/DmitryBaranovskiy/raphael/blob/master/raphael.js#L110
Dmitry can get very passionate about type checking.
There was a problem hiding this comment.
@ALL: I chose the name
@rkatic/@dmethvin: Agreed on null/undefined - I will make some tweaks to make sure that we handle those correctly.
@rkatic: Disagreed on the Array.isArray check. While it may provide a nominal speed up in some browsers - using that API has potential for cross-platform inconsistencies to arise.
@garetts: We're checking the setInterval property to roughly determine if the object is a window or not. The fact that it's being tested against a NodeList in that case is incidental. You are correct that the word 'exception' is a misnomer in this case as it does crash older WebKits outright (this is the reason for the additional typeof check, to make sure we don't get hit by that case). I've updated my comment to be more accurate.
There was a problem hiding this comment.
You guys wonder me . Are you really think that someone write such crazy code?
// checking by .constructor/instanceof is bad
var a = []; a.constructor = String
Array.isArray = function(){ return Object.prototype.toString.call(this) == '[object Array]' }
instead faster version
Array.isArray = function(){ return this.constructor == Array || Object.prototype.toString.call(this) == '[object Array]' };
yeah i know about frames, but it 0.000001% cases in real developing
// replace Object function like hasOwnProperty or toString
var a = {} a.hasOwnProperty = function(name){ return false; };
Object.prototype.hasOwnProperty.call
instead
a.hasOwnProperty
If user really want replace hasOwnProperty - he know what he do
global.undefined = 1; // Infinity NaN ...
no comments
// primitives in object form
var a = new String('aaa');
it slowdown code
it break typeof
lol who use it?
there is one exception in ES3(not in ES5)
String.prototype._fn = function(){ return this; }; typeof(''._fn()) // 'object'
I dont know why ES3 coerces engines convert primitives to object form - it only slowdown code and make troubles, but it ecma history
Because my rule is convert primitives in object form back like
String.prototype._fn = function(){ return '' + this; }; typeof(''._fn()) // 'string'
If someone really write such code - it trouble of he, not framework/library bug
There was a problem hiding this comment.
Brendan, while that appears to be a great suggestion on the surface, you need to consider that in the context of a primarily DOM-oriented toolkit like jQuery, calling a non-class-attribute method className might be even more confusing to end users (even if it is a static method).
There was a problem hiding this comment.
cowboy,
that's roughly the same reason why "type" can be confusing (in DOM contexts too). That's the reason why I prefer @gf3 suggestion ("what" or similar), staying away from duplicating similar names/functionalities and avoid mixing up expected results to javascript developers.
There was a problem hiding this comment.
To follow cowboy and dperini's line about DOM-oriented toolkit.
When devs try jQuery.type(document.body) in IE they will get object, but in Firefox and others they will get htmlbodyelement.
I could also see the confusion of a jQuery dev trying jQuery.type(inputElement) and thinking why don't I get its type attribute value.
There was a problem hiding this comment.
@jdalton: exactly the problem of different results with host objects, is something that my gist resolves (http://gist.github.com/305222). Maybe it is not ideal to return "object" for every DOM object, but at least it is consistent.
There was a problem hiding this comment.
@rkatic: Interesting tweaks - I like the point about returning 'object' for other objects for consistency. Want to make a patch and do a pull request (with tests)?
There was a problem hiding this comment.
@rkatic: Disagreed on the Array.isArray check. While it may provide a nominal speed up in some browsers - using that API has potential for cross-platform inconsistencies to arise.
Bananas :P
jQuery uses native Array#push and other native array methods that could be paved or overwritten. The spec for Array.isArray even states
15.4.3.2 Array.isArray ( arg )
The isArray function takes one argument arg, and returns the Boolean value true if the argument is an object whose class internal property is "Array"; otherwise it returns false. The following steps are taken:
- If Type(arg) is not Object, return false.
- If the value of the [[Class]] internal property of arg is "Array", then return true.
- Return false.
which is inline with your implementation. And speaking of speed, abstracting something like getting [[Class]] a few methods deep affects the performance of these methods esp in mobile. I don't know how often they are used in the core but I am sure it adds up.
There was a problem hiding this comment.
I have to agree with @jdalton here. Using jQuery.type mite confuse people as another way of using jQuery.attr("type", ...);. Maybe using the Javascript implemented way of jQuery.typeOf would be more in the line of XB coding.
There was a problem hiding this comment.
@rkatic, should a DOM element not return Element...
There was a problem hiding this comment.
@jdalton: You're mis-interpreting what I said. There is no reason for us to use isArray since we already have the exact code there that will achieve the same result at approximately the same speed across all platforms. Native methods like .push() have nothing to do with this as they aren't something that that only exists in the latest bleeding-edge browsers.
There was a problem hiding this comment.
I read that you're using typeof to guard against a crash caused by getting a not-defined property off a NodeList. You should have tested that presumption to make sure that it will actually prevent the crash. Actually it does not prevent that crash/ A [[HasProperty]] check will (using in and probably indirect hasOwnProperty check, though I've not tried that).
IMO, the crux of the problem on the Safari bug is to avoid it by not putting the code in the position of having to discriminate between NodeList and window.It is useless trivia.
In response to your fist line comment regarding typeof I had first posted a comment to the effect of "Sorry, but that is not correct..." and then suggested that it would have been nice if you'd joined in on some of the discussions on comp.lang.javascript. I don't think I deleted it, though mistakes can happen. Did you delete that, John?
There was a problem hiding this comment.
@jerone: Can you be more specific? What the word Element stands for? Returns where?
There was a problem hiding this comment.
@jeresig Since when has using native methods when available been a bad thing. http://jsperf.com/isarray-vs-other (native is millions of executions vs thousands for manual)
There was a problem hiding this comment.
jeresig, cowboy: good point about class being confusing in context of CSS and the DOM. I sometimes reach for "kind" when looking for a synonym that won't be confused.
People seem divided here between folks following the core language spec (ES5 now) and people who look to JS libraries and the DOM (or CSS) too. That is a conflict I won't even try to resolve. It's cool to make your own conventions and keep your own meanings.
"When I use a word," Humpty Dumpty said in rather a scornful tone, "it means just what I choose it to mean -- neither more nor less."
/be
There was a problem hiding this comment.
Last comment from me and I'll be quiet: I agree with jdalton again on using Array.isArray if it's there. It's in Fx4, Saf5, IE9, and Chrome (v8 tracks JSC). It is faster than anything that allocates strings and converts case. It seems worth using via || for best effect on the latest browsers (which I hope aren't bleeding too badly, even if they are edgy ;-).
/be
There was a problem hiding this comment.
@rkatic, every DOM object is inherited from the Element.prototype. Check the following example for a normal <div/>:
HTMLDivElement.prototype
|
HTMLElement.prototype
|
Element.prototype
|
Node.prototype
|
Object.prototype
|
null
Returning Element for every different DOM object would create unity.
There was a problem hiding this comment.
@jdalton: It's bad when it splits your code base into multiple branches. We always try to avoid additional branches in the logic to reduce the amount of confusion and uncertainty across browsers. However your perf test case convinced me that we should try to use it, even with this new branch so I landed it here: ea8b158
@BrendanEich: kind might be an option - although it's in the same realm as 'is' and 'which'. Definitely still something to mull over, though.
@GarrettS: We're only using the typeof to guard against touching a NodeList in WebKit browsers - due to this the property check simply never occurs in the browser and doesn't cause the crash. I created a sample page and can confirm that it works fine in Safari 2: http://ejohn.org/files/isWindow.html I should mention that the check really only makes sense within the context of this particular function - a true isWindow utility function would have to be much more robust.
As to your other short, snarky, comment, I removed it after you posted your explanation of callable host objects it as it didn't seem to include any content relevant to this discussion.
There was a problem hiding this comment.
@jerone: I suppose you are thinking about Node.prototype and not Element.prototype, because only element nodes eventually inherits Element.prototype.
Also, that prototype chain is neither an de-facto standard (IE), neither a part of ecma spec.
To test if an object is a node, you can do obj.nodeType. Doing that in
There was a problem hiding this comment.
@rkatic, both Element.prototype and HTMLElement.prototype are being just by multiple browsers. There's however very small amount of documentation and it's not much used around scripts. I can't say which browsers and up to how low the version they support. I do however find Element, HTMLElement or even Node a better definition than the overall used Object (as everything is in a way a Object). Just my opinion, have fun.
There was a problem hiding this comment.
First, you need a case that crashes Safari 2. Next, you need to find a workaround to that.
Try following the steps to reproduce the crash. I've outlined those above. You need to do two things before triggering the crash: 1) add a text node to that pre and then call 2) remove child. I believe that the typeof operator won't stop the crash at all. Again, this should be useless trivia; just don't put the code in a position where it needs to discern between window objects and NodeList objects.
I'm having trouble seeing how you got "sorry you did not participate in the discussion on comp.lang.javascript" as being snarky. Then again, I don't have that text verbatim, so there's a chance that there was a way to interpret it differently. We'll never know because you deleted it.
As for the censorship, if you've been reading my tweets and posts on comp.lang.javascript, you should have a fairly good idea of how strongly I feel on that.
There was a problem hiding this comment.
I thought jQuery only claimed to support Safari 3.0+, does this really matter? (via http://jquery.com/)
There was a problem hiding this comment.
@cowboy: A browser crash (even if it's not a browser that we support) is always a serious concern.
@GarrettS: I've been having a hard time replicating an actual crasher. The only active demo that I've found that actually crashes the browser is this MooTools one: http://tripledoubleyou.subtlegradient.com/Examples/mootools%20safari2%20crasher.html - but reducing it any further doesn't seem to yield the same result and the crash only seems to happen randomly. Either way - as you've mentioned using 'in' seems to avoid the problem entirely, and since it's shorter than the existing code that we were using, it makes sense to switch. I know that when we looked at this problem previously (a couple years back) and added in that typeof check it did solve the problem and we haven't had any complaints since.
Unfortunately I don't really have time to follow Twitter and don't have any interest in ever visiting CLJ so I'll just have to assume that you're not in favor. This repository and thread are all about the technical discussion of the commits going to jQuery. I've removed off-topic or glib comments before and will continue to do so to keep the discussions on track and useful for the jQuery team.
There was a problem hiding this comment.
For some more info on the crash and a possible test here is Prototype's patch from 07 that deals with this bug:
http://github.com/sstephenson/prototype/commit/a6984e
And a patch from Kangax's fork that shows the in operator use:
http://github.com/kangax/prototype/commit/1a375
Unfortunately I don't really have time to follow Twitter and don't have any interest in ever visiting CLJ so I'll just have to assume that you're not in favor.
Kangax posts to c.l.j. and I am guessing you would treat him better. I mean Garrett is taking his time to help here after all (instead of bashing you on the c.l.j.) and you seem more than willing to take his advice. Just saying.
There was a problem hiding this comment.
@jeresig: committed and made pull request (http://github.com/rkatic/jquery/commit/452c292daa8de2d45cf283bd51cc98d90e04327f).
There was a problem hiding this comment.
On running the tests repeatedly in Firefox 3.6.8 on Linux x86.64 non-native is frequently the better option, with the native being 30% slower.
But sometimes native is faster by 30%, and the other times they're almost equal... there's never any difference other than 0% or 30%. Timer problem?
There was a problem hiding this comment.
"I've been having a hard time replicating an actual crasher. " Did you follow the instructions I provided? Forget a bout Moo Tools and write a simple example. You're almost there with your own example, just give the PRE a text node child and then remove it from the dom using pre.parentNode.removeChild(pre). Next, get pre.childNodes and then I believe that should crash Safari 2.
But above all, you should not ever need to answer the question: Is this object a NodeList or a window?
On the flip side, all of this information has been posted on c.l.js. much of it by myself, Cornford, and kangax, so the rehashing could have been easily avoided by not shunning c.l.js.
I've started a new thread there "Mistakes, Criticism, and Flames".
is typeof type !== "function" always true?