From 01308e4c7cb032f36673a4899874bdf9e1339a8d Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Wed, 29 Jan 2020 22:22:10 -0800 Subject: [PATCH] refactor(ivy): Remove `TNode.directives` in favor of `TData` (#35050) `TNode.directives` was introduced in https://github.com/angular/angular/pull/34938. Turns out that it is unnecessary because the information is already present it `TData` when combining with `TNode.directiveStart` and `TNode.directiveEnd` Mainly this is true (conceptually): ``` expect(tNode.directives).toEqual( tData.slice( tNode.directivesStart, tNode.directivesEnd - tNode.DirectivesStart -1 ) ); ``` The refactoring removes `TNode.directives` and adds `TNode.directiveStyling` as we still need to keep location in the directive in `TNode` PR Close #35050 --- .../src/render3/instructions/lview_debug.ts | 4 +- .../core/src/render3/instructions/shared.ts | 8 +- .../core/src/render3/instructions/styling.ts | 141 +++++++++++++----- packages/core/src/render3/interfaces/node.ts | 76 +++------- .../test/render3/instructions/styling_spec.ts | 6 +- 5 files changed, 130 insertions(+), 105 deletions(-) diff --git a/packages/core/src/render3/instructions/lview_debug.ts b/packages/core/src/render3/instructions/lview_debug.ts index ea5a4f9a39..a0e4bb7262 100644 --- a/packages/core/src/render3/instructions/lview_debug.ts +++ b/packages/core/src/render3/instructions/lview_debug.ts @@ -15,7 +15,7 @@ import {initNgDevMode} from '../../util/ng_dev_mode'; import {ACTIVE_INDEX, ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS, NATIVE} from '../interfaces/container'; import {DirectiveDefList, PipeDefList, ViewQueriesFunction} from '../interfaces/definition'; import {COMMENT_MARKER, ELEMENT_MARKER, I18nMutateOpCode, I18nMutateOpCodes, I18nUpdateOpCode, I18nUpdateOpCodes, TIcu} from '../interfaces/i18n'; -import {PropertyAliases, TConstants, TContainerNode, TDirectiveDefs, TElementNode, TNode as ITNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TViewNode} from '../interfaces/node'; +import {PropertyAliases, TConstants, TContainerNode, TElementNode, TNode as ITNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TViewNode} from '../interfaces/node'; import {SelectorFlags} from '../interfaces/projection'; import {TQueries} from '../interfaces/query'; import {RComment, RElement, RNode} from '../interfaces/renderer'; @@ -159,6 +159,7 @@ class TNode implements ITNode { public injectorIndex: number, // public directiveStart: number, // public directiveEnd: number, // + public directiveStylingLast: number, // public propertyBindings: number[]|null, // public flags: TNodeFlags, // public providerIndexes: TNodeProviderIndexes, // @@ -181,7 +182,6 @@ class TNode implements ITNode { public residualClasses: KeyValueArray|undefined|null, // public classBindings: TStylingRange, // public styleBindings: TStylingRange, // - public directives: TDirectiveDefs|null, // ) {} get type_(): string { diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 3086944dc3..0bc63fa699 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -24,7 +24,7 @@ import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} fr import {ACTIVE_INDEX, ActiveIndexFlag, CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS} from '../interfaces/container'; import {ComponentDef, ComponentTemplate, DirectiveDef, DirectiveDefListOrFactory, PipeDefListOrFactory, RenderFlags, ViewQueriesFunction} from '../interfaces/definition'; import {INJECTOR_BLOOM_PARENT_SIZE, NodeInjectorFactory} from '../interfaces/injector'; -import {AttributeMarker, DirectiveDefsValues, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliasValue, PropertyAliases, TAttributes, TConstants, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node'; +import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliasValue, PropertyAliases, TAttributes, TConstants, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node'; import {RComment, RElement, RNode, RText, Renderer3, RendererFactory3, isProceduralRenderer} from '../interfaces/renderer'; import {SanitizerFn} from '../interfaces/sanitization'; import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks'; @@ -807,6 +807,7 @@ export function createTNode( injectorIndex, // injectorIndex: number -1, // directiveStart: number -1, // directiveEnd: number + -1, // directiveStylingLast: number null, // propertyBindings: number[]|null 0, // flags: TNodeFlags 0, // providerIndexes: TNodeProviderIndexes @@ -829,7 +830,6 @@ export function createTNode( undefined, // residualClasses: string|null 0 as any, // classBindings: TStylingRange; 0 as any, // styleBindings: TStylingRange; - null, // directives: TDirectiveDefs|null; ) : { type: type, @@ -837,6 +837,7 @@ export function createTNode( injectorIndex: injectorIndex, directiveStart: -1, directiveEnd: -1, + directiveStylingLast: -1, propertyBindings: null, flags: 0, providerIndexes: 0, @@ -859,7 +860,6 @@ export function createTNode( residualClasses: undefined, classBindings: 0 as any, styleBindings: 0 as any, - directives: null }; } @@ -1113,7 +1113,6 @@ export function resolveDirectives( const exportsMap: ({[key: string]: number} | null) = localRefs === null ? null : {'': -1}; if (directiveDefs !== null) { - tNode.directives = [DirectiveDefsValues.INITIAL_STYLING_CURSOR_VALUE]; let totalDirectiveHostVars = 0; hasDirectives = true; initTNodeFlags(tNode, tView.data.length, directiveDefs.length); @@ -1132,7 +1131,6 @@ export function resolveDirectives( let preOrderCheckHooksFound = false; for (let i = 0; i < directiveDefs.length; i++) { const def = directiveDefs[i]; - tNode.directives.push(def); // Merge the attrs in the order of matches. This assumes that the first directive is the // component itself, so that the component has the least priority. tNode.mergedAttrs = mergeHostAttrs(tNode.mergedAttrs, def.hostAttrs); diff --git a/packages/core/src/render3/instructions/styling.ts b/packages/core/src/render3/instructions/styling.ts index 89e02fc735..85ad742bda 100644 --- a/packages/core/src/render3/instructions/styling.ts +++ b/packages/core/src/render3/instructions/styling.ts @@ -10,13 +10,13 @@ import {SafeValue, unwrapSafeValue} from '../../sanitization/bypass'; import {stylePropNeedsSanitization, ɵɵsanitizeStyle} from '../../sanitization/sanitization'; import {StyleSanitizeFn} from '../../sanitization/style_sanitizer'; import {KeyValueArray, keyValueArrayGet, keyValueArraySet} from '../../util/array_utils'; -import {assertDefined, assertEqual, assertGreaterThanOrEqual, assertLessThan, assertNotEqual, assertNotSame, throwError} from '../../util/assert'; +import {assertDefined, assertEqual, assertLessThan, assertNotEqual, throwError} from '../../util/assert'; import {EMPTY_ARRAY} from '../../util/empty'; import {concatStringsWithSpace, stringify} from '../../util/stringify'; import {assertFirstUpdatePass} from '../assert'; import {bindingUpdated} from '../bindings'; import {DirectiveDef} from '../interfaces/definition'; -import {AttributeMarker, DirectiveDefs, TAttributes, TNode, TNodeFlags, TNodeType} from '../interfaces/node'; +import {AttributeMarker, TAttributes, TNode, TNodeFlags, TNodeType} from '../interfaces/node'; import {RElement, Renderer3} from '../interfaces/renderer'; import {SanitizerFn} from '../interfaces/sanitization'; import {TStylingKey, TStylingRange, getTStylingRangeNext, getTStylingRangeNextDuplicate, getTStylingRangePrev, getTStylingRangePrevDuplicate} from '../interfaces/styling'; @@ -27,9 +27,11 @@ import {insertTStylingBinding} from '../styling/style_binding_list'; import {getLastParsedKey, getLastParsedValue, parseClassName, parseClassNameNext, parseStyle, parseStyleNext} from '../styling/styling_parser'; import {NO_CHANGE} from '../tokens'; import {getNativeByIndex} from '../util/view_utils'; + import {setDirectiveInputsWhichShadowsStyling} from './property'; + /** * Sets the current style sanitizer function which will then be used * within all follow-up prop and map-based style binding instructions @@ -360,9 +362,9 @@ export function wrapInStaticStylingKey( } else { // We are in host binding node and there was no binding instruction in template node. // This means that we need to compute the residual. - const directives = tNode.directives; - const isFirstStylingInstructionInHostBinding = directives !== null && - directives[directives[DirectiveDefs.STYLING_CURSOR]] !== hostDirectiveDef; + const directiveStylingLast = tNode.directiveStylingLast; + const isFirstStylingInstructionInHostBinding = + directiveStylingLast === -1 || tData[directiveStylingLast] !== hostDirectiveDef; if (isFirstStylingInstructionInHostBinding) { stylingKey = collectStylingFromDirectives(hostDirectiveDef, tData, tNode, stylingKey, isClassBased); @@ -391,7 +393,7 @@ export function wrapInStaticStylingKey( // the statics may have moved from the residual to the `stylingKey` and so we have to // recompute. // - If `undefined` this is the first time we are running. - residual = collectResidual(tNode, isClassBased); + residual = collectResidual(tData, tNode, isClassBased); } } } @@ -424,6 +426,58 @@ function getTemplateHeadTStylingKey(tData: TData, tNode: TNode, isClassBased: bo return tData[getTStylingRangePrev(bindings)] as TStylingKey; } +/** + * Update the `TStylingKey` of the first template instruction in `TNode`. + * + * Logically `hostBindings` styling instructions are of lower priority than that of the template. + * However, they execute after the template styling instructions. This means that they get inserted + * in front of the template styling instructions. + * + * If we have a template styling instruction and a new `hostBindings` styling instruction is + * executed it means that it may need to steal static fields from the template instruction. This + * method allows us to update the first template instruction `TStylingKey` with a new value. + * + * Assume: + * ``` + *
+ * + * @Directive({ + * host: { + * 'style': 'width: 100px', + * '[style.color]': 'dirExp', + * } + * }) + * class MyDir {} + * ``` + * + * when `[style.color]="tmplExp"` executes it creates this data structure. + * ``` + * ['', 'color', 'color', 'red', 'width', '100px'], + * ``` + * + * The reason for this is that the template instruction does not know if there are styling + * instructions and must assume that there are none and must collect all of the static styling. + * (both + * `color' and 'width`) + * + * When `'[style.color]': 'dirExp',` executes we need to insert a new data into the linked list. + * ``` + * ['', 'color', 'width', '100px'], // newly inserted + * ['', 'color', 'color', 'red', 'width', '100px'], // this is wrong + * ``` + * + * Notice that the template statics is now wrong as it incorrectly contains `width` so we need to + * update it like so: + * ``` + * ['', 'color', 'width', '100px'], + * ['', 'color', 'color', 'red'], // UPDATE + * ``` + * + * @param tData `TData` where the linked list is stored. + * @param tNode `TNode` for which the styling is being computed. + * @param isClassBased `true` if `class` (`false` if `style`) + * @param tStylingKey New `TStylingKey` which is replacing the old one. + */ function setTemplateHeadTStylingKey( tData: TData, tNode: TNode, isClassBased: boolean, tStylingKey: TStylingKey): void { const bindings = isClassBased ? tNode.classBindings : tNode.styleBindings; @@ -433,15 +487,29 @@ function setTemplateHeadTStylingKey( tData[getTStylingRangePrev(bindings)] = tStylingKey; } -function collectResidual(tNode: TNode, isClassBased: boolean): KeyValueArray|null { +/** + * Collect all static values after the current `TNode.directiveStylingLast` index. + * + * Collect the remaining styling information which has not yet been collected by an existing + * styling instruction. + * + * @param tData `TData` where the `DirectiveDefs` are stored. + * @param tNode `TNode` which contains the directive range. + * @param isClassBased `true` if `class` (`false` if `style`) + */ +function collectResidual(tData: TData, tNode: TNode, isClassBased: boolean): KeyValueArray| + null { let residual: KeyValueArray|null|undefined = undefined; - const directives = tNode.directives; - if (directives) { - for (let i = directives[DirectiveDefs.STYLING_CURSOR] + 1; i < directives.length; i++) { - const attrs = (directives[i] as DirectiveDef).hostAttrs; - residual = - collectStylingFromTAttrs(residual, attrs, isClassBased) as KeyValueArray| null; - } + const directiveEnd = tNode.directiveEnd; + ngDevMode && + assertNotEqual( + tNode.directiveStylingLast, -1, + 'By the time this function gets called at least one hostBindings-node styling instruction must have executed.'); + // We add `1 + tNode.directiveStart` because we need to skip the current directive (as we are + // collecting things after the last `hostBindings` directive which had a styling instruction.) + for (let i = 1 + tNode.directiveStylingLast; i < directiveEnd; i++) { + const attrs = (tData[i] as DirectiveDef).hostAttrs; + residual = collectStylingFromTAttrs(residual, attrs, isClassBased) as KeyValueArray| null; } return collectStylingFromTAttrs(residual, tNode.attrs, isClassBased) as KeyValueArray| null; } @@ -461,29 +529,28 @@ function collectResidual(tNode: TNode, isClassBased: boolean): KeyValueArray| null, tData: TData, tNode: TNode, stylingKey: TStylingKey, isClassBased: boolean): TStylingKey { - const directives = tNode.directives; - if (directives != null) { - ngDevMode && hostDirectiveDef && - assertGreaterThanOrEqual( - directives.indexOf(hostDirectiveDef, directives[DirectiveDefs.STYLING_CURSOR]), 0, - 'Expecting that the current directive is in the directive list'); - // We need to loop because there can be directives which have `hostAttrs` but don't have - // `hostBindings` so this loop catches up up to the current directive.. - let currentDirective: DirectiveDef|null = null; - let index = directives[DirectiveDefs.STYLING_CURSOR]; - while (index + 1 < directives.length) { - index++; - currentDirective = directives[index] as DirectiveDef; - ngDevMode && assertDefined(currentDirective, 'expected to be defined'); - stylingKey = collectStylingFromTAttrs(stylingKey, currentDirective.hostAttrs, isClassBased); - if (currentDirective === hostDirectiveDef) break; - } - if (hostDirectiveDef !== null) { - // we only advance the styling cursor if we are collecting data from host bindings. - // Template executes before host bindings and so if we would update the index, - // host bindings would not get their statics. - directives[DirectiveDefs.STYLING_CURSOR] = index; - } + // We need to loop because there can be directives which have `hostAttrs` but don't have + // `hostBindings` so this loop catches up to the current directive.. + let currentDirective: DirectiveDef|null = null; + const directiveEnd = tNode.directiveEnd; + let directiveStylingLast = tNode.directiveStylingLast; + if (directiveStylingLast === -1) { + directiveStylingLast = tNode.directiveStart; + } else { + directiveStylingLast++; + } + while (directiveStylingLast < directiveEnd) { + currentDirective = tData[directiveStylingLast] as DirectiveDef; + ngDevMode && assertDefined(currentDirective, 'expected to be defined'); + stylingKey = collectStylingFromTAttrs(stylingKey, currentDirective.hostAttrs, isClassBased); + if (currentDirective === hostDirectiveDef) break; + directiveStylingLast++; + } + if (hostDirectiveDef !== null) { + // we only advance the styling cursor if we are collecting data from host bindings. + // Template executes before host bindings and so if we would update the index, + // host bindings would not get their statics. + tNode.directiveStylingLast = directiveStylingLast; } return stylingKey; } diff --git a/packages/core/src/render3/interfaces/node.ts b/packages/core/src/render3/interfaces/node.ts index 87dfcfeade..e2d662f517 100644 --- a/packages/core/src/render3/interfaces/node.ts +++ b/packages/core/src/render3/interfaces/node.ts @@ -294,6 +294,24 @@ export interface TNode { */ directiveEnd: number; + /** + * Stores the last directive which had a styling instruction. + * + * Initial value of this is `-1` which means that no `hostBindings` styling instruction has + * executed. As `hostBindings` instructions execute they set the value to the index of the + * `DirectiveDef` which contained the last `hostBindings` styling instruction. + * + * Valid values are: + * - `-1` No `hostBindings` instruction has executed. + * - `directiveStart <= directiveStylingLast < directiveEnd`: Points to the `DirectiveDef` of the + * last styling instruction which executed in the `hostBindings`. + * + * This data is needed so that styling instructions know which static styling data needs to be + * collected from the `DirectiveDef.hostAttrs`. A styling instruction needs to collect all data + * since last styling instruction. + */ + directiveStylingLast: number; + /** * Stores indexes of property bindings. This field is only set in the ngDevMode and holds indexes * of property bindings so TestBed can get bound property metadata for a given node. @@ -346,13 +364,6 @@ export interface TNode { */ mergedAttrs: TAttributes|null; - // TODO(misko): pre discussion with Kara, it seems that we don't need `directives` since the same - // information is already present in the TData. Maybe worth refactoring. - /** - * Stores the directive defs matched on the current TNode (along with style cursor.) - */ - directives: TDirectiveDefs|null; - /** * A set of local names under which a given element is exported in a template and * visible to queries. An entry in this array can be created for different reasons: @@ -814,54 +825,3 @@ export function hasClassInput(tNode: TNode) { export function hasStyleInput(tNode: TNode) { return (tNode.flags & TNodeFlags.hasStyleInput) !== 0; } - -/** - * Constant enums for accessing data in the `TDirectiveDefs` - */ -export const enum DirectiveDefs { - /// Location where the STYLING_CURSOR is stored. - STYLING_CURSOR = 0, - /// Header offset from which iterating over `DirectiveDefs` should start. - HEADER_OFFSET = 1 -} - -/** - * Constant enums for initial values in the `TDirectiveDefs` - */ -export const enum DirectiveDefsValues { - // Initial value for the `STYLING_CURSOR` - INITIAL_STYLING_CURSOR_VALUE = 0, -} - -/** - * Stores `DirectiveDefs` associated with the current `TNode` as well as styling cursor. - */ -export interface TDirectiveDefs extends Array> { - /** - * As styling instructions (`ɵɵstyleProp`/`ɵɵclassProp`/`ɵɵstyleMap`/`ɵɵclassMap`) are executing - * they also need to get a hold of the `DirectiveDef.hostAttrs` and so that they know what - * static styling values to use. The styling instructions need this information so that they can - * lazily create `TStylingStatic`. - * - * When styling is executing it can get a hold of its `DirectiveDefs` but that alone is not - * sufficient for two reasons: - * 1. Styling instruction needs to coalesce other directives which came before it and which have - * static value but may not have a styling instruction to attach the static values to. - * 2. There may be more than one styling instruction per `hostBindings` and only the first - * styling instruction should create the `TStylingStatic`. - * - * The algorithm for doing this is: - * - look up the current `DirectiveDef` associated with the current instruction. - * - If `STYLING_CURSOR === 0 || tDirectiveDefs[stylingCursor] !== currentDirectiveDef` than - * create `TStylingStatic` and: - * - iterate over `TDirectiveDefs[++stylingCursor]` and insert them into the `TStylingStatic` - * until you reach `DirectiveDef` associated with the current instruction. - * - If new `TStylingStatic` was created, recompute the residual styling values. - * - * The above algorithm will ensure that the styling instructions consume static styling values - * associated until a given instruction. After consuming instructions, it is always important to - * clear the residual (See `TNode.residualClass`/`TNode.residualStyle`), since this may be the - * last styling instruction, and we need to lazily recreate the residual value on as needed basis. - */ - [DirectiveDefs.STYLING_CURSOR]: number; -} \ No newline at end of file diff --git a/packages/core/test/render3/instructions/styling_spec.ts b/packages/core/test/render3/instructions/styling_spec.ts index bb20346389..bc71737c69 100644 --- a/packages/core/test/render3/instructions/styling_spec.ts +++ b/packages/core/test/render3/instructions/styling_spec.ts @@ -9,7 +9,7 @@ import {DirectiveDef} from '@angular/core/src/render3'; import {ɵɵdefineDirective} from '@angular/core/src/render3/definition'; import {classStringParser, styleStringParser, toStylingKeyValueArray, ɵɵclassProp, ɵɵstyleMap, ɵɵstyleProp, ɵɵstyleSanitizer} from '@angular/core/src/render3/instructions/styling'; -import {AttributeMarker, TAttributes, TDirectiveDefs} from '@angular/core/src/render3/interfaces/node'; +import {AttributeMarker, TAttributes} from '@angular/core/src/render3/interfaces/node'; import {StylingRange, TStylingKey, TStylingRange, getTStylingRangeNext, getTStylingRangeNextDuplicate, getTStylingRangePrev, getTStylingRangePrevDuplicate, setTStylingRangeNext, setTStylingRangePrev, toTStylingRange} from '@angular/core/src/render3/interfaces/styling'; import {HEADER_OFFSET, TVIEW} from '@angular/core/src/render3/interfaces/view'; import {getLView, leaveView, setBindingRootForHostBindings} from '@angular/core/src/render3/state'; @@ -520,13 +520,13 @@ class MockDir {} function givenDirectiveAttrs(tAttrs: TAttributes[]) { const tNode = getTNode(); const tData = getTData(); - const directives: TDirectiveDefs = tNode.directives = [0]; + tNode.directiveStart = getTDataIndexFromDirectiveIndex(0); + tNode.directiveEnd = getTDataIndexFromDirectiveIndex(tAttrs.length); for (let i = 0; i < tAttrs.length; i++) { const tAttr = tAttrs[i]; const directiveDef = ɵɵdefineDirective({type: MockDir, hostAttrs: tAttr}) as DirectiveDef; applyTAttributes(directiveDef.hostAttrs); tData[getTDataIndexFromDirectiveIndex(i)] = directiveDef; - directives.push(directiveDef); } }