Angular.js: Potential bug in recursive linking

[cary-smith]

Hello angular folks! Really enjoy the library and I am glad you maintain the older one even though the new trucks onwards.

We have recently been experimenting with ShadyDOM as we start preparing for the next iterations of our internal component library. We have run into an issue where the introduction of ShadyDOM to an angular application seems to break ng-switch statements but not all the time and is related to shadyDOM modifying functionality on the browser. In some instances, their code adds an 'item' function to the childNodes array object (not into the array so it does not affect length) which seems to throw things off.

We have a ticket logged with ShadyDOM with a simplified example of this.

I did a bit of digging and found it is failing when angularjs attempts to access the childNodes of an undefined object. Naively the fix is:

if (childLinkFn && linkNode) {
  childLinkFn(scopeToChild, linkNode.childNodes, undefined, boundTranscludeFn);
}

This fixes the failure allowing ng-switch to function in the limited scenario but I know there is far more to it than that.

I am unfortunately unable to submit a PR or verify the testing because we sit behind a firewall and do not use bower. Are there deeper ramifications to placing this fix? Seems that a non-node should not be compiled.

Thanks!

[Narretz]

I think I've seen this before in other circumstances, but so far we've never had a good repro for it. Obviously third party code can be tricky to work with, especially if it manipulates the DOM additionally to AngularJs.

[daniarora17]

I am also seeing the same issue. I can see error "Cannot read property "0" of undefined". Is it under progress
Is it specific to the use of ng-switch
can anyone please share version in which things are working fine ?

[Narretz]

Is it under progress

What do you mean? If we're working on it? No, because we don't have a reproduction.

[cary-smith]

I apologize that I cannot create an example without adding the external library but the ShadyDOM ticket does have a reproduction of it:

http://jsbin.com/cicujedimi/1/edit?html,js,output

[frederikprijck]

@cary-smith It looks like the jsbin is working fine for me, any chance you can link a jsbin which throws the error you talk about?

Edit: Adding ShadyDOM = {force: true} in your jsbin (prior to loading the shadydom js) seems to introduce the error you're talking about. See: http://jsbin.com/xizovucule/2/edit?html,js,output

Seems that a non-node should not be compiled.

I'm wondering why the non-node turns up after including ShadyDom in the first place, any idea why that might be happening ? (I'm not familiar with ShadyDom)

Fwiw, I did some first debugging and it looks like, when ShadyDom is enabled, undefined is added to the stableNodeList here: https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L2060 which eventually leads to passing undefined as a linkNode. It looks like undefined is getting added to the stableNodeList because nodeList[idx] tries to read the element with index 1 which doesn't exist. I'm not very familiar with this part of $compile, maybe @gkalpak or @Narretz can share some insights based on this ?

Naively the fix is:

Imho a less (but still) naive fix could be to avoid adding undefined to the array. But I'm still curious as to why it's getting added in the first place.

[cary-smith]

Sorry for my lack of clarity. We initially noticed the issue in IE11 which would require ShadyDOM to apply the polyfill for our shadow elements to function as expected. I just foolishly left that information out. Luckily you were able to get past the ineptitude. Thanks for adding the clarification.

[Narretz]

I haven't investigated closely, but maybe this is because we assume / require that the DOM does not change during linking. Checking if an element actually exists in the nodeList would introduce a performance penalty.

[gkalpak]

TBH, it ShadyDOM seems the most likely suspect. If I had to guess, I would say it is because ShadyDOM monkey-patches .childNodes and returns a plain ol' Array object, instead of a NodeList (which is "live"). This means that when an element is removed .childNodes should be updated, but ShadyDOM breaks.
_{(While the above is true, I can't be 100% sure that this is what causes the error, but seems very likely.)}

E.g.:

const div = document.createElement('div');
div.innerHTML = '1<span>2</span>3';

const childNodes = div.childNodes;
div.removeChild(childNodes[0]);

console.log(childNodes);
  // Without ShadyDOM: ['<span>2</span>', '3']   (correct)
  // With ShadyDOM: [1, '<span>2</span>', '3']   (incorrect)

#

I understand tht ShadyDOM sacrifices correctness/completeness for speed, but I am not sure if this falls under that umbrella. In any case, I don't think there is much we can do in AngularJS.