witoldsz / angular-http-auth

MIT License
2.38k stars 417 forks source link

Revert to old array iteration #48

Closed bobey closed 10 years ago

bobey commented 10 years ago

In order to be compatible with old versions of Internet Explorer.

for (var i in ...) doesn't work on IE < 9...

btesser commented 10 years ago

Also the for (var I in X) format is the slowest array iteration you can do. There are some jsperf tests online to back me up On Oct 30, 2013 6:43 AM, "Olivier Balais" notifications@github.com wrote:

In order to be compatible with old versions of Internet Explorer.

for (var i in ...) doesn't work on IE < 9...

You can merge this Pull Request by running

git pull https://github.com/bobey/angular-http-auth master

Or view, comment on, or merge it at:

https://github.com/witoldsz/angular-http-auth/pull/48 Commit Summary

  • Revert to old array iteration

File Changes

  • M src/http-auth-interceptor.jshttps://github.com/witoldsz/angular-http-auth/pull/48/files#diff-0(4)

Patch Links:

witoldsz commented 10 years ago

Are you sure the problem is the for...in statement itself? I am sure it works in IE8. Check this doc: http://msdn.microsoft.com/en-us/library/ie/55wb2d34%28v=vs.94%29.aspx They say is works even in IE6:

Supported in the following document modes: Quirks, Internet Explorer 6 standards, Internet Explorer 7 standards, Internet Explorer 8 standards, Internet Explorer 9 standards, Internet Explorer 10 standards, Internet Explorer 11 standards. Also supported in Windows Store apps.

Also, the verbose version using extra variable for length is really pointless in the situation. It adds extra code for no real or even theoretical reason: how many nanoseconds are you going to save, if any?

bobey commented 10 years ago

I'm pretty sure the problem is the statement itself.

The for...instatement seems to work on objet notation but not on arrays declared like the following:

var a = [1,2,3];

About the verbose version using extra variable, I can simplify it if you prefer...

witoldsz commented 10 years ago

This is strange, I just launched windows to check it out: http://witoldsz.github.io/angular-http-auth/ The so called browser (IE10) was set: Browser Mode: IE8, Document Mode: IE8 standards and the demo app still works. I have never seen a bug like this which was to be observed in real IE 8 and not to be reproduced in compatibility mode of IE9 and IE10.

Can you confirm the demo app does not work in IE8?

bobey commented 10 years ago

No I can't. Indeed, the demo works in IE8.. But in the context of our app, using a for...in statement, we loop over a "forEach" property, etc.

witoldsz commented 10 years ago

To be clear: demo app works, but the module does not work when used in your application?

bobey commented 10 years ago

That's right.

2013/10/31 Witold Szczerba notifications@github.com

To be clear: demo app works, but the module does not work when used in your application?

— Reply to this email directly or view it on GitHubhttps://github.com/witoldsz/angular-http-auth/pull/48#issuecomment-27471226 .

witoldsz commented 10 years ago

This has to be some kind of problem with your app and looks like it has nothing to do with the module. Even AngularJS does use for...in loops in it's sources. If you have IE with broken for...in implementation, AngularJS would fail to operate as well.

bobey commented 10 years ago

I can't see a single usage of the for...in statement in Angular source code on something else than an object... They seems to use exclusively for(var i = 0 ; ... for arrays...

witoldsz commented 10 years ago

I have just launched a virtual machine with Windows XP with IE8 and tested the for...in loop over an array in that thing. It really really does work correctly. Can you provide me an example of broken for...in over an array in IE8?

I will revert the change anyway, it seems to cause useless confusion.

bobey commented 10 years ago

Great thanks!

I'm trying to reproduce this behaviour in a fiddle but without success for now. I'll let you know if I can isolate it.

witoldsz commented 10 years ago

I doubt you will sucessfully reproduce the issue, because, as referenced documentation says, IE6 and newer do handle for...in loops over an array without troubles.

snekbaev commented 10 years ago

Hi,

yesterday I had a problem with IE11 (haven't tried anything else), basically that 'buffer' array was getting some item called "remove" out of... nowhere, so loop was iterating twice and on that "remove" item entire thing was failing because config and deferred were undefined.

I'm no JS expert, but quick a googling revealed that the zen way of iterating items in array is to use plain for loop, not for-each because former will iterate only actual content items and won't accidentally go over properties. I updated both rejectAll and retryAll loops to: for (var i = 0, total = buffer.length; i < total; i++)

error is gone.

Thank you!

witoldsz commented 10 years ago

Internet Explorer doing strange things? Hard to believe... Anyway, the code has been reverted to the proper way of iterating over the array items (I will remember that), a month ago already.

snekbaev commented 10 years ago

Hi,

I haven't tried with other browsers back then, but I was running it with latest Angular 1.2 though, maybe it was a side effect of that, can't say. Then I've changed the current implementation of the filter (adding to responseFilters) to the latest version according to Angular's docs, otherwise another filter wasn't working properly. I think you may want to update it too at some point (there is a fork already) :)

Thank you for the nice module :)