Skip to content

Commit a6d41c4

Browse files
mheveryalexeagle
mhevery
authored and
alexeagle
committed
refactor(ivy): move directive into elementStart (angular#21374)
We used to have a separate `directive` instruction for instantiating directives. However, such an instruction requires that directives are created in the correct order, which would require that template compiler would have knowledge of all dependent directives. This would break template compilation locality principle. This change only changes the APIs to expected form but does not change the semantics. The semantics will need to be corrected in subsequent commits. The semantic change needed is to resolve the directive instantiation error at runtime based on injection dependencies. PR Close angular#21374
1 parent c0080d7 commit a6d41c4

19 files changed

Lines changed: 397 additions & 505 deletions

‎modules/benchmarks/src/tree/render3/tree.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,7 @@ export class TreeComponent {
5656
let cm0 = V(0);
5757
{
5858
if (cm0) {
59-
E(0, TreeComponent.ngComponentDef);
60-
{ D(1, TreeComponent.ngComponentDef.n(), TreeComponent.ngComponentDef); }
59+
E(0, TreeComponent);
6160
e();
6261
}
6362
p(0, 'data', b(ctx.data.left));
@@ -74,8 +73,7 @@ export class TreeComponent {
7473
let cm0 = V(0);
7574
{
7675
if (cm0) {
77-
E(0, TreeComponent.ngComponentDef);
78-
{ D(1, TreeComponent.ngComponentDef.n(), TreeComponent.ngComponentDef); }
76+
E(0, TreeComponent);
7977
e();
8078
}
8179
p(0, 'data', b(ctx.data.right));

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,10 @@
2121
* @returns The stringified value
2222
*/
2323
function stringifyValueForError(value: any): string {
24-
return typeof value === 'string' ? `"${value}"` : '' + value;
24+
if (value && value.native && value.native.outerHTML) {
25+
return value.native.outerHTML;
26+
}
27+
return typeof value === 'string' ? `"${value}"` : value;
2528
}
2629

2730
export function assertNumber(actual: any, name: string) {
@@ -57,6 +60,8 @@ export function assertNotEqual<T>(actual: T, expected: T, name: string) {
5760
export function assertThrow<T>(
5861
actual: T, expected: T, name: string, operator: string,
5962
serializer: ((v: T) => string) = stringifyValueForError): never {
60-
throw new Error(
61-
`ASSERT: expected ${name} ${operator} ${serializer(expected)} but was ${serializer(actual)}!`);
63+
const error =
64+
`ASSERT: expected ${name} ${operator} ${serializer(expected)} but was ${serializer(actual)}!`;
65+
debugger; // leave `debugger` here to aid in debugging.
66+
throw new Error(error);
6267
}

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,13 @@ import {ComponentRef as viewEngine_ComponentRef} from '../linker/component_facto
1313
import {EmbeddedViewRef as viewEngine_EmbeddedViewRef} from '../linker/view_ref';
1414

1515
import {assertNotNull} from './assert';
16-
import {NG_HOST_SYMBOL, createError, createLView, directive, enterView, hostElement, leaveView, locateHostElement, renderComponentOrTemplate} from './instructions';
16+
import {NG_HOST_SYMBOL, createError, createLView, directive, enterView, hostElement, leaveView, locateHostElement, renderComponentOrTemplate, directiveCreate} from './instructions';
1717
import {ComponentDef, ComponentType} from './interfaces/definition';
1818
import {LElementNode} from './interfaces/node';
1919
import {RElement, RendererFactory3, domRendererFactory3} from './interfaces/renderer';
2020
import {notImplemented, stringify} from './util';
2121

2222

23-
2423
/** Options that control how the component should be bootstrapped. */
2524
export interface CreateComponentOptions {
2625
/** Which renderer factory to use. */
@@ -176,7 +175,7 @@ export function renderComponent<T>(
176175
// Create element node at index 0 in data array
177176
hostElement(hostNode, componentDef);
178177
// Create directive instance with n() and store at index 1 in data array (el is 0)
179-
component = directive(1, componentDef.n(), componentDef);
178+
component = directiveCreate(1, componentDef.n(), componentDef);
180179
} finally {
181180
leaveView(oldView);
182181
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ export function defineComponent<T>(componentDefinition: ComponentDefArgs<T>): Co
4444
outputs: invertObject(componentDefinition.outputs),
4545
methods: invertObject(componentDefinition.methods),
4646
rendererType: resolveRendererType2(componentDefinition.rendererType) || null,
47+
exportAs: componentDefinition.exportAs,
4748
};
4849
const feature = componentDefinition.features;
4950
feature && feature.forEach((fn) => fn(def));

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

Lines changed: 129 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import {LContainerNode, LElementNode, LNode, LNodeFlags, LProjectionNode, LTextN
2424
import {assertNodeType} from './node_assert';
2525
import {appendChild, insertChild, insertView, processProjectedNode, removeView} from './node_manipulation';
2626
import {isNodeMatchingSelector} from './node_selector_matcher';
27-
import {ComponentDef, ComponentTemplate, ComponentType, DirectiveDef} from './interfaces/definition';
27+
import {ComponentDef, ComponentTemplate, ComponentType, DirectiveDef, DirectiveType} from './interfaces/definition';
2828
import {InjectFlags, diPublicInInjector, getOrCreateNodeInjectorForNode, getOrCreateElementRef, getOrCreateTemplateRef, getOrCreateContainerRef, getOrCreateInjectable} from './di';
2929
import {QueryList, LQuery_} from './query';
3030
import {RComment, RElement, RText, Renderer3, RendererFactory3, ProceduralRenderer3, ObjectOrientedRenderer3, RendererStyleFlags3} from './interfaces/renderer';
@@ -411,29 +411,37 @@ export function injectViewContainerRef(): ViewContainerRef {
411411
* Create DOM element. The instruction must later be followed by `elementEnd()` call.
412412
*
413413
* @param index Index of the element in the data array
414-
* @param nameOrComponentDef Name of the DOM Node or `ComponentDef`.
414+
* @param nameOrComponentType Name of the DOM Node or `ComponentType` to create.
415415
* @param attrs Statically bound set of attributes to be written into the DOM element on creation.
416-
* @param localName A name under which a given element is exported.
416+
* @param directiveTypes A set of directives declared on this element.
417+
* @param localRefs A set of local reference bindings on the element.
417418
*
418-
* Attributes are passed as an array of strings where elements with an even index hold an attribute
419-
* name and elements with an odd index hold an attribute value, ex.:
419+
* Attributes and localRefs are passed as an array of strings where elements with an even index
420+
* hold an attribute name and elements with an odd index hold an attribute value, ex.:
420421
* ['id', 'warning5', 'class', 'alert']
421422
*/
422423
export function elementStart(
423-
index: number, nameOrComponentDef?: string | ComponentDef<any>, attrs?: string[] | null,
424-
localName?: string): RElement {
424+
index: number, nameOrComponentType?: string | ComponentType<any>, attrs?: string[] | null,
425+
directiveTypes?: DirectiveType<any>[] | null, localRefs?: string[] | null): RElement {
425426
let node: LElementNode;
426427
let native: RElement;
427428

428-
if (nameOrComponentDef == null) {
429+
if (nameOrComponentType == null) {
429430
// native node retrieval - used for exporting elements as tpl local variables (<div #foo>)
430431
const node = data[index] !;
431432
native = node && (node as LElementNode).native;
432433
} else {
433434
ngDevMode && assertEqual(currentView.bindingStartIndex, null, 'bindingStartIndex');
434-
const isHostElement = typeof nameOrComponentDef !== 'string';
435-
const name = isHostElement ? (nameOrComponentDef as ComponentDef<any>).tag :
436-
nameOrComponentDef as string;
435+
const isHostElement = typeof nameOrComponentType !== 'string';
436+
// MEGAMORPHIC: `ngComponentDef` is a megamorphic property access here.
437+
// This is OK, since we will refactor this code and store the result in `TView.data`
438+
// which means that we will be reading this value only once. We are trading clean/simple
439+
// template
440+
// code for slight startup(first run) performance. (No impact on subsequent runs)
441+
// TODO(misko): refactor this to store the `ComponentDef` in `TView.data`.
442+
const hostComponentDef =
443+
isHostElement ? (nameOrComponentType as ComponentType<any>).ngComponentDef : null;
444+
const name = isHostElement ? hostComponentDef !.tag : nameOrComponentType as string;
437445
if (name === null) {
438446
// TODO: future support for nameless components.
439447
throw 'for now name is required';
@@ -442,30 +450,83 @@ export function elementStart(
442450

443451
let componentView: LView|null = null;
444452
if (isHostElement) {
445-
const ngStaticData = getTemplateStatic((nameOrComponentDef as ComponentDef<any>).template);
453+
const ngStaticData = getTemplateStatic(hostComponentDef !.template);
446454
componentView = addToViewTree(createLView(
447-
-1, rendererFactory.createRenderer(
448-
native, (nameOrComponentDef as ComponentDef<any>).rendererType),
455+
-1, rendererFactory.createRenderer(native, hostComponentDef !.rendererType),
449456
ngStaticData));
450457
}
451458

452459
// Only component views should be added to the view tree directly. Embedded views are
453460
// accessed through their containers because they may be removed / re-added later.
454461
node = createLNode(index, LNodeFlags.Element, native, componentView);
455462

463+
// TODO(misko): implement code which caches the local reference resolution
464+
const queryName: string|null = hack_findQueryName(hostComponentDef, localRefs, '');
465+
456466
if (node.tNode == null) {
457467
ngDevMode && assertDataInRange(index - 1);
458468
node.tNode = ngStaticData[index] =
459-
createTNode(name, attrs || null, null, localName || null);
469+
createTNode(name, attrs || null, null, hostComponentDef ? null : queryName);
460470
}
461471

462472
if (attrs) setUpAttributes(native, attrs);
463473
appendChild(node.parent !, native, currentView);
474+
475+
if (hostComponentDef) {
476+
// TODO(mhevery): This assumes that the directives come in correct order, which
477+
// is not guaranteed. Must be refactored to take it into account.
478+
directiveCreate(++index, hostComponentDef.n(), hostComponentDef, queryName);
479+
}
480+
hack_declareDirectives(index, directiveTypes, localRefs);
464481
}
465482
}
466483
return native;
467484
}
468485

486+
/**
487+
* This function instantiates a directive with a correct queryName. It is a hack since we should
488+
* compute the query value only once and store it with the template (rather than on each invocation)
489+
*/
490+
function hack_declareDirectives(
491+
index: number, directiveTypes: DirectiveType<any>[] | null | undefined,
492+
localRefs: string[] | null | undefined, ) {
493+
if (directiveTypes) {
494+
// TODO(mhevery): This assumes that the directives come in correct order, which
495+
// is not guaranteed. Must be refactored to take it into account.
496+
for (let i = 0; i < directiveTypes.length; i++) {
497+
// MEGAMORPHIC: `ngDirectiveDef` is a megamorphic property access here.
498+
// This is OK, since we will refactor this code and store the result in `TView.data`
499+
// which means that we will be reading this value only once. We are trading clean/simple
500+
// template
501+
// code for slight startup(first run) performance. (No impact on subsequent runs)
502+
// TODO(misko): refactor this to store the `DirectiveDef` in `TView.data`.
503+
const directiveDef = directiveTypes[i].ngDirectiveDef;
504+
directiveCreate(
505+
++index, directiveDef.n(), directiveDef, hack_findQueryName(directiveDef, localRefs));
506+
}
507+
}
508+
}
509+
510+
/**
511+
* This function returns the queryName for a directive. It is a hack since we should
512+
* compute the query value only once and store it with the template (rather than on each invocation)
513+
*/
514+
function hack_findQueryName(
515+
directiveDef: DirectiveDef<any>| null, localRefs: string[] | null | undefined,
516+
defaultExport?: string, ): string|null {
517+
const exportAs = directiveDef && directiveDef.exportAs || defaultExport;
518+
if (exportAs != null && localRefs) {
519+
for (let i = 0; i < localRefs.length; i = i + 2) {
520+
const local = localRefs[i];
521+
const toExportAs = localRefs[i | 1];
522+
if (toExportAs === exportAs || toExportAs === defaultExport) {
523+
return local;
524+
}
525+
}
526+
}
527+
return null;
528+
}
529+
469530
/**
470531
* Gets static data from a template function or creates a new static
471532
* data array if it doesn't already exist.
@@ -836,7 +897,21 @@ export function textBinding<T>(index: number, value: T | NO_CHANGE): void {
836897
//////////////////////////
837898

838899
/**
839-
* Create or retrieve the directive.
900+
* Retrieve a directive.
901+
*
902+
* NOTE: directives can be created in order other than the index order. They can also
903+
* be retrieved before they are created in which case the value will be null.
904+
*
905+
* @param index Each directive in a `View` will have a unique index. Directives can
906+
* be created or retrieved out of order.
907+
*/
908+
export function directive<T>(index: number): T {
909+
ngDevMode && assertDataInRange(index);
910+
return data[index];
911+
}
912+
913+
/**
914+
* Create a directive.
840915
*
841916
* NOTE: directives can be created in order other than the index order. They can also
842917
* be retrieved before they are created in which case the value will be null.
@@ -845,56 +920,47 @@ export function textBinding<T>(index: number, value: T | NO_CHANGE): void {
845920
* be created or retrieved out of order.
846921
* @param directive The directive instance.
847922
* @param directiveDef DirectiveDef object which contains information about the template.
923+
* @param queryName Name under which the query can retrieve the directive instance.
848924
*/
849-
export function directive<T>(index: number): T;
850-
export function directive<T>(
851-
index: number, directive: T, directiveDef: DirectiveDef<T>, localName?: string): T;
852-
export function directive<T>(
853-
index: number, directive?: T, directiveDef?: DirectiveDef<T>, localName?: string): T {
925+
export function directiveCreate<T>(
926+
index: number, directive: T, directiveDef: DirectiveDef<T>, queryName?: string | null): T {
854927
let instance;
855-
if (directive == null) {
856-
// return existing
857-
ngDevMode && assertDataInRange(index);
858-
instance = data[index];
928+
ngDevMode && assertEqual(currentView.bindingStartIndex, null, 'bindingStartIndex');
929+
ngDevMode && assertPreviousIsParent();
930+
let flags = previousOrParentNode !.flags;
931+
let size = flags & LNodeFlags.SIZE_MASK;
932+
if (size === 0) {
933+
flags = (index << LNodeFlags.INDX_SHIFT) | LNodeFlags.SIZE_SKIP | flags & LNodeFlags.TYPE_MASK;
859934
} else {
860-
ngDevMode && assertEqual(currentView.bindingStartIndex, null, 'bindingStartIndex');
861-
ngDevMode && assertPreviousIsParent();
862-
let flags = previousOrParentNode !.flags;
863-
let size = flags & LNodeFlags.SIZE_MASK;
864-
if (size === 0) {
865-
flags =
866-
(index << LNodeFlags.INDX_SHIFT) | LNodeFlags.SIZE_SKIP | flags & LNodeFlags.TYPE_MASK;
867-
} else {
868-
flags += LNodeFlags.SIZE_SKIP;
869-
}
870-
previousOrParentNode !.flags = flags;
871-
872-
ngDevMode && assertDataInRange(index - 1);
873-
Object.defineProperty(
874-
directive, NG_HOST_SYMBOL, {enumerable: false, value: previousOrParentNode});
935+
flags += LNodeFlags.SIZE_SKIP;
936+
}
937+
previousOrParentNode !.flags = flags;
875938

876-
data[index] = instance = directive;
939+
ngDevMode && assertDataInRange(index - 1);
940+
Object.defineProperty(
941+
directive, NG_HOST_SYMBOL, {enumerable: false, value: previousOrParentNode});
877942

878-
if (index >= ngStaticData.length) {
879-
ngStaticData[index] = directiveDef !;
880-
if (localName) {
881-
ngDevMode && assertNotNull(previousOrParentNode.tNode, 'previousOrParentNode.staticData');
882-
const tNode = previousOrParentNode !.tNode !;
883-
(tNode.localNames || (tNode.localNames = [])).push(localName, index);
884-
}
885-
}
943+
data[index] = instance = directive;
886944

887-
const diPublic = directiveDef !.diPublic;
888-
if (diPublic) {
889-
diPublic(directiveDef !);
945+
if (index >= ngStaticData.length) {
946+
ngStaticData[index] = directiveDef !;
947+
if (queryName) {
948+
ngDevMode &&
949+
assertNotNull(previousOrParentNode.tNode, 'previousOrParentNode.staticData');
950+
const nodeStaticData = previousOrParentNode !.tNode !;
951+
(nodeStaticData.localNames || (nodeStaticData.localNames = [])).push(queryName, index);
890952
}
953+
}
891954

892-
const tNode: TNode|null = previousOrParentNode.tNode !;
893-
if (tNode && tNode.attrs) {
894-
setInputsFromAttrs<T>(instance, directiveDef !.inputs, tNode);
895-
}
955+
const diPublic = directiveDef !.diPublic;
956+
if (diPublic) {
957+
diPublic(directiveDef !);
896958
}
897959

960+
const staticData: TNode|null = previousOrParentNode.tNode !;
961+
if (staticData && staticData.attrs) {
962+
setInputsFromAttrs<T>(instance, directiveDef !.inputs, staticData);
963+
}
898964
return instance;
899965
}
900966

@@ -1039,10 +1105,11 @@ export function executeViewHooks(): void {
10391105
* @param template Optional inline template
10401106
* @param tagName The name of the container element, if applicable
10411107
* @param attrs The attrs attached to the container, if applicable
1108+
* @param localRefs A set of local reference bindings on the element.
10421109
*/
10431110
export function containerStart(
1044-
index: number, template?: ComponentTemplate<any>, tagName?: string, attrs?: string[],
1045-
localName?: string): void {
1111+
index: number, directiveTypes?: DirectiveType<any>[], template?: ComponentTemplate<any>,
1112+
tagName?: string, attrs?: string[], localRefs?: string[] | null): void {
10461113
ngDevMode && assertEqual(currentView.bindingStartIndex, null, 'bindingStartIndex');
10471114

10481115
// If the direct parent of the container is a view, its views (including its comment)
@@ -1068,13 +1135,16 @@ export function containerStart(
10681135
});
10691136

10701137
if (node.tNode == null) {
1138+
// TODO(misko): implement queryName caching
1139+
const queryName: string|null = hack_findQueryName(null, localRefs, '');
10711140
node.tNode = ngStaticData[index] =
1072-
createTNode(tagName || null, attrs || null, [], localName || null);
1141+
createTNode(tagName || null, attrs || null, [], queryName || null);
10731142
}
10741143

10751144
// Containers are added to the current view tree instead of their embedded views
10761145
// because views can be removed and re-inserted.
10771146
addToViewTree(node.data);
1147+
hack_declareDirectives(index, directiveTypes, localRefs);
10781148
}
10791149

10801150
export function containerEnd() {

‎packages/core/src/render3/interfaces/definition.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,11 @@ export interface DirectiveDef<T> {
5757
*/
5858
methods: {[P in keyof T]: P};
5959

60+
/**
61+
* Name under which the directive is exported (for use with local references in template)
62+
*/
63+
exportAs: string|null;
64+
6065
/**
6166
* factory function used to create a new directive instance.
6267
*
@@ -131,6 +136,7 @@ export interface DirectiveDefArgs<T> {
131136
outputs?: {[P in keyof T]?: string};
132137
methods?: {[P in keyof T]?: string};
133138
features?: DirectiveDefFeature[];
139+
exportAs?: string;
134140
}
135141

136142
export interface ComponentDefArgs<T> extends DirectiveDefArgs<T> {

0 commit comments

Comments
 (0)