Suggested alteration to PrototypeJS' Object.keys implementation.

[Andy E]

I recently discovered that several compatibility implementations of
Object.keys() on the web will throw an error for DOM objects in
Internet Explorer 8 and lower. This differs from the current browsers
having a native implementation and how the ECMAScript 5th edition
defines the behaviour of Object.keys.

The problem lies with the hasOwnProperty() check, which is not a valid
method on Internet Explorer DOM objects because they aren't instances
of Object. The fix is very simple; "borrow" hasOwnProperty() from
Object.prototype.hasOwnProperty() instead. The fixed PrototypeJS
implementation would be:

function keys(object) {
if (Type(object) !== OBJECT_TYPE) { throw new TypeError(); }
var results = [];
for (var property in object) {
if (object.prototype.hasOwnProperty.call(object, property)) {
results.push(property);
}
}
return results;
}

I've outlined the issue in more detail on my blog:
http://whattheheadsaid.com/2010/10/a-safer-object-keys-compatibility-implementation.

Cheers,

Andy E

[Tobie Langel]

Patch welcomed.

> I've outlined the issue in more detail on my blog:http://whattheheadsaid.com/2010/10/a-safer-object-keys-compatibility-....
>
> Cheers,
>
> Andy E

[Andrew Dupont]

Well, this is the entire change; only unit tests are missing, and I think we can add those ourselves if need be. Thanks, Andy!

I think this is small enough that it can still make it into 1.7 (which we're days away from releasing), but others may disagree.

Cheers,
Andrew

> --
> You received this message because you are subscribed to the Google Groups "Prototype: Core" group.
> To post to this group, send email to prototy...@googlegroups.com
> To unsubscribe from this group, send email to prototype-cor...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/prototype-core?hl=en

[kangax]

On Oct 14, 4:45 pm, Andy E <andyearns...@gmail.com> wrote:
> I recently discovered that several compatibility implementations of
> Object.keys() on the web will throw an error for DOM objects in
> Internet Explorer 8 and lower.  This differs from the current browsers
> having a native implementation and how the ECMAScript 5th edition
> defines the behaviour of Object.keys.
>
> The problem lies with the hasOwnProperty() check, which is not a valid
> method on Internet Explorer DOM objects because they aren't instances
> of Object.  The fix is very simple; "borrow"  hasOwnProperty() from
> Object.prototype.hasOwnProperty() instead.  The fixed PrototypeJS
> implementation would be:
>
>   function keys(object) {
>     if (Type(object) !== OBJECT_TYPE) { throw new TypeError(); }
>     var results = [];
>     for (var property in object) {
>       if (object.prototype.hasOwnProperty.call(object, property)) {
>         results.push(property);
>       }
>     }
>     return results;
>   }

No need to access `hasOwnProperty` off of `Object.prototype` on _every
iteration_, making things unnecessary slow. It's easy enough to alias
`hasOwnProperty` (if it isn't already, somewhere there in the source).

As a side note, this implementation (as well Prototype's original one)
doesn't take care of JScript's DontEnum bug (<IE9, IIRC), which means
`toString`, `valueOf`, etc. -named keys won't be returned. So that's
something else to keep in mind as far as ES5-compatibility goes.

--
kangax

[Andy E]

kangax: I was thinking the same earlier (about caching the
hasOwnProperty), when I realized that my method incorrectly doesn't
allow functions to be passed (not sure about Prototype's original
one). I've updated the function on my blog to:

Object.keys = Object.keys || function (o) {
if (typeof o != "object" && typeof o != "function" || o === null)
throw new TypeError("Object.keys called on a non-object");

var result = [], hop = Object.prototype.hasOwnProperty;
for (var name in o) {
if (hop.call(o, name))
result.push(name);
}
return result;
};

Regarding the DontEnum bug, you could work around that by checking
them explicitly hasOwnProperty outside of the for...in loop.

[kangax]

On Oct 21, 12:11 pm, Andy E <andyearns...@gmail.com> wrote:
> kangax: I was thinking the same earlier (about caching the
> hasOwnProperty), when I realized that my method incorrectly doesn't
> allow functions to be passed (not sure about Prototype's original
> one).  I've updated the function on my blog to:
>
> Object.keys = Object.keys || function (o) {
>     if (typeof o != "object" && typeof o != "function" || o === null)
>         throw new TypeError("Object.keys called on a non-object");
>
>     var result = [], hop = Object.prototype.hasOwnProperty;

That's better, but nothing is stopping from taking it a step further,
aliasing `hasOwnProperty` outside of the method.

>     for (var name in o) {
>         if (hop.call(o, name))
>             result.push(name);
>     }
>     return result;
>
> };
>
> Regarding the DontEnum bug, you could work around that by checking
> them explicitly hasOwnProperty outside of the for...in loop.

Exactly ;)

See for example `Object.forIn()` that I experimented with a couple of
years ago (wow, how time flies...) —
http://github.com/kangax/protolicious/blob/master/experimental/object.for_in.js#L18

Looking at the code now, I would simplify it slightly, but the idea
holds.

[...]

--
kangax

[Andy E]

That's great work. I've found myself on your blog a few times in the
last few weeks, but I hadn't seen this. I hope you don't mind, I used
it to update the example on my blog to:

(function () {
var hasOwnProperty = Object.prototype.hasOwnProperty,
hasDontEnumBug = true,
DontEnums = [
'toString',
'toLocaleString',
'valueOf',
'hasOwnProperty',
'isPrototypeOf',
'propertyIsEnumerable',
'constructor'
],
DontEnumsLength = DontEnums.length;

for (var k in {toString:null})
hasDontEnumBug = false;

Object.keys = Object.keys || function (o) {
if (typeof o != "object" && typeof o != "function" || o ===
null)
throw new TypeError("Object.keys called on a non-object");

var result = [];

for (var name in o) {

if (hasOwnProperty.call(o, name))
result.push(name);
}

if (hasDontEnumBug) {
for (var i = 0; i < DontEnumsLength; i--) {
if (hasOwnProperty.call(o, DontEnums[i]))
result.push(DontEnums[i]);
}
}

return result;
};
})();

You may notice that I sacrificed one of your optimizations for
brevity, I'm not sure the benefits would be noticeable in every day
use. A library like Prototype may benefit from such an optimization,
however.

by the way, I noticed that your implementation will have problems when
walking the DontEnum properties in Internet Explorer - it looks like
you copied and pasted the line of code from the for...in loop to your
for loop but forgot to change the variable that was passed in to the
iterator function.

iterator.call(context || iterator, prop, object[prop]); //
References to `prop` should be `DontEnumProperties[i]`

Or store DontEnumProperties[i] in a variable for a small optimization.

> years ago (wow, how time flies...) —http://github.com/kangax/protolicious/blob/master/experimental/object...

[Andrew Dupont]

This is great stuff. Due to its breadth, however, it will have to wait until 1.7.0.1, for which we've got a bunch of other ES5-compatibility fixes planned. If you and kangax think it would be worth getting the initially-proposed fix into 1.7, then I'll try my best to do so.

Cheers,
Andrew

[kangax]

On Oct 22, 6:41 am, Andy E <andyearns...@gmail.com> wrote:
[...]

> by the way, I noticed that your implementation will have problems when
> walking the DontEnum properties in Internet Explorer - it looks like
> you copied and pasted the line of code from the for...in loop to your
> for loop but forgot to change the variable that was passed in to the
> iterator function.
>
>             iterator.call(context || iterator, prop, object[prop]); //
> References to `prop` should be `DontEnumProperties[i]`
>
> Or store DontEnumProperties[i] in a variable for a small optimization.

Ah, yes. Thanks for catching that. That's lack of unit tests / code
review :)

[..]

--
kangax

[Andy E]

Oops, small error causing infinite loops in IE :-)

for (var i = 0; i < DontEnumsLength; i--) {

Should be

for (var i = 0; i < DontEnumsLength; i++) {

I guess unit tests would have caught that.

Andrew: I have to say, I've not seen any complaints anywhere about the
errors received when using Object.keys. Between kangax and I, the
final solution fixes two bugs. I think if you're close to releasing
1.7, it might not be worth the risk of complicating anything for these
fixes.