#2968 closed bug (fixed)
isFunction and object problems
Reported by: | Andrea Giammarchi | Owned by: | flesler |
---|---|---|---|
Priority: | major | Milestone: | 1.3 |
Component: | core | Version: | 1.2.6 |
Keywords: | isFunction | Cc: | |
Blocked by: | Blocking: |
Description
Version 1.2.6 of jQuery has a public static isFunction method that fails in some case.
These are only two cases:
jQuery.isFunction(new String("function")); // true jQuery.isFunction({toString:function(){return "function"}}); // true jQuery.isFunction({valueOf:function(){return "function"}}); // true
At the same time, every compatible browser, IE a part, could simply use this function:
isFunction:function(fn){return fn && fn instanceof Function}
that seems to work as expected with every unit test case.
My suggestion is to implement latter function for Firefox, Opera, and Safari, plus a dedicated one for IE:
isFunction: jQuery.browser.msie ? function( fn ) { if( !fn ) return false; var s = "toString", v = "valueOf", t = typeof fn[s] === "function" && fn[s], o = typeof fn[v] === "function" && fn[v], r; if( t ) delete fn[s]; if( o ) delete fn[v]; r = typeof fn !== "string" && !(fn instanceof String) && !fn.nodeName && fn.constructor != Array && /^[\s[]?function/.test(fn + ""); if( t ) fn[s] = t; if( o ) fn[v] = o; return r; } : function( fn ) { return fn && fn instanceof Function; },
The only problem is that browser object property is assigned too late, but since it does not depend on jQuery, I suggest to move its assignment on the top of the library, to let other methods use a direct assignment strategy, like above one, instead of a lazy one.
Above code has been tested with every compatible IE, plus Opera 9 and Safari 3 ( I could not test Safari 2 yet )
Kind Regards
Attachments (2)
Change History (11)
comment:1 Changed 15 years ago by
Owner: | set to flesler |
---|---|
Status: | new → assigned |
comment:2 Changed 15 years ago by
comment:3 Changed 15 years ago by
flesler ... I have just realized that first check is not useful ... so this is my last proposal that speedup IE too in an optimistic way:
isFunction: jQuery.browser.msie ? function( fn ) { if( fn instanceof Function ) return true; if( !fn ) return false; var s = "toString", v = "valueOf", t = typeof fn[s] === "function" && fn[s], o = typeof fn[v] === "function" && fn[v], r; if( t ) delete fn[s]; if( o ) delete fn[v]; r = typeof fn !== "string" && !(fn instanceof String) && !fn.nodeName && fn.constructor != Array && /^[\s[]?function/.test(fn + ""); if( t ) fn[s] = t; if( o ) fn[v] = o; return r; } : function( fn ) { return fn instanceof Function; },
hiping this will help you and will be a "definitive" solution :)
Changed 15 years ago by
Patch implementing suggested algorithm (simplified and reformatted)
Changed 15 years ago by
Attachment: | 2968.test.diff added |
---|
Added test cases for isFunction (merging these one and bug #3134)
comment:4 Changed 15 years ago by
as I wrote in the ML, please try this code too:
isFunction:function( fn ){ return !!fn && (fn instanceof Function || (typeof fn == "object" && !(fn instanceof Object))); }
that with a browser based implementation, to avoid last useless check, could be:
isFunction:jQuery.browser.msie? function( fn ){ return !!fn && (fn instanceof Function || (typeof fn == "object" && !(fn instanceof Object))); }: function( fn ){ return fn instanceof Function; }
Regards
comment:5 Changed 15 years ago by
Above suggestions fail with different IE cases. Please consider this last piece of code that after some test, seems to be the best one for IE.
isFunction : jQuery.browser.msie ? function( fn ){ return !!fn && (fn instanceof Function || (fn instanceof ActiveXObject && /\[native code\]/.test(fn + ""))); }: function( fn ){ return fn instanceof Function; }
Hoping this will be the last proposal, I am going to test unit cases.
Kind Regards
comment:6 follow-up: 8 Changed 15 years ago by
By using:
function( fn ) { return fn instanceof Function; }
All tests pass on FF and IE only fails on DOM methods (input.focus, foo.getAttribute). I think this is good enough, it will surely bump perfomance.
I can't imagine a situation where one would separate DOM methods from their node to pass them as argument.
comment:7 Changed 15 years ago by
need: | Review → Commit |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
Version: | 1.2.5 → 1.2.6 |
Applied at [5792]. Commented out 2 tests, and the suite passes on all major browsers.
comment:8 Changed 14 years ago by
comment:9 Changed 14 years ago by
I've proposed a solution to the built-in DOM function problem here, see ticket #4536.
Just a note, flesler, to respect return boolean value, other browsers could better use this one:
so null or undefined will be returned as false