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

Revert to old array iteration #48

Closed
wants to merge 1 commit into from
Closed

Conversation

bobey
Copy link

@bobey bobey commented Oct 30, 2013

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

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

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

```for (var i in ...)``` doesn't work on IE < 9...
@btesser
Copy link

btesser commented Oct 30, 2013

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" [email protected] 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:

#48
Commit Summary

  • Revert to old array iteration

File Changes

Patch Links:

@witoldsz
Copy link
Owner

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
Copy link
Author

bobey commented Oct 30, 2013

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
Copy link
Owner

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
Copy link
Author

bobey commented Oct 31, 2013

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
Copy link
Owner

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

@bobey
Copy link
Author

bobey commented Oct 31, 2013

That's right.

2013/10/31 Witold Szczerba [email protected]

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//pull/48#issuecomment-27471226
.

@witoldsz
Copy link
Owner

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.

@witoldsz witoldsz closed this Oct 31, 2013
@bobey
Copy link
Author

bobey commented Oct 31, 2013

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
Copy link
Owner

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.

witoldsz pushed a commit that referenced this pull request Oct 31, 2013
witoldsz added a commit that referenced this pull request Oct 31, 2013
@bobey
Copy link
Author

bobey commented Oct 31, 2013

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
Copy link
Owner

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
Copy link

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
Copy link
Owner

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants