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

for(var x in someArray) caused crashes if Array.prototype modified #7

Conversation

justin808
Copy link

* Issue with for in loops described:  http://stackoverflow.com/a/3010848/1009332
* Changed for in loops to lodash forEach
* Added test showing error
* Chaged all for in loops
@xzyfer
Copy link
Owner

xzyfer commented Feb 2, 2015

Thanks for raising this issue @justin808. I've opted for a simpler patch until I do a complete overhaul in 1.1.0.

@justin808
Copy link
Author

@xzyfer Did you see an issue with my technique for array iteration? The only complexity is that you want to short circuit the loop, and that's handled by lodash by returning false.

@xzyfer
Copy link
Owner

xzyfer commented Feb 2, 2015

My issue was really that it was unnecessary complexity for something JavaScript does natively.

IMHO continue for short-circuiting is more obvious. Also I'm considering dropping the lodash dependency in 1.1.0. So I want to get a real sense of why it's currently being used.

note although I was involved originally I'm not the original author of this module

@justin808
Copy link
Author

@xzyfer I'd take a look at the stack overflow thread on this topic and the referenced article.

I just disagree that on iterating over the object properties for an array. A simple for loop would be better.

http://stackoverflow.com/a/3010848/1009332 and http://web.archive.org/web/20101213150231/http://dhtmlkitchen.com/?category=/JavaScript/&date=2007/10/21/&entry=Iteration-Enumeration-Primitives-and-Objects

The example given is:

Iterate over an Array

An Array is an object that is specialized by its length property, it's [[put]] method, and its literal initializer syntax. Array.prototype is sometimes modified to add features that are not supported in certain browsers (like Array Extras, supported in Webkit and Gecko, but not Opera 9.2 or IE 7).

Because Array.prototype is often modified, it is not safe to enumerate over an Array (unless you have a sparse array and know how hasOwnProperty works).

Non existent properties that need to be added by programmer-defined code do not exist, and therefore, cannot have the DontEnum attribute. For example, Array.prototype.some does not exist on Array.prototype in IE or Opera, but is non-enumerable, native code in other browsers.

To avoid enumerating over a property in the prototype chain, a cautious programmer will avoid using for in on an Array, and use hasOwnProperty when he does use for in.

Library authors might consider adding only the top-level Extras, e.g. Array.some to browsers that don't have Array.some, and leaving the prototype alone.

@xzyfer
Copy link
Owner

xzyfer commented Feb 2, 2015

A simple for loop would be better.

I agree. I misread the code and assumed they were objects because of the for..in.

Given arrays I would opt for a simple for(i = 0; i < arr.length; i++) or even native forEach rather than lean on a library.

I'm aware the native forEach is significantly slower than the lodash implementation.

IMHO basic for loop in this improves readability, and allows for the use for continue as a short-circuit.

@lox
Copy link
Collaborator

lox commented Mar 18, 2015

My bad!

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.

None yet

3 participants