Globally extending Array with indexOf method in old MSIE breaks other code

[hessu]

timezone-js extends the Array prototype with a missing indexOf method in MSIE:

//Handle array indexOf in IE   
if (!Array.prototype.indexOf) {
    Array.prototype.indexOf = function (el) {

Unfortunately that breaks other code - indexOf becomes a key in all Arrays:

    for (var idx in array_of_stuff)
            dostuff(idx);

That code will now call dostuff() with 'indexOf' with IE8 at least. For IE support the indexOf should be avoided, or it somehow needs to be added locally. I'll look into this myself and try to get a patch submitted soon.

[longlho]

Hmm not sure why this is an issue. What kind of operation would you do on every function of the Array object? That same operation will also break on every browser that has indexOf in the Array, which is prob not cross-browser anw.

[hessu]

If you create a sparse array:

var a = [];
a[42] = 'foo';

it's possible to iterate through that using this syntax:

for (var key in a)
    bar(key);

The bar function will get called once with a value of 42 - it will not iterate through all the methods of Array, just the keys present in the associative array. That works just fine unless the Array prototype is extended with additional methods like indexOf, in which case it'll also call bar("indexOf") on MSIE. Please take a look at this example on both IE8 and some less annoying browser which already has indexOf:

http://he.fi/misc/array-indexOf.html

So, it would be easier for some users of the timezone-js library if it would not go extending the Array class and adding keys in it. It's a really nice timezone handling library which is greatly needed, but I would rather prefer to knowingly extend Array myself instead of getting caught pants down when a really nice timezone handling library does it for me :)

I know using object as an associative array instead would work around this, but that's not the point.

[mde]

This is a problem with enhancing the native prototypes. Built-in methods are not enumerable, but these enhancements to the prototype are. I normally object strongly to monkeying with the natives, but I have softened my stance when it comes to polyfills for really old, crappy browsers. At this point, you could argue that vastly more users have indexOf than do not, and this sort of breakage is a strong indication the user should upgrade their browser. After spending years and years with this sort of "enabling" behavior of bending over backward to support old IE, at some point you do have to draw the line -- and I'm fine with drawing it here.

[hessu]

Unfortunately my site still gets over 80k visits per month from IE7 and IE8 clients (according to Google Analytics), and them being real-world end users from all around the internets, forcing them to upgrade is not practical. So, I'll have to keep things working for them to some degree (as much as I hate working around IE7 bugs).

I'll submit a pull request early next year with a little workaround for this, and if it won't be accepted, I'll just maintain my own branch. No hard feelings. :)

[mde]

Oh, wait, indexOf isn't even implemented in IE8? If you can give us a workaround that doesn't break stuff for others, or make things really unmaintainable, we'll definitely merge it in. Thanks!

[hessu]

Yeah. Unfortunately IE9 is the first one to have it, and IE9 isn't available for the XP systems which are going to live longer than any of us would like.