fix(ivy): take components into account when discovering directives - #26113

Now that we know the component is stored first, I'm not sure this loop is necessary anymore. I think to get the directives list in discoverDirectives, we can just copy the existing directives array using the start/end indices:

function discoverDirectives(nodeIndex: number, lViewData: LViewData, includeComponents: boolean = false) {
  const directiveInstances = lViewData[DIRECTIVES];
  if (directiveInstances != null) {
     const tNode = lViewData[TVIEW].data[nodeIndex] as TNode;
     const startIndex = 
         includeComponents ? getDirectiveStartIndex(tNode) : getDirectiveStartIndex(tNode) + 1;
     const endIndex = getDirectiveEndIndex(tNode, startIndex);
     return directiveInstances.slice(startIndex, endIndex);
  }
  return null;
}

Oh, right! This is much simpler indeed, thnx!

OK, so finally it is slightly more complex as we can't simply do const startIndex = includeComponents ? getDirectiveStartIndex(tNode) : getDirectiveStartIndex(tNode) + 1; const endIndex = getDirectiveEndIndex(tNode, startIndex); as we don't know if a given element has a component on it or not.

Anyway, I've decided to change this method to always return components and people can filter them out if not needed.

The default assignment generates some additional code, especially in ES5 targets. Why not simply leave it optional?

👍

changing back to includeComponents?: boolean

Does this now mean that getDirectives will return any components inside no matter what? Can there not be an if statement that checks the first value to see if it's a component instance? There should be a new function here if you want to have a combined list of of the component + directives.

Adding the includeComponents flag (falsy by default) to this method so we can decide if components should be returned or not.

I think {}[] is better than any[] as it is more restrictive. For example array of numbers will not match {} but will match any

OK, changing it back

I think we can avoid the megamorphic lookup in isComponentInstance if we use the isComponent method and pass in the tNode

Changed - didn't know about this util!

Same here. I think we can just check the isComponent flag using the isComponent method. We can do it inside discoverDirectives maybe, since we already have the tNode there?

Hmm, I don't think we've got access to TNode at this point any more...

We can look it up using context.lNodeIndex, see discoverDirectiveIndices. I believe this should still be better than the megamorphic lookup.

@kara see alternative proposal in https://github.com/angular/angular/pull/26113/files#r221942800

If we could store directives start index, no of directives we could easily just slice lViewData here. And we could use TNode's isComponent to adjust slicing start. We could even store a isComponent flag on LContext to avoid TNode lookup all the time.

WDYT?

I'm not sure I understand why we still need to generate an array of indices when we could just slice from the directives array directly to retrieve the instances?

@kara I think I might know where the confusion is coming from - you comment seems to imply that the discoverDirectiveIndices is used only to get directive instances but the way I understood the existing code is that discoverDirectiveIndices and discoverDirectives are 2 independent functions that could, but don't have to be used together.

In fact for the TestBed implementation I don't care about directive instances but need to get directive definitions. For this case the discoverDirectiveIndices comes handy and in this particular case I just don't want to slice LViewData to get directive instances as I don't care about instances.

In the similar spirit I've interpreted discoverDirectives as a utility function that can take any set of indices (not all directives present on a node need to be present in this set) in any particular order. Maybe this wasn't the intention of those 2 functions.

Having said all this I very much dislike the fact that we've got both directive indices and directive instances on LContext as those things can easily get out of sync.

Maybe a better approach would be to store sth like directvesStartIndex and directivesCount and have higher-level utility functions working on those 2 pieces of data? If we store the above:

• we don't have to generate an array of indices here;
• utility methods to get a set of directive instances / directive defs become trivial (array slicing as you suggest).

WDYT?

If you need the defs in TestBed and don't want to look up the TNode, I'd prefer storing the start index/count or the TNode itself (which contains the start index/count) in LContext. Otherwise we are unnecessarily allocating an extra array in memory, as well as looping over the directives array twice for no good reason.

IIRC, the original intent of discoverDirectiveIndices was simply to grab non-component directive indices when we couldn't assume the component would be first in the list. I don't think it's particularly useful externally, so I think we should get rid of it and discoverDirectives/TestBed should use start/end indices to slice what they need directly.

OK, so based on your comment it seems to me like I'm trying to re-purpose the discoverDirectives / discoverDirectiveIndices in the way that is different from the initial / intended design. As such I think that I will just add the needed code to the TestBed impl (I've got access to TNode).

I'm going to close this PR for now. In the future, if there is some code that can be shared / refactored I will pull it into LContext. Thnx for taking time to review and bearing with me!