TypeError when trying to load angular in PhantomJS

[smashercosmo]

TypeError: '[object NodeList]' is not a valid argument for 'Function.prototype.apply' (evaluating 'push.apply') is caused by the line 2594 of angular source.

this line:

push.apply(root, elements);

this is surely a PhantomJS issue, but we could fix it by changing this line to

push.apply(root, slice.call(elements));

do not know if this change will be acceptable, because this variant is little less performant.

[svemir]

Same problem here - had to revert to beta11 to be able to keep running karma tests.

[caitp]

Before this change (which was mine), we were doing this

angular.js/src/jqLite.js

Lines 346 to 348 in ccfa72d

	for(var i=0; i < elements.length; i++) {
	root.push(elements[i]);
	}

I think slice should still essentially work, and is probably more performant than N calls to push(), at least in the case of pushing multiple nodes.

On the other hand, we know that this works correctly in Safari 6+, so it's not clear that it's worth caring about.

If you can pinpoint which version of webkit fixed this, then it's easier for us to see which browsers are affected, and whether cloning the nodelist is worth it

[caitp]

actually, maybe it wasn't my CL that broke jqLiteAddNodes, hah. lets see

[caitp]

Right, so this is 31faeaa --- I guess given that it's a perf fix, we'd have to be careful about copying the array --- @IgorMinar do you think cloning the nodelist will hurt that much?

[smashercosmo]

Ok, I've just run it in unofficial build of upcoming PhantomJS 2.0 (https://groups.google.com/forum/#!topic/phantomjs/cgTH-jqCSGg/discussion) and it works perfectly. So either we add this Array.slice fix, and then remove it after phantom 2.0 will be released or we just mention somewhere that it is required to use phantom 2.0 build to run karma tests with angular version >= 1.3.0-beta12

[MeghaY]

Had the same issue, so tried with different version/build of Angular. Doesn't happen on 1.3.0-build.2738+sha.f5f1fd1

[aresn]

I made a jspref to compare push.apply vs push for loop vs push.apply and slice
take a look at this yourself. I tried this with various array elements (string, integer, node) and every time I came to a different conclusion. At the end I created a nodeList and used that as my source array.

http://jsperf.com/appending-to-an-array-push-apply-vs-loop/9

[joshvillbrandt]

Same issue here!

[IgorMinar]

@caitp it's the non-array branch that it super hot, so I just threw in slice conversion to deal with phantom. I tested the change manually on it did the trick.

we currently don't run our tests on phantom because it's not a real browser, so fixes like this one are on best effort basis.

[alexgorbatchev]

👍 same issue on angular beta 13

[caitp]

@alexgorbatchev the fix already landed (see above), and will be released in a few days.

[alexgorbatchev]

Great, thank you!

[WEREMSOFT]

Any ETA with this bug? Breaks on IPAD :(

[caitp]

@WEREMSOFT this was fixed already, the next release will be next week --- however this bug was only present in the last release, so you can work around this by simply reverting to a single patch revision earlier

[IgorMinar]

Wasn't this fix in Tuesday's release?
On Jul 4, 2014 10:08 AM, "Caitlin Potter" notifications@github.com wrote:

@WEREMSOFT https://github.com/WEREMSOFT this was fixed already, the
next release will be next week --- however this bug was only present in the
last release, so you can work around this by simply reverting to a single
patch revision earlier

—
Reply to this email directly or view it on GitHub
#7851 (comment).

[caitp]

ceaea86 looks like it was released, I retract my statement about it waiting for next week

[vikasprogrammer]

For some strange reason , I am unable to find this "fixed" code in 1.4.1 (the latest release) and hence getting the same error. Any ideas?