Side navigation
#2619 closed enhancement (fixed)
Opened March 29, 2008 08:04PM UTC
Closed April 28, 2008 01:55PM UTC
Extend makeArray
Reported by: | flesler | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | 1.2.4 |
Component: | core | Version: | 1.2.3 |
Keywords: | makearray array | Cc: | |
Blocked by: | Blocking: |
Description
Hi, don't you think makeArray should be a bit smarter?
Instead of doing $.makeArray( foo || [] ) all the time, it should handle that internally.
$.fn.setArray does a lot of checks that IMO, belong to $.makeArray.
By improving it, you can remove those pesky checks from many places.
This version handles pretty much any kind of argument, without using too many checks.
I know that internal functions have to as fast as possible, I optimized this as much as I could, but well, in the end, it's up to you (core team).
Attachments (1)
Change History (13)
Changed March 29, 2008 08:11PM UTC by comment:1
Changed March 29, 2008 08:13PM UTC by comment:2
I made a swift test runner, and after a few fixes (specially for safari), they all passed on IE6, FF2, Opera 9 and Safari 3.
These are the tests:
//nodelist var scripts = document.getElementsByTagName('script'); if( $.makeArray(scripts).slice(0,1)[0].nodeName != 'SCRIPT' ) alert('error-1'); //arguments if( (function(){ return $.makeArray(arguments); })(1,2).join('') != '12' ) alert('error-2'); //real array if( $.makeArray([1,2,3]).join('') != '123' ) alert('error-3'); //empty if( $.makeArray().length != 0 ) alert('error-4'); //number if( $.makeArray( 0 )[0] != 0 ) alert('error-5'); //string if( $.makeArray( 'foo' )[0] != 'foo' ) alert('error-6'); //boolean if( typeof $.makeArray( true )[0] != 'boolean' ) alert('error-7'); //node if( $.makeArray( scripts[0] )[0].nodeName != 'SCRIPT' ) alert('error-8'); //array-like object if( $.makeArray( {length:2, 0:'a', 1:'b'} ).join('') != 'ab' ) alert('error-9'); //childnodes array if( $.makeArray( document.documentElement.childNodes ).slice(-1)[0].nodeName != 'HEAD' ) alert('error-10'); //function (tricky, they have length) if( $.makeArray( function(){ return 1;} )[0]() != '1' ) alert('error-11');
Changed March 29, 2008 08:22PM UTC by comment:3
You might say: "ok, slice is not used for regular arrays". I benchmarked my option against slice and concat (with an empty page w/o jquery or anything else) and the native alternative was only faster on Safari 3. Firefox 2 was actually like 10x times faster with a reversed for.
This is the benchmark:
(function(){ var arr = Array(30), times = 1e5, i, t; t = times; console.time('mine'); while( t-- ){ var i = arr.length while( i ) arr[--i] = arr[i]; } console.timeEnd('mine'); t = times; console.time('native'); while( t-- ) arr.slice(); console.timeEnd('native'); })();
I made a fake console with time() and timeEnd() for all but Firefox.
Results:
Firefox 2.0.0.13 (turned firebug off) * mine: 641ms * native: 5312ms IE 6 * mine: 2078ms * native: 2422ms Opera 9.22 * mine: 1656ms * native: 1844ms Safari 3.0.4 * mine: 1281ms * native: 344ms
This should be considered for the rest of the code as well, native methods might not always be the fastest.
Changed March 29, 2008 08:26PM UTC by comment:4
One correction: the while loop was an experiment, i was comparing loops.
The real loop (the one in the code) is:
t = times; console.time('mine'); while( t-- ){ for( i = arr.length; i; ) arr[--i] = arr[i]; } console.timeEnd('mine');
Benchmarked all back and the numbers were the same.
Changed April 22, 2008 10:22PM UTC by comment:5
Could you rewrite those tests using the jQuery testsuite?
Changed April 22, 2008 11:16PM UTC by comment:6
Commited the required tests on [5284].
Changed April 24, 2008 09:25PM UTC by comment:7
Commited patch: [5314]. Keeping ticket open until a few other places make use of the new implementation.
Changed April 24, 2008 09:47PM UTC by comment:8
Simplified the code using the new makeArray where possible, at [5137]
Changed April 24, 2008 09:53PM UTC by comment:9
That should have been [5317].
Changed April 24, 2008 09:57PM UTC by comment:10
I wonder if makeArray can really replace all those checks in the init method:
// Watch for when an array-like object, contains DOM nodes, is passed in as the selector (selector.jquery || selector.length && selector != window && !selector.nodeType && selector[0] != undefined && selector[0].nodeType) && jQuery.makeArray( selector )
Odds are good that the testsuite doesn't really cover all of them.
The other changes look fine.
Changed April 24, 2008 11:20PM UTC by comment:11
I just checked, and 'setArray' is only used inside 'init'. 'init' is only used inside jQuery.
In consequence, setArray needs to handle whatever $() handles.
String selectors/html and functions are sniffed out before, that only leaves us with:
- Arrays
- Array-like (jQuery, nodelist)
- DOM elements
The first two are of course handled. DOM elements are checked first, prior to this.
We could actually remove that prior check because makeArray handles it well.
Of course, the this[0] = selector part, is faster than a whole makeArray+setArray, so
that if can be kept to speed up that case.
This enhancement to makeArray, actually opens up some new possibilities.
Like $( 2 ), $( new Date ), $( {foo:'bar'} ), $( /[abc]/ ), $( new MyClass ), etc.
I think this is really good!
The only problem that might arise is with window, as it has length.
We can add a " && !array.location " or .setInterval, to detect the window.
The !array.call can be removed if we don't need to support functions.
Changed April 25, 2008 03:53AM UTC by comment:12
Fixed the detection for window, and added a test case for window and a regex at [5318].
Changed April 28, 2008 01:55PM UTC by comment:13
resolution: | → fixed |
---|---|
status: | new → closed |
The check for array.call can be removed, if there's no interest on supporting functions, I just added because it may happen.
!array.split can be changed for typeof array != 'string' if object detection is not the best option.