Bug Tracker

Modify

Ticket #2968 (closed bug: fixed)

Opened 6 years ago

Last modified 2 years ago

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:
Blocking: Blocked by:

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

2968.diff Download (1.1 KB) - added by genezys 6 years ago.
Patch implementing suggested algorithm (simplified and reformatted)
2968.test.diff Download (1.3 KB) - added by genezys 6 years ago.
Added test cases for isFunction (merging these one and bug #3134)

Change History

comment:1 Changed 6 years ago by flesler

  • Owner set to flesler
  • Status changed from new to assigned

comment:2 Changed 6 years ago by Andrea Giamm

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

comment:3 Changed 6 years ago by Andrea Giamm

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 6 years ago by genezys

Patch implementing suggested algorithm (simplified and reformatted)

Changed 6 years ago by genezys

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

comment:4 Changed 6 years ago by Andrea Giamm

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 6 years ago by Andrea Giamm

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 6 years ago by flesler

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 6 years ago by flesler

  • need changed from Review to Commit
  • Status changed from assigned to closed
  • Version changed from 1.2.5 to 1.2.6
  • Resolution set to fixed

Applied at [5792]. Commented out 2 tests, and the suite passes on all major browsers.

comment:8 in reply to: ↑ 6 Changed 6 years ago by choan

Replying to flesler:

By using:

function( fn ) {
    return fn instanceof Function;
}

Unfortunately, memory leaks appear on IE6 when applying instanceof to COM objects. I've created a ticket and provided a patch at #3485

comment:9 Changed 5 years ago by leidegre

I've proposed a solution to the built-in DOM function problem here, see  ticket #4536.

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.