Opened 14 years ago
Closed 14 years ago
#2619 closed enhancement (fixed)
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?
[] ) 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 (14)
Changed 14 years ago by
Attachment: | makeArray.diff added |
---|
comment:1 Changed 14 years ago by
comment:2 Changed 14 years ago by
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');
comment:3 Changed 14 years ago by
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.
comment:4 Changed 14 years ago by
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.
comment:7 Changed 14 years ago by
Commited patch: [5314]. Keeping ticket open until a few other places make use of the new implementation.
comment:8 Changed 14 years ago by
Simplified the code using the new makeArray where possible, at [5137]
comment:10 Changed 14 years ago by
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.
comment:11 Changed 14 years ago by
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.
comment:12 Changed 14 years ago by
Fixed the detection for window, and added a test case for window and a regex at [5318].
comment:13 Changed 14 years ago by
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.