Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pkozlowski-opensource wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 17 additions & 24 deletions packages/core/src/render3/context_discovery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {assertEqual} from './assert';
import {LElementNode, TNode, TNodeFlags} from './interfaces/node';
import {RElement} from './interfaces/renderer';
import {CONTEXT, DIRECTIVES, HEADER_OFFSET, LViewData, TVIEW} from './interfaces/view';
import {isComponent} from './util';

/**
* This property will be monkey-patched on elements, components and directives
Expand Down Expand Up @@ -107,15 +108,19 @@ export function getContext(target: any): LContext|null {
}
directiveIndices = discoverDirectiveIndices(lViewData, lNodeIndex);
directives = directiveIndices ? discoverDirectives(lViewData, directiveIndices) : null;
const tNode = lViewData[TVIEW].data[lNodeIndex] as TNode;
if (directives && directives.length && isComponent(tNode)) {
component = directives[0];
}
} else {
lNodeIndex = findViaNativeElement(lViewData, target as RElement);
if (lNodeIndex == -1) {
return null;
}
}

// the goal is not to fill the entire context full of data because the lookups
// are expensive. Instead, only the target data (the element, compontent or
// The goal is not to fill the entire context full of data because the lookups
// are expensive. Instead, only the target data (the element, component or
// directive details) are filled into the context. If called multiple times
// with different target values then the missing target data will be filled in.
const lNode = getLNodeFromViewData(lViewData, lNodeIndex) !;
Expand Down Expand Up @@ -356,7 +361,7 @@ function assertDomElement(element: any) {
}

/**
* Retruns the instance of the LElementNode at the given index in the LViewData.
* Returns the instance of the LElementNode at the given index in the LViewData.
*
* This function will also unwrap the inner value incase it's stuffed into an
* array (which is what happens when [style] and [class] bindings are present
Expand All @@ -371,26 +376,14 @@ function getLNodeFromViewData(lViewData: LViewData, lElementIndex: number): LEle
* Returns a collection of directive index values that are used on the element
* (which is referenced by the lNodeIndex)
*/
export function discoverDirectiveIndices(
lViewData: LViewData, lNodeIndex: number, includeComponents?: boolean): number[]|null {
const directivesAcrossView = lViewData[DIRECTIVES];
export function discoverDirectiveIndices(lViewData: LViewData, lNodeIndex: number): number[]|null {
const tNode = lViewData[TVIEW].data[lNodeIndex] as TNode;
if (directivesAcrossView && directivesAcrossView.length) {
// this check for tNode is to determine if the value is a LElementNode instance
const directiveIndexStart = getDirectiveStartIndex(tNode);
const directiveIndexEnd = getDirectiveEndIndex(tNode, directiveIndexStart);
const directiveIndices: number[] = [];
for (let i = directiveIndexStart; i < directiveIndexEnd; i++) {
// special case since the instance of the component (if it exists)
// is stored in the directives array.
if (i > directiveIndexStart ||
!isComponentInstance(directivesAcrossView[directiveIndexStart])) {
directiveIndices.push(i);
}
}
return directiveIndices.length ? directiveIndices : null;
}
return null;
const idxStart = getDirectiveStartIndex(tNode);
const idxEnd = getDirectiveEndIndex(tNode, idxStart);

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

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

@kara kara Sep 27, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@pkozlowski-opensource pkozlowski-opensource Oct 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@kara kara Oct 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pkozlowski-opensource pkozlowski-opensource Oct 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

/**
Expand All @@ -401,8 +394,8 @@ export function discoverDirectiveIndices(
* @param indices A collection of directive index values which will be used to
* figure out the directive instances
*/
export function discoverDirectives(lViewData: LViewData, indices: number[]): number[]|null {
const directives: any[] = [];
export function discoverDirectives(lViewData: LViewData, indices: number[]): Array<{}> {
const directives: {}[] = [];
const directiveInstances = lViewData[DIRECTIVES];
if (directiveInstances) {
for (let i = 0; i < indices.length; i++) {
Expand Down
16 changes: 12 additions & 4 deletions packages/core/src/render3/discovery_utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,11 @@ export function getInjector(target: {}): Injector|null {
}

/**
* Returns a list of all the directives that are associated
* with the underlying target element.
* Returns a list of all the directives that are associated with the underlying target element.
* Accepts an optional argument to indicate if component instances should be returned as well (false
* by default).
*/
export function getDirectives(target: {}): Array<{}> {
export function getDirectives(target: {}, includeComponents?: boolean): Array<{}> {
const context = loadContext(target) !;

if (context.directives === undefined) {
Expand All @@ -108,7 +109,14 @@ export function getDirectives(target: {}): Array<{}> {
null;
}

return context.directives || [];
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])) {

@kara kara Sep 27, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@pkozlowski-opensource pkozlowski-opensource Sep 27, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kara kara Sep 27, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pkozlowski-opensource pkozlowski-opensource Oct 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

return result.slice(1);
}

return result;
}

function loadContext(target: {}): LContext {
Expand Down
7 changes: 5 additions & 2 deletions packages/core/test/bundling/animation_world/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,10 +660,10 @@
"name": "getRendererFactory"
},
{
"name": "getRootContext$1"
"name": "getRootContext"
},
{
"name": "getRootView$1"
"name": "getRootView"
},
{
"name": "getStyleSanitizer"
Expand Down Expand Up @@ -725,6 +725,9 @@
{
"name": "isClassBased"
},
{
"name": "isComponent"
},
{
"name": "isComponentInstance"
},
Expand Down
42 changes: 42 additions & 0 deletions packages/core/test/render3/discovery_utils_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,48 @@ describe('discovery utils', () => {
const elm2Dirs = getDirectives(elm2);
expect(elm2Dirs).toContain(myDir3Instance !);
});

it('should return a component as part of a directives list when asked for it', () => {
class MyDir {
static ngDirectiveDef = defineDirective(
{type: MyDir, selectors: [['', 'my-dir', '']], factory: () => new MyDir()});
}

class MyCmpt {
static ngComponentDef = defineComponent({
type: MyCmpt,
selectors: [['my-cmpt']],
factory: () => new MyCmpt(),
consts: 0,
vars: 0,
template: (rf: RenderFlags, ctx: MyCmpt) => {}
});
}

class TestComp {
static ngComponentDef = defineComponent({
type: TestComp,
selectors: [['comp']],
factory: () => new TestComp(),
consts: 1,
vars: 0,
template: (rf: RenderFlags, ctx: TestComp) => {
if (rf & RenderFlags.Create) {
element(0, 'my-cmpt', ['my-dir']);
}
},
directives: [MyCmpt, MyDir]
});
}

const fixture = new ComponentFixture(TestComp);
fixture.update();

const myCmptEl = fixture.hostElement.querySelector('my-cmpt');

expect(getDirectives(myCmptEl !, false).length).toBe(1);
expect(getDirectives(myCmptEl !, true).length).toBe(2);
});
});

describe('getHostComponent()', () => {
Expand Down
14 changes: 7 additions & 7 deletions packages/core/test/render3/integration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2219,22 +2219,22 @@ describe('render3 integration test', () => {
assertMonkeyPatchValueIsLViewData(childComponentInstance);

expect(getContext(myDir1Instance)).toBe(childNodeContext);
expect(childNodeContext.component).toBeFalsy();
expect(childNodeContext.directives !.length).toEqual(2);
expect(childNodeContext.component).toBeTruthy();
expect(childNodeContext.directives !.length).toEqual(3);
assertMonkeyPatchValueIsLViewData(myDir1Instance, false);
assertMonkeyPatchValueIsLViewData(myDir2Instance, false);
assertMonkeyPatchValueIsLViewData(childComponentInstance);
assertMonkeyPatchValueIsLViewData(childComponentInstance, false);

expect(getContext(myDir2Instance)).toBe(childNodeContext);
expect(childNodeContext.component).toBeFalsy();
expect(childNodeContext.directives !.length).toEqual(2);
expect(childNodeContext.component).toBeTruthy();
expect(childNodeContext.directives !.length).toEqual(3);
assertMonkeyPatchValueIsLViewData(myDir1Instance, false);
assertMonkeyPatchValueIsLViewData(myDir2Instance, false);
assertMonkeyPatchValueIsLViewData(childComponentInstance);
assertMonkeyPatchValueIsLViewData(childComponentInstance, false);

expect(getContext(childComponentInstance)).toBe(childNodeContext);
expect(childNodeContext.component).toBeTruthy();
expect(childNodeContext.directives !.length).toEqual(2);
expect(childNodeContext.directives !.length).toEqual(3);
assertMonkeyPatchValueIsLViewData(myDir1Instance, false);
assertMonkeyPatchValueIsLViewData(myDir2Instance, false);
assertMonkeyPatchValueIsLViewData(childComponentInstance, false);
Expand Down