Iterating over node and edge sets fails if Array.prototype changed

[wimrijnders]

Adding fields to Array.prototype causes for( in ) loops over arrays to fail.

Shoutout to @justinharrell for detecting this.

---

Original text, taken from #3356:

So I took a look again, this is an interesting issue, could not reproduce using the test pages, realized our common app framework is adding to the Array.prototype which causes problems with for in in Clustering.js. I would recommend doing a normal for or maybe forEach (could be slower) on arrays for this issue, for in is iterating properties of the array. See: https://stackoverflow.com/questions/500504/why-is-using-for-in-with-array-iteration-a-bad-idea

To reproduce simple add Array.prototype.foo = 1; before network initialization.

[wimrijnders]

Example: in the basicUsage example, add the following line at the top:

  Array.prototype.foo = 1; 

This directly results in the following error:

vis.js:50569 Uncaught TypeError: Cannot read property 'fromId' of undefined
    at ClusterEngine._updateState (vis.js:50569)
    at Network.<anonymous> (vis.js:41208)
    at Network.Emitter.emit (vis.js:6506)
    at Network.<anonymous> (vis.js:41202)
    at Network.Emitter.emit (vis.js:6506)
    at Network.setData (vis.js:41262)
    at new Network (vis.js:41047)
    at basicUsage.html:72

Looking up the code reference:

     for (n in ids) {            // <===== !!!!!
        var _edgeId = ids[n];
        var _edge6 = this.body.edges[_edgeId];

        var shouldBeClustered = this._isClusteredNode(_edge6.fromId) || this._isClusteredNode(_edge6.toId);
        if (shouldBeClustered === this._isClusteredEdge(_edge6.id)) {
          continue; // all is well
        }
        ...

[wimrijnders]

@justinharrell I enabled eslint rule guard-for-in to catch any for-in loops that might be suspicious. 33 violations within Network. I have a serious case of `how did this ever work?' reflectioning.

Thanks for bringing this to my attention. It's not like adding properties to the Array-prototype is a freakishly singular act. TIL.

[justinharrell]

Not a big fan of JS when in comes to for in, very natural syntax iteration that sorta works on arrays until it doesn't. As a C# dev I am used to just doing a foreach thats the same as a for without the boilerplate, and now Linq.

JS now has forEach which is better but also slower than for. With a library like vis.js where performance matters I would probably stick to for, maybe even doing things like caching length or reversing to eek out a little more perf.

Checkout: https://jsperf.com/for-vs-foreach/37 surprising how bad for in is for perf too.

[wimrijnders]

ruby/C++ here. javascript is just one of those things you have to deal with. The quirks are bountiful and I encounter more the deeper I go. However, I do like this project so I'll deal with it.

for (var i = 0;...;++i) is the way to go, I think, for simple array looping. There will be exceptions, I expect.