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

pkozlowski-opensource · 2018-09-26T10:31:02Z

This PR fixes an issue where the includeComponents argument to the
discoverDirectiveIndices was not taken into account.

Additionally it returns components as part of directives list when
calling getDirectives (please not that we can't parametrize this
function with includeComponents as results are cached).

mary-poppins · 2018-09-26T10:39:57Z

You can preview 23a9b33 at https://pr26113-23a9b33.ngbuilds.io/.

mary-poppins · 2018-09-26T10:42:17Z

You can preview 83e6567 at https://pr26113-83e6567.ngbuilds.io/.

mary-poppins · 2018-09-26T10:44:10Z

You can preview 5be12f8 at https://pr26113-5be12f8.ngbuilds.io/.

kara · 2018-09-26T16:57:54Z

packages/core/src/render3/context_discovery.ts

@@ -353,21 +353,22 @@ function getLNodeFromViewData(lViewData: LViewData, lElementIndex: number): LEle
 * (which is referenced by the lNodeIndex)
 */
 export function discoverDirectiveIndices(
-    lViewData: LViewData, lNodeIndex: number, includeComponents?: boolean): number[]|null {
+    lViewData: LViewData, lNodeIndex: number, includeComponents: boolean = false): number[]|null {
  const directivesAcrossView = lViewData[DIRECTIVES];
  const tNode = lViewData[TVIEW].data[lNodeIndex] as TNode;
  if (directivesAcrossView && directivesAcrossView.length) {


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.

JoostK · 2018-09-26T20:06:06Z

packages/core/src/render3/context_discovery.ts

@@ -353,21 +353,22 @@ function getLNodeFromViewData(lViewData: LViewData, lElementIndex: number): LEle
 * (which is referenced by the lNodeIndex)
 */
 export function discoverDirectiveIndices(
-    lViewData: LViewData, lNodeIndex: number, includeComponents?: boolean): number[]|null {
+    lViewData: LViewData, lNodeIndex: number, includeComponents: boolean = false): number[]|null {


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

changing back to includeComponents?: boolean

matsko · 2018-09-27T02:14:37Z

packages/core/src/render3/discovery_utils.ts

  const context = loadContext(target) !;

  if (context.directives === undefined) {
-    context.directiveIndices = discoverDirectiveIndices(context.lViewData, context.lNodeIndex);
+    context.directiveIndices =
+        discoverDirectiveIndices(context.lViewData, context.lNodeIndex, true);


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.

mhevery

Please take care of the comments. LGTM otherwise.

mhevery · 2018-09-27T04:35:57Z

packages/core/src/render3/discovery_utils.ts

@@ -98,11 +98,12 @@ export function getInjector(target: {}): Injector|null {
 * Returns a list of all the directives that are associated
 * with the underlying target element.
 */
-export function getDirectives(target: {}): Array<{}> {
+export function getDirectives(target: {}): any[] {


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

mary-poppins · 2018-09-27T12:54:21Z

You can preview 1d68e58 at https://pr26113-1d68e58.ngbuilds.io/.

pkozlowski-opensource · 2018-09-27T15:41:20Z

Marking as merge-assistance since:

it requires g3 pre-submit
test_io_preview is failing due to the broken master (see ci(docs-infra): run the script in the correct folder #26136)

kara · 2018-09-27T16:01:31Z

packages/core/src/render3/context_discovery.ts

@@ -89,15 +89,18 @@ export function getContext(target: any): LContext|null {
        }
        directiveIndices = discoverDirectiveIndices(lViewData, lNodeIndex);
        directives = directiveIndices ? discoverDirectives(lViewData, directiveIndices) : null;
+        if (directives && directives.length && isComponentInstance(directives[0])) {


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!

kara · 2018-09-27T16:03:26Z

packages/core/src/render3/discovery_utils.ts

+  const result = context.directives || [];
+
+  // discard first directive if it is a component and we should not include components
+  if (!includeComponents && result.length && isComponentInstance(result[0])) {


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?

mary-poppins · 2018-09-27T16:26:21Z

You can preview 04ed6af at https://pr26113-04ed6af.ngbuilds.io/.

kara · 2018-09-27T16:35:32Z

packages/core/src/render3/context_discovery.ts

+
+  const result = Array.from({length: idxEnd - idxStart}, (v, k) => idxStart + k);
+
+  return result && result.length ? result : null;


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!

pkozlowski-opensource · 2018-10-02T13:16:18Z

@kara I've got an impression that we think differently about role of the discoverDirectiveIndices / discoverDirectives functions, see my comment here: #26113 (comment)

I think that we can simplify the whole system, there is a proposal in the above comment. Let's discuss this in person later today.

mary-poppins · 2018-10-02T13:26:11Z

You can preview 2d71f63 at https://pr26113-2d71f63.ngbuilds.io/.

This PR fixes an issue where the includeComponents argument to the discoverDirectiveIndices was not taken into account. Additionally it returns components as part of directives list when calling getDirectives (please not that we can't parametrize this function with includeComponents as results are cached).

mary-poppins · 2018-10-02T13:59:51Z

You can preview 8210f54 at https://pr26113-8210f54.ngbuilds.io/.

pkozlowski-opensource · 2018-10-02T17:31:18Z

Closing based on #26113 (comment)

angular-automatic-lock-bot · 2019-09-14T02:51:11Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Sep 26, 2018

pkozlowski-opensource force-pushed the discovery_util_fixes_featqures branch 2 times, most recently from 83e6567 to 5be12f8 Compare September 26, 2018 10:36

pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Sep 26, 2018

kara reviewed Sep 26, 2018

View reviewed changes

JoostK reviewed Sep 26, 2018

View reviewed changes

matsko reviewed Sep 27, 2018

View reviewed changes

mhevery approved these changes Sep 27, 2018

View reviewed changes

mhevery added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 27, 2018

pkozlowski-opensource force-pushed the discovery_util_fixes_featqures branch from 5be12f8 to 1d68e58 Compare September 27, 2018 12:46

pkozlowski-opensource removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 27, 2018

pkozlowski-opensource added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Sep 27, 2018

matsko approved these changes Sep 27, 2018

View reviewed changes

kara reviewed Sep 27, 2018

View reviewed changes

pkozlowski-opensource force-pushed the discovery_util_fixes_featqures branch from 1d68e58 to 04ed6af Compare September 27, 2018 16:18

kara reviewed Sep 27, 2018

View reviewed changes

kara added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 27, 2018

pkozlowski-opensource removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 2, 2018

pkozlowski-opensource force-pushed the discovery_util_fixes_featqures branch from 04ed6af to 2d71f63 Compare October 2, 2018 13:17

pkozlowski-opensource force-pushed the discovery_util_fixes_featqures branch from 2d71f63 to 8210f54 Compare October 2, 2018 13:52

pkozlowski-opensource mentioned this pull request Oct 2, 2018

Test bed with context discovery #26211

Closed

pkozlowski-opensource closed this Oct 2, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019


		const result = Array.from({length: idxEnd - idxStart}, (v, k) => idxStart + k);

		return result && result.length ? result : null;

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

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

Uh oh!

Conversation

pkozlowski-opensource commented Sep 26, 2018

Uh oh!

mary-poppins commented Sep 26, 2018

Uh oh!

mary-poppins commented Sep 26, 2018

Uh oh!

mary-poppins commented Sep 26, 2018

Uh oh!

kara Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matsko Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Sep 27, 2018

Uh oh!

pkozlowski-opensource commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kara Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kara Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Sep 27, 2018

Uh oh!

kara Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kara Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkozlowski-opensource commented Oct 2, 2018

Uh oh!

mary-poppins commented Oct 2, 2018

Uh oh!

mary-poppins commented Oct 2, 2018

Uh oh!

pkozlowski-opensource commented Oct 2, 2018

kara Sep 26, 2018 •

edited

Loading

matsko Sep 27, 2018 •

edited

Loading

kara Sep 27, 2018 •

edited

Loading

kara Sep 27, 2018 •

edited

Loading

kara Sep 27, 2018 •

edited

Loading

kara Oct 2, 2018 •

edited

Loading