Bug Tracker

Opened 13 years ago

Closed 13 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:


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)

makeArray.diff (713 bytes) - added by flesler 13 years ago.

Download all attachments as: .zip

Change History (14)

Changed 13 years ago by flesler

Attachment: makeArray.diff added

comment:1 Changed 13 years ago by flesler

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.

comment:2 Changed 13 years ago by flesler

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:

var scripts = document.getElementsByTagName('script');
if( $.makeArray(scripts).slice(0,1)[0].nodeName != 'SCRIPT' )

if( (function(){ return $.makeArray(arguments); })(1,2).join('') != '12' )

//real array
if( $.makeArray([1,2,3]).join('') != '123' )

if( $.makeArray().length != 0 )
if( $.makeArray( 0 )[0] != 0 )
if( $.makeArray( 'foo' )[0] != 'foo' )
if( typeof $.makeArray( true )[0] != 'boolean' )
if( $.makeArray( scripts[0] )[0].nodeName != 'SCRIPT' )
//array-like object
if( $.makeArray( {length:2, 0:'a', 1:'b'} ).join('') != 'ab' )

//childnodes array
if( $.makeArray( document.documentElement.childNodes ).slice(-1)[0].nodeName != 'HEAD' )
//function (tricky, they have length)
if( $.makeArray( function(){ return 1;} )[0]() != '1' )

comment:3 Changed 13 years ago by flesler

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:

		arr = Array(30),
		times = 1e5,
		i, t;

	t = times;
	while( t-- ){
		var i = arr.length
		while( i )
			arr[--i] = arr[i];

	t = times;
	while( t-- )

I made a fake console with time() and timeEnd() for all but Firefox. Results:

Firefox (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 13 years ago by flesler

One correction: the while loop was an experiment, i was comparing loops. The real loop (the one in the code) is:

	t = times;
	while( t-- ){
		for( i = arr.length; i; )
			arr[--i] = arr[i];

Benchmarked all back and the numbers were the same.

comment:5 Changed 13 years ago by joern

Could you rewrite those tests using the jQuery testsuite?

comment:6 Changed 13 years ago by flesler

Commited the required tests on [5284].

comment:7 Changed 13 years ago by joern

Commited patch: [5314]. Keeping ticket open until a few other places make use of the new implementation.

comment:8 Changed 13 years ago by flesler

Simplified the code using the new makeArray where possible, at [5137]

comment:9 Changed 13 years ago by joern

That should have been [5317].

comment:10 Changed 13 years ago by joern

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

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

Fixed the detection for window, and added a test case for window and a regex at [5318].

comment:13 Changed 13 years ago by joern

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.