fix($compile): Detect when a directive compile removes tElement

[lgalfaso]

Delect when a directive compile function removes tElement and keep on with the compilation from the following node

Closes #9198

[caitp]

I don't really understand this ---- ngCanary is set up for each item above, so how could it ever not be set here? there isn't any user code running in between these places. Am i missing something?

[caitp]

Also, the increment expression doesn't make a lot of sense. This means that we potentially aren't incrementing, no? It's hard to read and the intent is really not super clear, I would suggest changing it

[lgalfaso]

ngCanary cannot be there if:

• The element was replaced by a directive
• We remove it with the delete (or set to undefined)

[lgalfaso]

We have to increment only if nodeList[i] was not processed, in this case, if nodeList[i] has the canary, then this is the node we have to compile

[caitp]

But if the element is replaced by a directive, that would happen either before or after ngCanary was set up (and subsequently deleted).

In the lines above, you're saying "add ngCanary to each node in the node list", and then immediately after, iterating over each node in the node list and deleting the ngCanary attribute.

To me, this makes no sense, the intent is really unclear, and its function is absolutely unclear from the code

[caitp]

The logic can be explained, but it's not expressed well, it's expressed in a completely nonsensical form, which is why we need either inline comments or code which is expressed better.

And additionally, personally I don't think we care if the element is removed during compile, that's a bug in the user's code

[gkalpak]

I agree that the code is not self-explanatory, but maybe only because ngCanary is such a cryptic name for a property.
What if it was changed like this:

for (i = 0; i < nodeList.length; ++i) {
    nodeList[i].ngNotProcessedYet = true;
}

for (i = 0; i < nodeList.length; (!nodeList[i].ngNotProcessedYet) && i++) {
    nodeList[i].ngNotProcessedYet = undefined;
    ...

UPDATE: Changed ngHasBeenProcessed to ngNotProcessedYet and reversed the logic in order to get rid of the extra property once the element has been processed.

[caitp]

The thing that is super unclear is the structure of the loop, like this doesn't make sense. If we're going to do this we should probably change it to a do/while loop instead, and make the conditions for incrementing extremely clear.

I don't think changing the name will matter much, it's going to be unclear no matter what it's called. The more important thing is making its intent and functionality crystal clear with comments

[lgalfaso]

Will update the patch at some time tomorrow with proper comments and clear logic. Anyhow, I will answer a few of the issues raised

Another thing, we seem to be optimizing for a bizarre case where we actually do call tElement.detach() in compile, and it's not clear that there's ever a good reason to do this

Agree, it is not clear if we should even care about this case at all, and that is a discussion that is independent of the solution

Now, if this is something we want to fix then there are two solutions

• Loop thru all the nodes, capture all the directives and then apply them (that is a refactor that is too big for anybody to feel comfortable)
• Mark the nodes so we know the ones that should be processed in this loop

The use of simple boolean values using ngHasBeenProcessed or ngNotProcessedYet is no good. The reason being that we need to process only the nodes that originally came to this loop and not elements that can show up later (think or a directive has replace: true and terminal: true)

[caitp]

I like this comment, lucas.

Anyways, just to clarify some of the things I was saying before, lets look at this diff (with comments inserted showing the WAT):

      for (i = 0; i < nodeList.length; ++i) {
        // Okay, we're looping over everything and adding our canary string (why is this
        // converted to a string? who knows!)
        nodeList[i].ngCanary = canary;
      }

      for (i = 0; i < nodeList.length; (nodeList[i].ngCanary != canary) && i++) {
         attrs = new Attributes();
        // We're now looping over all our nodes again.
        //
        // The very first thing we do in this loop (after creating the Attributes object), is
        // set the canary we literally __just__ set up, to "undefined". WAT.
        nodeList[i].ngCanary = undefined;

So, I understand the logic --- if the node is detached and removed from the nodeList collection, then nodeList[i].ngCanary will still be the canary, and everything is okay (because incrementing doesn't happen). But the problem is that this is just extremely unclear.

What I want is for this logic to be self-explanatory, just by reading it. I want to see inline comments like // If the processed node was not removed from the nodeList collection, then incrementi` to process the next node. I don't want the increment logic to be in the form it's currently in, as the third part of a for-loop, because that just makes the intent and function of it a complete mystery to anyone reading the code.

I hope I'm saying this in a nice and amicable fashion. Ordinarily the smallest diff is the best diff, but I think in this case -- if we do want to support this -- then a more readable refactoring is probably in order.

[lgalfaso]

Hi @caitp, hopefully the new patch makes it clear what is going on

[caitp]

This is better --- We could still shrink this a bit, though, by adding if (i + 1 < nodeList.length) nodeList[i+1].ngCanary = canary;

This would let us remove the initial loop. It doesn't make a huge difference though

[lgalfaso]

If nodeList[i] has a directive with -begin, then just marking the next node would not make it

[IgorMinar]

you remove it before the element is processed and after the element is processed you check if the current node doesn't have canary flag any more. no?

[lgalfaso]

yes