From a2e890e4f7d01f0edd1c69ce5be61ef69318d9e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Mon, 9 Sep 2019 13:56:29 -0700 Subject: [PATCH] refactor(ivy): get rid of styling cleanup functions outside of styling code (#32591) Prior to this patch, each time `advance()` would run (or when a templateFn or hostBindings code exits) then the core change detection code would check to see whether the styling data needs to be reset. This patch removes that functionality and places everything inside of the scheduled styling exit function. This means that each time one or more styling bindings run (even if the value hasn't changed) then an exit function will be scheduled and that will do all the cleanup. PR Close #32591 --- integration/_payload-limits.json | 2 +- .../core/src/render3/instructions/advance.ts | 5 - .../core/src/render3/instructions/shared.ts | 4 - packages/core/src/render3/state.ts | 13 +- .../core/src/render3/styling_next/bindings.ts | 16 +- .../src/render3/styling_next/instructions.ts | 34 ++-- .../core/test/acceptance/styling_next_spec.ts | 171 +++++++++++++++++- .../cyclic_import/bundle.golden_symbols.json | 6 - .../hello_world/bundle.golden_symbols.json | 6 - .../bundling/todo/bundle.golden_symbols.json | 14 +- 10 files changed, 203 insertions(+), 68 deletions(-) diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index 0086b1ca34..9945a54f07 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime": 1440, - "main": 13604, + "main": 13415, "polyfills": 45340 } } diff --git a/packages/core/src/render3/instructions/advance.ts b/packages/core/src/render3/instructions/advance.ts index 86707b3781..307d9ece05 100644 --- a/packages/core/src/render3/instructions/advance.ts +++ b/packages/core/src/render3/instructions/advance.ts @@ -9,7 +9,6 @@ import {assertDataInRange, assertGreaterThan} from '../../util/assert'; import {executeCheckHooks, executeInitAndCheckHooks} from '../hooks'; import {FLAGS, HEADER_OFFSET, InitPhaseState, LView, LViewFlags, TVIEW} from '../interfaces/view'; import {ActiveElementFlags, executeElementExitFn, getCheckNoChangesMode, getLView, getSelectedIndex, hasActiveElementFlag, setSelectedIndex} from '../state'; -import {resetStylingState} from '../styling_next/state'; @@ -76,10 +75,6 @@ export function selectIndexInternal(lView: LView, index: number, checkNoChangesM } } - if (hasActiveElementFlag(ActiveElementFlags.ResetStylesOnExit)) { - resetStylingState(); - } - // We must set the selected index *after* running the hooks, because hooks may have side-effects // that cause other template functions to run, thus updating the selected index, which is global // state. If we run `setSelectedIndex` *before* we run the hooks, in some cases the selected index diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index d3e7fd1d80..74fd5e88da 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -32,7 +32,6 @@ import {assertNodeOfPossibleTypes} from '../node_assert'; import {isNodeMatchingSelectorList} from '../node_selector_matcher'; import {ActiveElementFlags, executeElementExitFn, getBindingsEnabled, getCheckNoChangesMode, getIsParent, getPreviousOrParentTNode, getSelectedIndex, hasActiveElementFlag, incrementActiveDirectiveId, namespaceHTMLInternal, selectView, setActiveHostElement, setBindingRoot, setCheckNoChangesMode, setCurrentDirectiveDef, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state'; import {renderStylingMap} from '../styling_next/bindings'; -import {resetStylingState} from '../styling_next/state'; import {NO_CHANGE} from '../tokens'; import {ANIMATION_PROP_PREFIX, isAnimationProp} from '../util/attrs_utils'; import {INTERPOLATION_DELIMITER, renderStringify, stringifyForError} from '../util/misc_utils'; @@ -505,9 +504,6 @@ function executeTemplate( if (hasActiveElementFlag(ActiveElementFlags.RunExitFn)) { executeElementExitFn(); } - if (hasActiveElementFlag(ActiveElementFlags.ResetStylesOnExit)) { - resetStylingState(); - } setSelectedIndex(prevSelectedIndex); } } diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index 0699a1e42a..ce7d5d2a67 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -12,8 +12,7 @@ import {assertDefined} from '../util/assert'; import {assertLViewOrUndefined} from './assert'; import {ComponentDef, DirectiveDef} from './interfaces/definition'; import {TElementNode, TNode, TViewNode} from './interfaces/node'; -import {CONTEXT, DECLARATION_VIEW, LView, OpaqueViewState, TVIEW} from './interfaces/view'; -import {resetStylingState} from './styling_next/state'; +import {CONTEXT, DECLARATION_VIEW, LView, OpaqueViewState} from './interfaces/view'; /** @@ -144,8 +143,7 @@ let activeDirectiveId = 0; export const enum ActiveElementFlags { Initial = 0b00, RunExitFn = 0b01, - ResetStylesOnExit = 0b10, - Size = 2, + Size = 1, } /** @@ -174,9 +172,6 @@ export function setActiveHostElement(elementIndex: number | null = null) { if (hasActiveElementFlag(ActiveElementFlags.RunExitFn)) { executeElementExitFn(); } - if (hasActiveElementFlag(ActiveElementFlags.ResetStylesOnExit)) { - resetStylingState(); - } setSelectedIndex(elementIndex === null ? -1 : elementIndex); activeDirectiveId = 0; } @@ -390,6 +385,10 @@ export function setCurrentQueryIndex(value: number): void { * @returns the previously active lView; */ export function selectView(newView: LView, hostTNode: TElementNode | TViewNode | null): LView { + if (hasActiveElementFlag(ActiveElementFlags.RunExitFn)) { + executeElementExitFn(); + } + ngDevMode && assertLViewOrUndefined(newView); const oldView = lView; diff --git a/packages/core/src/render3/styling_next/bindings.ts b/packages/core/src/render3/styling_next/bindings.ts index cc40299b64..1a216b826c 100644 --- a/packages/core/src/render3/styling_next/bindings.ts +++ b/packages/core/src/render3/styling_next/bindings.ts @@ -405,18 +405,22 @@ export function flushStyling( if (!isContextLocked(stylesContext, hostBindingsMode)) { lockAndFinalizeContext(stylesContext, hostBindingsMode); } - applyStylingViaContext( - stylesContext, renderer, element, data, state.stylesBitMask, setStyle, styleSanitizer, - hostBindingsMode); + if (state.stylesBitMask !== 0) { + applyStylingViaContext( + stylesContext, renderer, element, data, state.stylesBitMask, setStyle, styleSanitizer, + hostBindingsMode); + } } if (classesContext) { if (!isContextLocked(classesContext, hostBindingsMode)) { lockAndFinalizeContext(classesContext, hostBindingsMode); } - applyStylingViaContext( - classesContext, renderer, element, data, state.classesBitMask, setClass, null, - hostBindingsMode); + if (state.classesBitMask !== 0) { + applyStylingViaContext( + classesContext, renderer, element, data, state.classesBitMask, setClass, null, + hostBindingsMode); + } } resetStylingState(); diff --git a/packages/core/src/render3/styling_next/instructions.ts b/packages/core/src/render3/styling_next/instructions.ts index f967d27e76..bf4688d468 100644 --- a/packages/core/src/render3/styling_next/instructions.ts +++ b/packages/core/src/render3/styling_next/instructions.ts @@ -11,7 +11,7 @@ import {setInputsForProperty} from '../instructions/shared'; import {AttributeMarker, TAttributes, TNode, TNodeType} from '../interfaces/node'; import {RElement} from '../interfaces/renderer'; import {BINDING_INDEX, LView, RENDERER} from '../interfaces/view'; -import {ActiveElementFlags, getActiveDirectiveId, getCurrentStyleSanitizer, getLView, getSelectedIndex, setActiveElementFlag, setCurrentStyleSanitizer, setElementExitFn} from '../state'; +import {getActiveDirectiveId, getCurrentStyleSanitizer, getLView, getSelectedIndex, setCurrentStyleSanitizer, setElementExitFn} from '../state'; import {NO_CHANGE} from '../tokens'; import {renderStringify} from '../util/misc_utils'; import {getNativeByTNode, getTNode} from '../util/view_utils'; @@ -186,10 +186,7 @@ function stylingProp( value as string | SafeValue | null, sanitizer); } - if (updated) { - setElementExitFn(applyStyling); - } - markStylingStateAsDirty(); + setElementExitFn(stylingApply); } return updated; @@ -331,6 +328,9 @@ function _stylingMap( renderer, context, native, lView, bindingIndex, stylingMapArr as StylingMapArray, isClassBased ? setClass : setStyle, sanitizer, valueHasChanged); } else { + updated = valueHasChanged; + activateStylingMapFeature(); + // Context Resolution (or first update) Case: save the map value // and defer to the context to flush and apply the style/class binding // value to the element. @@ -344,15 +344,9 @@ function _stylingMap( valueHasChanged); } - if (valueHasChanged) { - updated = true; - setElementExitFn(applyStyling); - } - - activateStylingMapFeature(); + setElementExitFn(stylingApply); } - markStylingStateAsDirty(); return updated; } @@ -384,9 +378,9 @@ function updateDirectiveInputValue( const initialValue = getInitialStylingValue(context); const value = normalizeStylingDirectiveInputValue(initialValue, newValue, isClassBased); setInputsForProperty(lView, inputs, value); + setElementExitFn(stylingApply); } setValue(lView, bindingIndex, newValue); - setElementExitFn(applyStyling); } } @@ -423,17 +417,17 @@ function normalizeStylingDirectiveInputValue( * in this file. When called it will flush all style and class bindings to the element * via the context resolution algorithm. */ -function applyStyling(): void { - const elementIndex = getSelectedIndex(); +function stylingApply(): void { const lView = getLView(); + const elementIndex = getSelectedIndex(); const tNode = getTNode(elementIndex, lView); - const renderer = getRenderer(tNode, lView); const native = getNativeByTNode(tNode, lView) as RElement; + const directiveIndex = getActiveDirectiveId(); + const renderer = getRenderer(tNode, lView); const sanitizer = getCurrentStyleSanitizer(); const classesContext = isStylingContext(tNode.classes) ? tNode.classes as TStylingContext : null; const stylesContext = isStylingContext(tNode.styles) ? tNode.styles as TStylingContext : null; - flushStyling( - renderer, lView, classesContext, stylesContext, native, getActiveDirectiveId(), sanitizer); + flushStyling(renderer, lView, classesContext, stylesContext, native, directiveIndex, sanitizer); setCurrentStyleSanitizer(null); } @@ -538,10 +532,6 @@ function resolveStylePropValue( return resolvedValue; } -function markStylingStateAsDirty(): void { - setActiveElementFlag(ActiveElementFlags.ResetStylesOnExit); -} - /** * Whether or not the style/class binding being applied was executed within a host bindings * function. diff --git a/packages/core/test/acceptance/styling_next_spec.ts b/packages/core/test/acceptance/styling_next_spec.ts index 392b996974..fbc5f01d1c 100644 --- a/packages/core/test/acceptance/styling_next_spec.ts +++ b/packages/core/test/acceptance/styling_next_spec.ts @@ -5,7 +5,7 @@ * 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 {Component, Directive, HostBinding, Input, ViewChild} from '@angular/core'; +import {Component, ComponentFactoryResolver, ComponentRef, Directive, HostBinding, Input, NgModule, ViewChild, ViewContainerRef} from '@angular/core'; import {DebugNode, LViewDebug, toDebug} from '@angular/core/src/render3/instructions/lview_debug'; import {loadLContextFromNode} from '@angular/core/src/render3/util/discovery_utils'; import {ngDevModeResetPerfCounters as resetStylingCounters} from '@angular/core/src/util/ng_dev_mode'; @@ -1168,6 +1168,175 @@ describe('new styling integration', () => { expect(div.classList.contains('foo')).toBeTruthy(); expect(div.classList.contains('bar')).toBeTruthy(); }); + + it('should allow detectChanges to be run in a property change that causes additional styling to be rendered', + () => { + @Component({ + selector: 'child', + template: ` +
+ `, + }) + class ChildCmp { + readyTpl = false; + + @HostBinding('class.ready-host') + readyHost = false; + } + + @Component({ + selector: 'parent', + template: ` +
+
+

{{prop}}

+
+ `, + host: { + '[style.color]': 'color', + }, + }) + class ParentCmp { + private _prop = ''; + + @ViewChild('template', {read: ViewContainerRef, static: false}) + vcr: ViewContainerRef = null !; + + private child: ComponentRef = null !; + + @Input() + set prop(value: string) { + this._prop = value; + if (this.child && value === 'go') { + this.child.instance.readyHost = true; + this.child.instance.readyTpl = true; + this.child.changeDetectorRef.detectChanges(); + } + } + + get prop() { return this._prop; } + + ngAfterViewInit() { + const factory = this.componentFactoryResolver.resolveComponentFactory(ChildCmp); + this.child = this.vcr.createComponent(factory); + } + + constructor(private componentFactoryResolver: ComponentFactoryResolver) {} + } + + @Component({ + template: ``, + }) + class App { + prop = 'a'; + } + + @NgModule({ + entryComponents: [ChildCmp], + declarations: [ChildCmp], + }) + class ChildCmpModule { + } + + TestBed.configureTestingModule({declarations: [App, ParentCmp], imports: [ChildCmpModule]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(false); + + let readyHost = fixture.nativeElement.querySelector('.ready-host'); + let readyChild = fixture.nativeElement.querySelector('.ready-child'); + + expect(readyHost).toBeFalsy(); + expect(readyChild).toBeFalsy(); + + fixture.componentInstance.prop = 'go'; + fixture.detectChanges(false); + + readyHost = fixture.nativeElement.querySelector('.ready-host'); + readyChild = fixture.nativeElement.querySelector('.ready-child'); + expect(readyHost).toBeTruthy(); + expect(readyChild).toBeTruthy(); + }); + + it('should allow detectChanges to be run in a hook that causes additional styling to be rendered', + () => { + @Component({ + selector: 'child', + template: ` +
+ `, + }) + class ChildCmp { + readyTpl = false; + + @HostBinding('class.ready-host') + readyHost = false; + } + + @Component({ + selector: 'parent', + template: ` +
+
+

{{prop}}

+
+ `, + }) + class ParentCmp { + updateChild = false; + + @ViewChild('template', {read: ViewContainerRef, static: false}) + vcr: ViewContainerRef = null !; + + private child: ComponentRef = null !; + + ngDoCheck() { + if (this.updateChild) { + this.child.instance.readyHost = true; + this.child.instance.readyTpl = true; + this.child.changeDetectorRef.detectChanges(); + } + } + + ngAfterViewInit() { + const factory = this.componentFactoryResolver.resolveComponentFactory(ChildCmp); + this.child = this.vcr.createComponent(factory); + } + + constructor(private componentFactoryResolver: ComponentFactoryResolver) {} + } + + @Component({ + template: ``, + }) + class App { + @ViewChild('parent', {static: true}) public parent: ParentCmp|null = null; + } + + @NgModule({ + entryComponents: [ChildCmp], + declarations: [ChildCmp], + }) + class ChildCmpModule { + } + + TestBed.configureTestingModule({declarations: [App, ParentCmp], imports: [ChildCmpModule]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(false); + + let readyHost = fixture.nativeElement.querySelector('.ready-host'); + let readyChild = fixture.nativeElement.querySelector('.ready-child'); + expect(readyHost).toBeFalsy(); + expect(readyChild).toBeFalsy(); + + const parent = fixture.componentInstance.parent !; + parent.updateChild = true; + fixture.detectChanges(false); + + readyHost = fixture.nativeElement.querySelector('.ready-host'); + readyChild = fixture.nativeElement.querySelector('.ready-child'); + expect(readyHost).toBeTruthy(); + expect(readyChild).toBeTruthy(); + }); }); function assertStyleCounters(countForSet: number, countForRemove: number) { diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index f3a00560ea..11fcec25b5 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -167,9 +167,6 @@ { "name": "_selectedIndex" }, - { - "name": "_state" - }, { "name": "addComponentLogic" }, @@ -590,9 +587,6 @@ { "name": "resetPreOrderHookFlags" }, - { - "name": "resetStylingState" - }, { "name": "resolveDirectives" }, diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index 441806495d..11b96b90b7 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -146,9 +146,6 @@ { "name": "_selectedIndex" }, - { - "name": "_state" - }, { "name": "addToViewTree" }, @@ -434,9 +431,6 @@ { "name": "resetPreOrderHookFlags" }, - { - "name": "resetStylingState" - }, { "name": "selectIndexInternal" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index e617d0c535..3654a26b81 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -467,9 +467,6 @@ { "name": "applyProjectionRecursive" }, - { - "name": "applyStyling" - }, { "name": "applyStylingValue" }, @@ -914,9 +911,6 @@ { "name": "hasConfig" }, - { - "name": "hasDirectives" - }, { "name": "hasParentInjector" }, @@ -945,7 +939,7 @@ "name": "initNodeFlags" }, { - "name": "initializeTNodeInputs" + "name": "initializeInputAndOutputAliases" }, { "name": "injectElementRef" @@ -1106,9 +1100,6 @@ { "name": "markDirtyIfOnPush" }, - { - "name": "markStylingStateAsDirty" - }, { "name": "markViewDirty" }, @@ -1355,6 +1346,9 @@ { "name": "stringifyForError" }, + { + "name": "stylingApply" + }, { "name": "stylingMapToString" },