Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iterate non-enumerables in old IE also #2696

Merged
merged 4 commits into from
Feb 9, 2015

Conversation

SergioCrisostomo
Copy link
Member

fixes #2613

@megawac
Copy link

megawac commented Feb 1, 2015

👍 but why can't you guys implement all these methods by way of Object.keys which will also be faster in browsers that support keys natively

@SergioCrisostomo
Copy link
Member Author

@megawac this was the best I came up with. Input is really welcome, to make it even better :)

In this case since its a IE8- fix i think Object.keys is not a option since IE8 doesn't support it.
(do correct me if I understood you wrong)

@megawac
Copy link

megawac commented Feb 1, 2015

I mean Mootools polyfills Object.keys that also has this fix no?

Looks like this fix isn't being applied to keys: https://github.com/mootools/mootools-core/blob/master/Source/Types/Object.js#L63-69

@SergioCrisostomo
Copy link
Member Author

@megawac oh, I see. Actually Object.keys is also depending on a for var in. Maybe it should use a patched Object.each instead...

@megawac
Copy link

megawac commented Feb 1, 2015

@SergioCrisostomo I would say it should be the other way around (thats how most other frameworks otu there are doing it). Having keys be the root fix for the non enumerable bug in ie also lets other browsers benefit from a faster implementation of all the object methods as native keys is faster than for .. in.

I would argue all the object methods (each, map, filter, etc) that iterate own properties should be using keys, and the enumerable fix where keys doesn';t

@arian
Copy link
Member

arian commented Feb 2, 2015

I think that'd be better indeed.

We have to move Object.keys to Core.js from Types/Object.js. After that we could rewrite all for in occurrences to use Object.keys.

var keys = [];
eachKey(object, function(key, value){
if (hasOwnProperty.call(object, key)) keys.push(key);
}, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this this here..

@SergioCrisostomo
Copy link
Member Author

@arian updated again.

var enumerables = true;
for (var i in {toString: 1}) enumerables = null;
if (enumerables) enumerables = ['hasOwnProperty', 'valueOf', 'isPrototypeOf', 'propertyIsEnumerable', 'toLocaleString', 'toString', 'constructor'];
/*</ltIE8>*/
var hasOwnProperty = Object.prototype.hasOwnProperty;
function eachKey(object, fn, thisArg){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think eachKey isn't a very nice name. maybe objectEach, or just each?

Also I'm thinking if this shouldn't be:

var objectKeys = Object.keys || function(object){
   // ...
   return keys;
};

And use that in the overloadSetter, with a for-loop there.

And below, just have:

Object.extend({
  keys: objectKeys,
  forEach: // ...
});
var keys = [];
for (var k in object){
if (alsoPrototypeChain) keys.push(k);
else hasOwnProperty.call(object, k) && keys.push(k);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seriously, use else if, not the x && y thing!

@SergioCrisostomo
Copy link
Member Author

added commit and just rebased.

SergioCrisostomo added a commit that referenced this pull request Feb 9, 2015
iterate non-enumerables in old IE also
@SergioCrisostomo SergioCrisostomo merged commit b6fdbb9 into mootools:master Feb 9, 2015
@SergioCrisostomo SergioCrisostomo deleted the fix-2613 branch February 9, 2015 02:35
@mootools mootools deleted a comment Feb 3, 2020
@mootools mootools deleted a comment Mar 14, 2020
@mootools mootools deleted a comment Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants