Skip to main content

Bug Tracker

Side navigation

#5499 closed enhancement (wontfix)

Opened November 12, 2009 02:54PM UTC

Closed November 22, 2010 06:36AM UTC

Last modified March 10, 2012 10:15AM UTC

jQuery.each

Reported by: dbjdbj Owned by:
Priority: low Milestone: 1.5
Component: core Version: 1.3.2
Keywords: foreach Cc:
Blocked by: Blocking:
Description

Can we please make jQuery more robust by using hasOwnProperty to check each item in the jQuery.each loop to be sure it belongs to the object itself:

1 forEach = function(obj, f) {

2 for (var key in obj) {

3 if (obj.hasOwnProperty(key)) {

4 f.call(obj, obj[key], key);

5 }

6 }

7 };

I know this might slow jQuery down.

Regards: DBJ

Attachments (0)
Change History (15)

Changed November 12, 2009 02:55PM UTC by dbjdbj comment:1

<code>

1 forEach = function(obj, f) {

2 for (var key in obj) {

3 if (obj.hasOwnProperty(key)) {

4 f.call(obj, obj[key], key);

5 }

6 }

7 };

</code>

Changed November 12, 2009 02:57PM UTC by dbjdbj comment:2

 1 forEach = function(obj, f) { 
 2  for (var key in obj) { 
 3   if (obj.hasOwnProperty(key)) { 
 4    f.call(obj, obj[key], key); 
 5   } 
 6  } 
 7 }; 

Changed November 18, 2009 01:05AM UTC by dbjdbj comment:3

Please see :

<code>

http://github.com/DBJDBJ/jquery/blob/master/speed/dbj.each.js

http://github.com/DBJDBJ/jquery/blob/master/speed/dbj.each.htm

</code>

It appears the speed difference might not be that bad. Slightly slower, but more robust?

This requires a lot more testing.

--DBJ

Changed November 26, 2009 08:45AM UTC by dbjdbj comment:4

I added this version in my fork:

// Ticket#5499  ---> use hasOwnProperty()
each: function( object, callback, args ) {
var name, length = object.length, value;
if (length === undefined || jQuery.isFunction(object)) {
    if (args) {
        for (name in object) {
         if (hasOwnProperty.call(object,name) && (callback.apply(object[name], args) === false)) break;
        }
    } else {
       for (name in object) {
            if (hasOwnProperty.call(object,name) && callback.call(value = object[name], name, value) === false)) break;
        }
    }
} else {
    if (args) {
        while (length-- && (callback.apply(object[length], args) !== false));
    } else {
        while (length-- && (callback.call(value = object[length], length, value) !== false));
    }
}
return object;
},

Any comments?

--DBJ

Changed November 26, 2009 04:33PM UTC by dbjdbj comment:5

I think I would like to change the logic of each() ? Fiddling with the ticket #5554, made me think. If each() would do only arrays as arrays, that is using lemgth, and "everything" else with for/in loop what dould happend then? What I mean is what if each() would look like this :

// Ticket#5499  ---> use hasOwnProperty()
// also just divide on isArray()
each: function( object, callback, args ) {
var name, value;
if ( ! jQuery.isArray(object)) {
    if (args) {
        for (name in object) {
         if (hasOwnProperty.call(object,name) && (callback.apply(object[name], args) === false)) break;
        }
    } else {
       for (name in object) {
            if (hasOwnProperty.call(object,name) && callback.call(value = object[name], name, value) === false)) break;
        }
    }
} else {
var length = object.length ;
    if (args) {
        while (length-- && (callback.apply(object[length], args) !== false));
    } else {
        while (length-- && (callback.call(value = object[length], length, value) !== false));
    }
}
return object;
},

Above will not iterate over jQuery instance using its length property. But is that an issue? jQuery is an object and for/in above , will iterate over its "stack" properly.

Changed November 27, 2009 09:42AM UTC by dbjdbj comment:6

After few days (!), and several false commits, I think I am finally satisfied with the state of affairs in my core.js ( http://github.com/DBJDBJ/jquery/blob/master/src/core.js )

This is really a non-trivial change and involves merge(), grep(), map(), makeArray() as users of this.

Testing and revieweing by the core team , I think, might be required here.

Thanks: Dusan

Changed February 26, 2010 09:42AM UTC by dbjdbj comment:7

Now, I realize it will be never feasible to try and improve jQuery.each(). It is way non-standard. It uses non ES5 callbacks and it uses "args" arguments which is also not part of ES5 callback footprint. It also can iterate over function arguments.

It is not feasible to "improve" it by using (native or not) Array.forEach, Object.keys, etc ...

It is much more feasible to develop jQuery.forEach( object, callback ), which will be built upon ES5 iteration and callbacks concepts.

object type/class in this instance may be : array, object, string.

And even number , or DOM node. Internally it would use native or not, ES5 Array iteration methods.

And yes it might or might not take in (while iterating) properties which are not "owned" by the object.

jQuery.forEach( object, callback, use_only_own_properties ) {

use_only_own_properties = use_only_own_properties || true ;

}

Thanks: DBJ

Changed October 21, 2010 07:24PM UTC by rwaldron comment:8

.hasOwnProperty(key) lookups are too costly for realistic addition to $.each()

Changed October 21, 2010 07:40PM UTC by rwaldron comment:9

resolution: → wontfix
status: newclosed

Changed October 21, 2010 07:50PM UTC by rwaldron comment:10

milestone: 1.3.2
resolution: wontfix
status: closedreopened

Changed October 21, 2010 07:51PM UTC by rwaldron comment:11

milestone: → 1.5
priority: majorlow

Changed October 21, 2010 07:52PM UTC by rwaldron comment:12

Will create a patch and perf tests to see if there is any benefit.

Changed October 21, 2010 10:29PM UTC by dbjdbj@gmail.com comment:13

I can't believe this was 11 months ago ? Thanks for looking into this again.

Benefit(s) I think are purely in conformance to ES5.

jQuery.forEach() would deliver that.

In the same time making jQuery.each() a candidate for deprecation.

A real can of worms, I think we all agree.

--DBJ

Changed November 22, 2010 06:36AM UTC by snover comment:14

resolution: → wontfix
status: reopenedclosed

There is potentially a benefit to allowing people to enumerate the prototype’s properties as well; if we forcibly filter it away, then nobody gets that benefit. If someone really needs to filter, they can do it in their callback.

Changed January 22, 2012 05:44PM UTC by sun comment:15

_comment0: Other frameworks are automatically filtering by hasOwnProperty() in their loop helpers. \ \ This change would vastly help projects like Drupal, which can be extended in unlimited ways and potentially loading any kind of externally developed JavaScript - whereas extending prototypes is considered bad practice within Drupal itself, so its core code does not necessarily account for the possibility. \ \ > allowing people to enumerate the prototype’s properties as well \ \ That sounds like an extreme edge-case, for which developers can still use a custom for...in loop? \ \ Alternatively, I don't see \ \ 1) why this couldn't be a function argument (shifting the internal args), or \ \ 2) why there couldn't be a .eachOwn()? \ 1327254830110778

Other frameworks are automatically filtering by hasOwnProperty() in their loop helpers.

This change would vastly help projects like Drupal, which can be extended in unlimited ways and potentially loading any kind of externally developed JavaScript - whereas extending prototypes is considered bad practice within Drupal itself, so its core code does not necessarily account for the possibility.

allowing people to enumerate the prototype’s properties as well

That sounds like an extreme edge-case, for which developers can still use a custom for...in loop?

Alternatively, I don't see

1) why this couldn't be a function argument (shifting the internal args), or

2) why there couldn't be a .eachOwn()?

EDIT: Related: https://github.com/jquery/sizzle/issues/21