cargdev/angular
1
0
Fork 0
Code Issues 6 Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(ivy): move hostVars/hostAttrs from instruction to DirectiveDef (#34683)

Browse Source 
This change moves information from instructions to declarative position:
- `ɵɵallocHostVars(vars)` => `DirectiveDef.hostVars`
- `ɵɵelementHostAttrs(attrs)` => `DirectiveDef.hostAttrs`

When merging directives it is necessary to know about `hostVars` and `hostAttrs`. Before this change the information was stored in the `hostBindings` function. This was problematic, because in order to get to the information the `hostBindings` would have to be executed. In order for `hostBindings` to be executed the directives would have to be instantiated. This means that the directive instantiation would happen before we had knowledge about the `hostAttrs` and as a result the directive could observe in the constructor that not all of the `hostAttrs` have been applied. This further complicates the runtime as we have to apply `hostAttrs` in parts over many invocations.

`ɵɵallocHostVars` was unnecessarily complicated because it would have to update the `LView` (and Blueprint) while existing directives are already executing. By moving it out of `hostBindings` function we can access it statically and we can create correct `LView` (and Blueprint) in a single pass.

This change only changes how the instructions are generated, but does not change the runtime much. (We cheat by emulating the old behavior by calling `ɵɵallocHostVars` and `ɵɵelementHostAttrs`) Subsequent change will refactor the runtime to take advantage of the static information.

PR Close #34683
This commit is contained in:
Miško Hevery
2020-01-08 11:32:33 -08:00
parent 239c602f69
commit d566621fef
33 changed files with 751 additions and 185 deletions
Download Patch File Download Diff File Expand all files Collapse all files

82
packages/core/test/acceptance/inherit_definition_feature_spec.ts
View File

@ -7,6 +7,8 @@
*/
import {Component, ContentChildren, Directive, EventEmitter, HostBinding, HostListener, Input, OnChanges, Output, QueryList, ViewChildren} from '@angular/core';
import {ivyEnabled} from '@angular/core/src/ivy_switch';
import {getDirectiveDef} from '@angular/core/src/render3/definition';
import {TestBed} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {onlyInIvy} from '@angular/private/testing';
@ -42,6 +44,86 @@ describe('inheritance', () => {
}).toThrowError('Directives cannot inherit Components');
});
describe('multiple children', () => {
it('should ensure that multiple child classes don\'t cause multiple parent execution', () => {
// Assume this inheritance:
// Base
// |
// Super
// / \
// Sub1 Sub2
//
// In the above case:
// 1. Sub1 as will walk the inheritance Sub1, Super, Base
// 2. Sub2 as will walk the inheritance Sub2, Super, Base
//
// Notice that Super, Base will get walked twice. Because inheritance works by wrapping parent
// hostBindings function in a delegate which calls the hostBindings of the directive as well
// as super, we need to ensure that we don't double wrap the hostBindings function. Doing so
// would result in calling the hostBindings multiple times (unnecessarily). This would be
// especially an issue if we have a lot of sub-classes (as is common in component libraries)
const log: string[] = [];
@Directive({selector: '[superDir]'})
class BaseDirective {
@HostBinding('style.background-color')
get backgroundColor() {
log.push('Base.backgroundColor');
return 'white';
}
}
@Directive({selector: '[superDir]'})
class SuperDirective extends BaseDirective {
@HostBinding('style.color')
get color() {
log.push('Super.color');
return 'blue';
}
}
@Directive({selector: '[subDir1]'})
class Sub1Directive extends SuperDirective {
@HostBinding('style.height')
get height() {
log.push('Sub1.height');
return '200px';
}
}
@Directive({selector: '[subDir2]'})
class Sub2Directive extends SuperDirective {
@HostBinding('style.width')
get width() {
log.push('Sub2.width');
return '100px';
}
}
@Component({template: `<div subDir1 subDir2></div>`})
class App {
}
TestBed.configureTestingModule({
declarations: [App, Sub1Directive, Sub2Directive, SuperDirective],
});
const fixture = TestBed.createComponent(App);
fixture.detectChanges(false); // Don't check for no changes (so that assertion does not need
// to worry about it.)
expect(log).toEqual([
'Base.backgroundColor', 'Super.color', 'Sub1.height', //
'Base.backgroundColor', 'Super.color', 'Sub2.width', //
]);
if (ivyEnabled) {
expect(getDirectiveDef(BaseDirective) !.hostVars).toEqual(1);
expect(getDirectiveDef(SuperDirective) !.hostVars).toEqual(2);
expect(getDirectiveDef(Sub1Directive) !.hostVars).toEqual(3);
expect(getDirectiveDef(Sub2Directive) !.hostVars).toEqual(3);
}
});
});
describe('ngOnChanges', () => {
it('should be inherited when super is a directive', () => {
const log: string[] = [];

18
packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json
View File

@ -314,6 +314,9 @@
{
"name": "getContainerRenderParent"
},
{
"name": "getCurrentDirectiveDef"
},
{
"name": "getDirectiveDef"
},
@ -404,6 +407,9 @@
{
"name": "getStylingMapArray"
},
{
"name": "getTNode"
},
{
"name": "hasActiveElementFlag"
},
@ -545,6 +551,12 @@
{
"name": "objectToClassName"
},
{
"name": "prefillHostVars"
},
{
"name": "queueHostBindingForCheck"
},
{
"name": "refreshChildComponents"
},
@ -695,6 +707,9 @@
{
"name": "writeStylingValueDirectly"
},
{
"name": "ɵɵallocHostVars"
},
{
"name": "ɵɵdefineComponent"
},
@ -710,6 +725,9 @@
{
"name": "ɵɵelementEnd"
},
{
"name": "ɵɵelementHostAttrs"
},
{
"name": "ɵɵelementStart"
},

96
packages/core/test/bundling/hello_world/bundle.golden_symbols.json
View File

@ -104,6 +104,9 @@
{
"name": "RENDERER_FACTORY"
},
{
"name": "RendererStyleFlags3"
},
{
"name": "SANITIZER"
},
@ -137,12 +140,18 @@
{
"name": "_renderCompCount"
},
{
"name": "addItemToStylingMap"
},
{
"name": "addToViewTree"
},
{
"name": "allocLFrame"
},
{
"name": "allocStylingMapArray"
},
{
"name": "appendChild"
},
@ -161,6 +170,9 @@
{
"name": "callHooks"
},
{
"name": "concatString"
},
{
"name": "createLFrame"
},
@ -227,6 +239,9 @@
{
"name": "extractPipeDef"
},
{
"name": "forceStylesAsString"
},
{
"name": "generateExpandoInstructionBlock"
},
@ -248,6 +263,9 @@
{
"name": "getContainerRenderParent"
},
{
"name": "getCurrentDirectiveDef"
},
{
"name": "getDirectiveDef"
},
@ -260,6 +278,9 @@
{
"name": "getFirstNativeNode"
},
{
"name": "getInitialStylingValue"
},
{
"name": "getInjectorIndex"
},
@ -275,6 +296,12 @@
{
"name": "getLViewParent"
},
{
"name": "getMapProp"
},
{
"name": "getMapValue"
},
{
"name": "getNativeAnchorNode"
},
@ -317,12 +344,21 @@
{
"name": "getSelectedIndex"
},
{
"name": "getStylingMapArray"
},
{
"name": "getTNode"
},
{
"name": "hasActiveElementFlag"
},
{
"name": "hasParentInjector"
},
{
"name": "hyphenate"
},
{
"name": "includeViewProviders"
},
@ -350,6 +386,9 @@
{
"name": "invokeHostBindingsInCreationMode"
},
{
"name": "isAnimationProp"
},
{
"name": "isComponentDef"
},
@ -365,6 +404,12 @@
{
"name": "isProceduralRenderer"
},
{
"name": "isStylingContext"
},
{
"name": "isStylingValueDefined"
},
{
"name": "leaveDI"
},
@ -398,6 +443,15 @@
{
"name": "noSideEffects"
},
{
"name": "objectToClassName"
},
{
"name": "prefillHostVars"
},
{
"name": "queueHostBindingForCheck"
},
{
"name": "refreshChildComponents"
},
@ -416,6 +470,9 @@
{
"name": "refreshView"
},
{
"name": "registerInitialStylingOnTNode"
},
{
"name": "registerPreOrderHooks"
},
@ -428,9 +485,15 @@
{
"name": "renderComponent"
},
{
"name": "renderInitialStyling"
},
{
"name": "renderStringify"
},
{
"name": "renderStylingMap"
},
{
"name": "renderView"
},
@ -449,6 +512,12 @@
{
"name": "setBindingRoot"
},
{
"name": "setClass"
},
{
"name": "setClassName"
},
{
"name": "setCurrentDirectiveDef"
},
@ -464,27 +533,54 @@
{
"name": "setInjectImplementation"
},
{
"name": "setMapValue"
},
{
"name": "setPreviousOrParentTNode"
},
{
"name": "setSelectedIndex"
},
{
"name": "setStyle"
},
{
"name": "setStyleAttr"
},
{
"name": "setUpAttributes"
},
{
"name": "stringifyForError"
},
{
"name": "stylingMapToString"
},
{
"name": "syncViewWithBlueprint"
},
{
"name": "unwrapRNode"
},
{
"name": "updateRawValueOnContext"
},
{
"name": "viewAttachedToChangeDetector"
},
{
"name": "writeStylingValueDirectly"
},
{
"name": "ɵɵallocHostVars"
},
{
"name": "ɵɵdefineComponent"
},
{
"name": "ɵɵelementHostAttrs"
},
{
"name": "ɵɵtext"
}

15
packages/core/test/bundling/todo/bundle.golden_symbols.json
View File

@ -647,6 +647,9 @@
{
"name": "getContextLView"
},
{
"name": "getCurrentDirectiveDef"
},
{
"name": "getCurrentStyleSanitizer"
},
@ -1079,6 +1082,12 @@
{
"name": "patchHostStylingFlag"
},
{
"name": "prefillHostVars"
},
{
"name": "queueHostBindingForCheck"
},
{
"name": "readPatchedData"
},
@ -1358,6 +1367,9 @@
{
"name": "ɵɵadvance"
},
{
"name": "ɵɵallocHostVars"
},
{
"name": "ɵɵclassProp"
},
@ -1376,6 +1388,9 @@
{
"name": "ɵɵelementEnd"
},
{
"name": "ɵɵelementHostAttrs"
},
{
"name": "ɵɵelementStart"
},

119
packages/core/test/render3/util/attr_util_spec.ts Normal file
View File

@ -0,0 +1,119 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {AttributeMarker} from '@angular/core/src/render3';
import {TAttributes} from '@angular/core/src/render3/interfaces/node';
import {mergeHostAttribute, mergeHostAttrs} from '@angular/core/src/render3/util/attrs_utils';
import {describe} from '@angular/core/testing/src/testing_internal';
describe('attr_util', () => {
describe('mergeHostAttribute', () => {
it('should add new attributes', () => {
const attrs: TAttributes = [];
mergeHostAttribute(attrs, -1, 'Key', null, 'value');
expect(attrs).toEqual(['Key', 'value']);
mergeHostAttribute(attrs, -1, 'A', null, 'a');
expect(attrs).toEqual(['Key', 'value', 'A', 'a']);
mergeHostAttribute(attrs, -1, 'X', null, 'x');
expect(attrs).toEqual(['Key', 'value', 'A', 'a', 'X', 'x']);
mergeHostAttribute(attrs, -1, 'Key', null, 'new');
expect(attrs).toEqual(['Key', 'new', 'A', 'a', 'X', 'x']);
});
it('should add new classes', () => {
const attrs: TAttributes = [];
mergeHostAttribute(attrs, AttributeMarker.Classes, 'CLASS', null, null);
expect(attrs).toEqual([AttributeMarker.Classes, 'CLASS']);
mergeHostAttribute(attrs, AttributeMarker.Classes, 'A', null, null);
expect(attrs).toEqual([AttributeMarker.Classes, 'CLASS', 'A']);
mergeHostAttribute(attrs, AttributeMarker.Classes, 'X', null, null);
expect(attrs).toEqual([AttributeMarker.Classes, 'CLASS', 'A', 'X']);
mergeHostAttribute(attrs, AttributeMarker.Classes, 'CLASS', null, null);
expect(attrs).toEqual([AttributeMarker.Classes, 'CLASS', 'A', 'X']);
});
it('should add new styles', () => {
const attrs: TAttributes = [];
mergeHostAttribute(attrs, AttributeMarker.Styles, 'Style', null, 'v1');
expect(attrs).toEqual([AttributeMarker.Styles, 'Style', 'v1']);
mergeHostAttribute(attrs, AttributeMarker.Styles, 'A', null, 'v2');
expect(attrs).toEqual([AttributeMarker.Styles, 'Style', 'v1', 'A', 'v2']);
mergeHostAttribute(attrs, AttributeMarker.Styles, 'X', null, 'v3');
expect(attrs).toEqual([AttributeMarker.Styles, 'Style', 'v1', 'A', 'v2', 'X', 'v3']);
mergeHostAttribute(attrs, AttributeMarker.Styles, 'Style', null, 'new');
expect(attrs).toEqual([AttributeMarker.Styles, 'Style', 'new', 'A', 'v2', 'X', 'v3']);
});
it('should keep different types together', () => {
const attrs: TAttributes = [];
mergeHostAttribute(attrs, -1, 'Key', null, 'value');
expect(attrs).toEqual(['Key', 'value']);
mergeHostAttribute(attrs, AttributeMarker.Classes, 'CLASS', null, null);
expect(attrs).toEqual(['Key', 'value', AttributeMarker.Classes, 'CLASS']);
mergeHostAttribute(attrs, AttributeMarker.Styles, 'Style', null, 'v1');
expect(attrs).toEqual([
'Key', 'value', AttributeMarker.Classes, 'CLASS', AttributeMarker.Styles, 'Style', 'v1'
]);
mergeHostAttribute(attrs, -1, 'Key2', null, 'value2');
expect(attrs).toEqual([
'Key', 'value', 'Key2', 'value2', AttributeMarker.Classes, 'CLASS', AttributeMarker.Styles,
'Style', 'v1'
]);
mergeHostAttribute(attrs, AttributeMarker.Classes, 'CLASS2', null, null);
expect(attrs).toEqual([
'Key', 'value', 'Key2', 'value2', AttributeMarker.Classes, 'CLASS', 'CLASS2',
AttributeMarker.Styles, 'Style', 'v1'
]);
mergeHostAttribute(attrs, AttributeMarker.Styles, 'Style2', null, 'v2');
expect(attrs).toEqual([
'Key', 'value', 'Key2', 'value2', AttributeMarker.Classes, 'CLASS', 'CLASS2',
AttributeMarker.Styles, 'Style', 'v1', 'Style2', 'v2'
]);
mergeHostAttribute(attrs, AttributeMarker.NamespaceURI, 'uri', 'key', 'value');
expect(attrs).toEqual([
'Key', 'value', 'Key2', 'value2', AttributeMarker.NamespaceURI, 'uri', 'key', 'value',
AttributeMarker.Classes, 'CLASS', 'CLASS2', AttributeMarker.Styles, 'Style', 'v1', 'Style2',
'v2'
]);
mergeHostAttribute(attrs, AttributeMarker.NamespaceURI, 'uri', 'key', 'new value');
expect(attrs).toEqual([
'Key', 'value', 'Key2', 'value2', AttributeMarker.NamespaceURI, 'uri', 'key', 'new value',
AttributeMarker.Classes, 'CLASS', 'CLASS2', AttributeMarker.Styles, 'Style', 'v1', 'Style2',
'v2'
]);
});
});
describe('mergeHostAttrs', () => {
it('should ignore nulls/empty', () => {
expect(mergeHostAttrs(null, null)).toEqual(null);
expect(mergeHostAttrs([], null)).toEqual([]);
expect(mergeHostAttrs(null, [])).toEqual(null);
});
it('should copy if dst is null', () => {
expect(mergeHostAttrs(null, ['K', 'v'])).toEqual(['K', 'v']);
expect(mergeHostAttrs(['K', '', 'X', 'x'], ['K', 'v'])).toEqual(['K', 'v', 'X', 'x']);
});
});
});