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

Projects
None yet
3 participants
@SergioCrisostomo
Member

SergioCrisostomo commented Feb 1, 2015

fixes #2613

@megawac

This comment has been minimized.

Show comment
Hide comment
@megawac

megawac 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

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

This comment has been minimized.

Show comment
Hide comment
@SergioCrisostomo

SergioCrisostomo Feb 1, 2015

Member

@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)

Member

SergioCrisostomo commented Feb 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@megawac

megawac 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

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

This comment has been minimized.

Show comment
Hide comment
@SergioCrisostomo

SergioCrisostomo Feb 1, 2015

Member

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

Member

SergioCrisostomo commented Feb 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@megawac

megawac 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

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

This comment has been minimized.

Show comment
Hide comment
@arian

arian Feb 2, 2015

Member

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.

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.

Show outdated Hide outdated Source/Core/Core.js
var keys = [];
eachKey(object, function(key, value){
if (hasOwnProperty.call(object, key)) keys.push(key);
}, this);

This comment has been minimized.

@arian

arian Feb 4, 2015

Member

You don't need this this here..

@arian

arian Feb 4, 2015

Member

You don't need this this here..

},
forEach: function(object, fn, bind){
Object.keys(object).forEach(function(key){

This comment has been minimized.

@arian

arian Feb 4, 2015

Member

Array.forEach is not defined at this point yet.. (but only a few lines after this). Maybe do the Object.* methods after the Array ones?

@arian

arian Feb 4, 2015

Member

Array.forEach is not defined at this point yet.. (but only a few lines after this). Maybe do the Object.* methods after the Array ones?

This comment has been minimized.

@arian

arian Feb 5, 2015

Member

Actually, you can use eachKey here directly:

forEach: eachKey
@arian

arian Feb 5, 2015

Member

Actually, you can use eachKey here directly:

forEach: eachKey
Show outdated Hide outdated Source/Types/Object.js
if (hasOwnProperty.call(object, key)) values.push(object[key]);
}
return values;
return Object.keys(object).map(function(key){

This comment has been minimized.

@arian

arian Feb 4, 2015

Member

Here you depend on one of the Array methods, which isn't required in the yaml. This code is definitely nicer, but not sure if we want to require Array for this, or just do the for-loop.

@arian

arian Feb 4, 2015

Member

Here you depend on one of the Array methods, which isn't required in the yaml. This code is definitely nicer, but not sure if we want to require Array for this, or just do the for-loop.

Show outdated Hide outdated Specs/Core/Core.js
@@ -790,6 +811,32 @@ describe('Object.each', function(){
expect(daysObj).toEqual({first: 'Sunday', second: 'Monday', third: 'Tuesday'});
});
/*<ltIE8>*/

This comment has been minimized.

@arian

arian Feb 4, 2015

Member

This is not just for lt IE8.

@arian

arian Feb 4, 2015

Member

This is not just for lt IE8.

@SergioCrisostomo

This comment has been minimized.

Show comment
Hide comment
@SergioCrisostomo

SergioCrisostomo Feb 4, 2015

Member

@arian updated again.

Member

SergioCrisostomo commented Feb 4, 2015

@arian updated again.

Show outdated Hide outdated Source/Core/Core.js
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){

This comment has been minimized.

@arian

arian Feb 5, 2015

Member

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: // ...
});
@arian

arian Feb 5, 2015

Member

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: // ...
});
Show outdated Hide outdated Source/Core/Core.js
var keys = [];
for (var k in object){
if (alsoPrototypeChain) keys.push(k);
else hasOwnProperty.call(object, k) && keys.push(k);

This comment has been minimized.

@arian

arian Feb 7, 2015

Member

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

@arian

arian Feb 7, 2015

Member

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

@SergioCrisostomo

This comment has been minimized.

Show comment
Hide comment
@SergioCrisostomo

SergioCrisostomo Feb 8, 2015

Member

added commit and just rebased.

Member

SergioCrisostomo commented Feb 8, 2015

added commit and just rebased.

SergioCrisostomo added a commit that referenced this pull request Feb 9, 2015

Merge pull request #2696 from SergioCrisostomo/fix-2613
iterate non-enumerables in old IE also

@SergioCrisostomo SergioCrisostomo merged commit b6fdbb9 into mootools:master Feb 9, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@SergioCrisostomo SergioCrisostomo deleted the SergioCrisostomo:fix-2613 branch Feb 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment