Make auto-tests work

[brianmhunt]

Fail on CircleCI:

https://circleci.com/gh/brianmhunt/knockout-fast-foreach/6?utm_campaign=build-failed&utm_medium=email&utm_source=notification

Note comments at end of https://github.com/brianmhunt/knockout-fast-foreach/issues/12#issuecomment-121437758

[cervengoc]

@brianmhunt Are you sure that this line fails? https://github.com/brianmhunt/knockout-fast-foreach/blob/master/index.js#L236

There is a quite similar one in the code: https://github.com/brianmhunt/knockout-fast-foreach/blob/master/index.js#L176 I guess there is at least one test which coverts that too, doesn't that fail too?

[brianmhunt]

@cervengoc In the latter case (changeQueue) it's an array, but in the prior (allChildNodes) it's a NodeList instance. The extensive differences between the NodeList and an Array are detailed there.

Perhaps Array.prototype.push.apply(allChildNodes, childNodes) might work. Haven't tested it yet, but it feels like it might work.

[brianmhunt]

It looks like Array.prototype.push.apply will not work because the NodeList has an immutable length property in some browsers e.g. Chrome.

Another alternative is to covert the childNodes to an array before insertion with Array.prototype.slice.call(childNodes).

That said, copying from a NodeList to an Array then push onto the list may be slower than a single iteration through a for loop. I think one would have to experiment to determine what's faster (if there's any measurable difference).

[cervengoc]

But I initialize allChildNodes with an empty array literal. Does it turn into a NodeList somewhere implicitly, or do I miss something very basic?

[cervengoc]

Wouldn't a simple for loop and single calls to push solve the problem? I mean instead of the apply technique.

[cervengoc]

Oh I see now, the problem was with childNodes, not with allChildNodes. In your comment you referred to the latter, that's why I was surprised. Still, maybe it could be a bit more performant if you iterated through childNodes and called ˙allChildNodes.push˙ one-by-one.

[brianmhunt]

@cervengoc I also saw that allChildNodes was an empty array; it seems it's the argument: The error PhantomJS spits out is:

Message: '[object NodeList]' is not a valid argument for 'Function.prototype.apply' (evaluating 'allChildNodes.push.apply')
TypeError: '[object NodeList]' is not a valid argument for 'Function.prototype.apply' (evaluating 'allChildNodes.push.apply')
    at /Users/bmh/Repos/knockout-fast-foreach/index.js:236
    at /Users/bmh/Repos/knockout-fast-foreach/index.js:199
    at /Users/bmh/Repos/knockout-fast-foreach/node_modules/knockout/build/output/knockout-latest.js:10
 ...

A for loop is significantly slower than .push.apply, the reason apparently being that push.apply will pre-reserve the space for the insertion.

I've pushed a fix for this with the Array::slice method, but am always open to better alternatives. :)

[brianmhunt]

When jsPerf is back up maybe we can put a test up to see which performs better. :)

[cervengoc]

Ok, I undertand now, thank you for the explanation. I believe you, no need for jsPerf :) Would you please reflect to my notes about the $index problem on the other thread?