#2968 closed bug (fixed)

Opened June 02, 2008 03:48PM UTC

Closed July 23, 2008 04:18PM UTC

Last modified March 14, 2012 05:00PM UTC

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)

2968.diff (1.1 KB) - added by genezys July 07, 2008 08:41PM UTC.

Patch implementing suggested algorithm (simplified and reformatted)
2968.test.diff (1.3 KB) - added by genezys July 07, 2008 08:42PM UTC.

Added test cases for isFunction (merging these one and bug #3134)

Change History (9)

Changed June 04, 2008 03:56PM UTC by flesler comment:1

owner:	→ flesler
status:	new → assigned

Changed June 04, 2008 05:28PM UTC by Andrea Giamm comment:2

Just a note, flesler, to respect return boolean value, other browsers could better use this one:

function( fn ) {
    return !!fn && fn instanceof Function;
}

so null or undefined will be returned as false

Changed June 04, 2008 07:42PM UTC by Andrea Giamm comment:3

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 July 08, 2008 12:00AM UTC by Andrea Giamm comment:4

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

Changed July 08, 2008 10:33AM UTC by Andrea Giamm comment:5

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

Changed July 22, 2008 04:53PM UTC by flesler comment:6

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.

Changed July 23, 2008 04:18PM UTC by flesler comment:7

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.

Changed October 15, 2008 04:52PM UTC by choan comment:8

Replying to [comment:6 flesler]: