Skip to content

Commit 8210f54

Browse files
pkozlowski-opensourcepkozlowski-opensource
committed
fix(ivy): take components into account when discovering directives
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).
1 parent a2878b0 commit 8210f54

File tree

5 files changed

+83
-37
lines changed

5 files changed

+83
-37
lines changed

‎packages/core/src/render3/context_discovery.ts

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {assertEqual} from './assert';
1111
import {LElementNode, TNode, TNodeFlags} from './interfaces/node';
1212
import {RElement} from './interfaces/renderer';
1313
import {CONTEXT, DIRECTIVES, HEADER_OFFSET, LViewData, TVIEW} from './interfaces/view';
14+
import {isComponent} from './util';
1415

1516
/**
1617
* This property will be monkey-patched on elements, components and directives
@@ -107,15 +108,19 @@ export function getContext(target: any): LContext|null {
107108
}
108109
directiveIndices = discoverDirectiveIndices(lViewData, lNodeIndex);
109110
directives = directiveIndices ? discoverDirectives(lViewData, directiveIndices) : null;
111+
const tNode = lViewData[TVIEW].data[lNodeIndex] as TNode;
112+
if (directives && directives.length && isComponent(tNode)) {
113+
component = directives[0];
114+
}
110115
} else {
111116
lNodeIndex = findViaNativeElement(lViewData, target as RElement);
112117
if (lNodeIndex == -1) {
113118
return null;
114119
}
115120
}
116121

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

358363
/**
359-
* Retruns the instance of the LElementNode at the given index in the LViewData.
364+
* Returns the instance of the LElementNode at the given index in the LViewData.
360365
*
361366
* This function will also unwrap the inner value incase it's stuffed into an
362367
* array (which is what happens when [style] and [class] bindings are present
@@ -371,26 +376,14 @@ function getLNodeFromViewData(lViewData: LViewData, lElementIndex: number): LEle
371376
* Returns a collection of directive index values that are used on the element
372377
* (which is referenced by the lNodeIndex)
373378
*/
374-
export function discoverDirectiveIndices(
375-
lViewData: LViewData, lNodeIndex: number, includeComponents?: boolean): number[]|null {
376-
const directivesAcrossView = lViewData[DIRECTIVES];
379+
export function discoverDirectiveIndices(lViewData: LViewData, lNodeIndex: number): number[]|null {
377380
const tNode = lViewData[TVIEW].data[lNodeIndex] as TNode;
378-
if (directivesAcrossView && directivesAcrossView.length) {
379-
// this check for tNode is to determine if the value is a LElementNode instance
380-
const directiveIndexStart = getDirectiveStartIndex(tNode);
381-
const directiveIndexEnd = getDirectiveEndIndex(tNode, directiveIndexStart);
382-
const directiveIndices: number[] = [];
383-
for (let i = directiveIndexStart; i < directiveIndexEnd; i++) {
384-
// special case since the instance of the component (if it exists)
385-
// is stored in the directives array.
386-
if (i > directiveIndexStart ||
387-
!isComponentInstance(directivesAcrossView[directiveIndexStart])) {
388-
directiveIndices.push(i);
389-
}
390-
}
391-
return directiveIndices.length ? directiveIndices : null;
392-
}
393-
return null;
381+
const idxStart = getDirectiveStartIndex(tNode);
382+
const idxEnd = getDirectiveEndIndex(tNode, idxStart);
383+
384+
const result = Array.from({length: idxEnd - idxStart}, (v, k) => idxStart + k);
385+
386+
return result && result.length ? result : null;
394387
}
395388

396389
/**
@@ -401,8 +394,8 @@ export function discoverDirectiveIndices(
401394
* @param indices A collection of directive index values which will be used to
402395
* figure out the directive instances
403396
*/
404-
export function discoverDirectives(lViewData: LViewData, indices: number[]): number[]|null {
405-
const directives: any[] = [];
397+
export function discoverDirectives(lViewData: LViewData, indices: number[]): Array<{}> {
398+
const directives: {}[] = [];
406399
const directiveInstances = lViewData[DIRECTIVES];
407400
if (directiveInstances) {
408401
for (let i = 0; i < indices.length; i++) {

‎packages/core/src/render3/discovery_utils.ts

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,10 +95,11 @@ export function getInjector(target: {}): Injector|null {
9595
}
9696

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

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

111-
return context.directives || [];
112+
const result = context.directives || [];
113+
114+
// discard first directive if it is a component and we should not include components
115+
if (!includeComponents && result.length && isComponentInstance(result[0])) {
116+
return result.slice(1);
117+
}
118+
119+
return result;
112120
}
113121

114122
function loadContext(target: {}): LContext {

‎packages/core/test/bundling/animation_world/bundle.golden_symbols.json

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -660,10 +660,10 @@
660660
"name": "getRendererFactory"
661661
},
662662
{
663-
"name": "getRootContext$1"
663+
"name": "getRootContext"
664664
},
665665
{
666-
"name": "getRootView$1"
666+
"name": "getRootView"
667667
},
668668
{
669669
"name": "getStyleSanitizer"
@@ -725,6 +725,9 @@
725725
{
726726
"name": "isClassBased"
727727
},
728+
{
729+
"name": "isComponent"
730+
},
728731
{
729732
"name": "isComponentInstance"
730733
},

‎packages/core/test/render3/discovery_utils_spec.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,48 @@ describe('discovery utils', () => {
169169
const elm2Dirs = getDirectives(elm2);
170170
expect(elm2Dirs).toContain(myDir3Instance !);
171171
});
172+
173+
it('should return a component as part of a directives list when asked for it', () => {
174+
class MyDir {
175+
static ngDirectiveDef = defineDirective(
176+
{type: MyDir, selectors: [['', 'my-dir', '']], factory: () => new MyDir()});
177+
}
178+
179+
class MyCmpt {
180+
static ngComponentDef = defineComponent({
181+
type: MyCmpt,
182+
selectors: [['my-cmpt']],
183+
factory: () => new MyCmpt(),
184+
consts: 0,
185+
vars: 0,
186+
template: (rf: RenderFlags, ctx: MyCmpt) => {}
187+
});
188+
}
189+
190+
class TestComp {
191+
static ngComponentDef = defineComponent({
192+
type: TestComp,
193+
selectors: [['comp']],
194+
factory: () => new TestComp(),
195+
consts: 1,
196+
vars: 0,
197+
template: (rf: RenderFlags, ctx: TestComp) => {
198+
if (rf & RenderFlags.Create) {
199+
element(0, 'my-cmpt', ['my-dir']);
200+
}
201+
},
202+
directives: [MyCmpt, MyDir]
203+
});
204+
}
205+
206+
const fixture = new ComponentFixture(TestComp);
207+
fixture.update();
208+
209+
const myCmptEl = fixture.hostElement.querySelector('my-cmpt');
210+
211+
expect(getDirectives(myCmptEl !, false).length).toBe(1);
212+
expect(getDirectives(myCmptEl !, true).length).toBe(2);
213+
});
172214
});
173215

174216
describe('getHostComponent()', () => {

‎packages/core/test/render3/integration_spec.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2219,22 +2219,22 @@ describe('render3 integration test', () => {
22192219
assertMonkeyPatchValueIsLViewData(childComponentInstance);
22202220

22212221
expect(getContext(myDir1Instance)).toBe(childNodeContext);
2222-
expect(childNodeContext.component).toBeFalsy();
2223-
expect(childNodeContext.directives !.length).toEqual(2);
2222+
expect(childNodeContext.component).toBeTruthy();
2223+
expect(childNodeContext.directives !.length).toEqual(3);
22242224
assertMonkeyPatchValueIsLViewData(myDir1Instance, false);
22252225
assertMonkeyPatchValueIsLViewData(myDir2Instance, false);
2226-
assertMonkeyPatchValueIsLViewData(childComponentInstance);
2226+
assertMonkeyPatchValueIsLViewData(childComponentInstance, false);
22272227

22282228
expect(getContext(myDir2Instance)).toBe(childNodeContext);
2229-
expect(childNodeContext.component).toBeFalsy();
2230-
expect(childNodeContext.directives !.length).toEqual(2);
2229+
expect(childNodeContext.component).toBeTruthy();
2230+
expect(childNodeContext.directives !.length).toEqual(3);
22312231
assertMonkeyPatchValueIsLViewData(myDir1Instance, false);
22322232
assertMonkeyPatchValueIsLViewData(myDir2Instance, false);
2233-
assertMonkeyPatchValueIsLViewData(childComponentInstance);
2233+
assertMonkeyPatchValueIsLViewData(childComponentInstance, false);
22342234

22352235
expect(getContext(childComponentInstance)).toBe(childNodeContext);
22362236
expect(childNodeContext.component).toBeTruthy();
2237-
expect(childNodeContext.directives !.length).toEqual(2);
2237+
expect(childNodeContext.directives !.length).toEqual(3);
22382238
assertMonkeyPatchValueIsLViewData(myDir1Instance, false);
22392239
assertMonkeyPatchValueIsLViewData(myDir2Instance, false);
22402240
assertMonkeyPatchValueIsLViewData(childComponentInstance, false);

0 commit comments

Comments
 (0)