From 3861a73135ca9111c0ec10d52ee7db0a0e95f262 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 20 Oct 2023 11:33:37 -0700 Subject: [PATCH] perf(core): Update LView consumer to only mark component for check (#52302) This commit updates the reactive template and host binding consumers to only mark their declaration components for refresh, but not parents/ancestors. This also updates the `AfterViewChecked` hook to run when a component is refreshed during change detection but its host is not. It is reasonable to expect that the `ngAfterViewChecked` lifecycle hook will run when a signal updates and the component is refreshed. The hooks are typically run when the host is refreshed so without this change, the update to not mark ancestors dirty would have caused `ngAfterViewChecked` to not run. resolves #14628 resolves #22646 resolves #34347 - this is not the direct request of the issue but generally forcing change detection to run is necessary only because a value was updated that needs to be synced to the DOM. Values that use signals will mark the component for check automatically so accessing the `ChangeDetectorRef` of a child is not necessary. The other part of this request was to avoid the need to "mark all views for checking since it wouldn't affect anything but itself". This is directly addressed by this commit - updating a signal that's read in the view's template will not cause ancestors/"all views" to be refreshed. PR Close #52302 --- .../render3/instructions/change_detection.ts | 13 +- .../src/render3/reactive_lview_consumer.ts | 7 +- packages/core/src/render3/util/view_utils.ts | 21 +- .../change_detection_signals_in_zones_spec.ts | 318 +++++++++++++++- ...change_detection_transplanted_view_spec.ts | 360 +++++++++++------- 5 files changed, 573 insertions(+), 146 deletions(-) diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index 5e292d8c54770..f5f2ba5ca7728 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -13,7 +13,7 @@ import {getComponentViewByInstance} from '../context_discovery'; import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks'; import {CONTAINER_HEADER_OFFSET, HAS_CHILD_VIEWS_TO_REFRESH, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container'; import {ComponentTemplate, RenderFlags} from '../interfaces/definition'; -import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView} from '../interfaces/view'; +import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view'; import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state'; import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils'; import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils'; @@ -344,6 +344,7 @@ function detectChangesInViewIfAttached(lView: LView, mode: ChangeDetectionMode) * view HasChildViewsToRefresh flag is set. */ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) { + const isInCheckNoChangesPass = ngDevMode && isInCheckNoChangesMode(); const tView = lView[TVIEW]; const flags = lView[FLAGS]; @@ -351,8 +352,14 @@ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) { // necessary. lView[FLAGS] &= ~(LViewFlags.HasChildViewsToRefresh | LViewFlags.RefreshView); - if ((flags & (LViewFlags.CheckAlways | LViewFlags.Dirty) && - mode === ChangeDetectionMode.Global) || + if ((flags & LViewFlags.CheckAlways && mode === ChangeDetectionMode.Global) || + (flags & LViewFlags.Dirty && mode === ChangeDetectionMode.Global && + // CheckNoChanges never worked with `OnPush` components because the `Dirty` flag was cleared + // before checkNoChanges ran. Because there is now a loop for to check for backwards views, + // it gives an opportunity for `OnPush` components to be marked `Dirty` before the + // CheckNoChanges pass. We don't want existing errors that are hidden by the current + // CheckNoChanges bug to surface when making unrelated changes. + !isInCheckNoChangesPass) || flags & LViewFlags.RefreshView) { refreshView(tView, lView, tView.template, lView[CONTEXT]); } else if (flags & LViewFlags.HasChildViewsToRefresh) { diff --git a/packages/core/src/render3/reactive_lview_consumer.ts b/packages/core/src/render3/reactive_lview_consumer.ts index ed7cdedc15aa2..2bb276a3de632 100644 --- a/packages/core/src/render3/reactive_lview_consumer.ts +++ b/packages/core/src/render3/reactive_lview_consumer.ts @@ -8,11 +8,8 @@ import {REACTIVE_NODE, ReactiveNode} from '@angular/core/primitives/signals'; -import {RuntimeError} from '../errors'; -import {assertDefined} from '../util/assert'; - -import {markViewDirty} from './instructions/mark_view_dirty'; import {LView, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view'; +import {markViewDirtyFromSignal} from './util/view_utils'; let currentConsumer: ReactiveLViewConsumer|null = null; export interface ReactiveLViewConsumer extends ReactiveNode { @@ -43,7 +40,7 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit ` in ExpressionChangedAfterItHasBeenChecked errors or future updates not working` + ` entirely.`); } - markViewDirty(node.lView); + markViewDirtyFromSignal(node.lView); }, consumerOnSignalRead(this: ReactiveLViewConsumer): void { if (currentConsumer !== this) { diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts index 8e792824f4b16..da88a183d1153 100644 --- a/packages/core/src/render3/util/view_utils.ts +++ b/packages/core/src/render3/util/view_utils.ts @@ -13,7 +13,7 @@ import {HAS_CHILD_VIEWS_TO_REFRESH, LContainer, TYPE} from '../interfaces/contai import {TConstants, TNode} from '../interfaces/node'; import {RNode} from '../interfaces/renderer_dom'; import {isLContainer, isLView} from '../interfaces/type_checks'; -import {DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view'; +import {DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view'; @@ -241,6 +241,25 @@ export function markAncestorsForTraversal(lView: LView) { } } +/** + * Marks the component or root view of an LView for refresh. + * + * This function locates the declaration component view of a given LView and marks it for refresh. + * With this, we get component-level change detection granularity. Marking the `LView` itself for + * refresh would be view-level granularity. + * + * Note that when an LView is a root view, the DECLARATION_COMPONENT_VIEW will be the root view + * itself. This is a bit confusing since the TView.type is `Root`, rather than `Component`, but this + * is actually what we need for host bindings in a root view. + */ +export function markViewDirtyFromSignal(lView: LView): void { + const declarationComponentView = lView[DECLARATION_COMPONENT_VIEW]; + declarationComponentView[FLAGS] |= LViewFlags.RefreshView; + if (viewAttachedToChangeDetector(declarationComponentView)) { + markAncestorsForTraversal(declarationComponentView); + } +} + /** * Stores a LView-specific destroy callback. */ diff --git a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts index 7595ee41107ff..6fe555c4e8e1c 100644 --- a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts +++ b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts @@ -6,8 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import {NgIf} from '@angular/common'; -import {ChangeDetectionStrategy, Component, Directive, Input, signal, ViewChild} from '@angular/core'; +import {NgFor, NgIf} from '@angular/common'; +import {ChangeDetectionStrategy, ChangeDetectorRef, Component, Directive, Input, signal, untracked, ViewChild} from '@angular/core'; import {TestBed} from '@angular/core/testing'; describe('CheckAlways components', () => { @@ -88,6 +88,87 @@ describe('CheckAlways components', () => { fixture.detectChanges(); expect(fixture.nativeElement.textContent.trim()).toBe('new'); }); + + it('continues to refresh views until none are dirty', () => { + const aVal = signal('initial'); + const bVal = signal('initial'); + let updateAValDuringAChangeDetection = false; + + @Component({ + template: '{{val()}}', + standalone: true, + selector: 'a-comp', + }) + class A { + val = aVal; + } + @Component({ + template: '{{val()}}', + standalone: true, + selector: 'b-comp', + }) + class B { + val = bVal; + ngAfterViewChecked() { + // Set value in parent view after this view is checked + // Without signals, this is ExpressionChangedAfterItWasChecked + if (updateAValDuringAChangeDetection) { + aVal.set('new'); + } + } + } + + @Component({template: '-', standalone: true, imports: [A, B]}) + class App { + } + + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toContain('initial-initial'); + + bVal.set('new'); + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toContain('initial-new'); + + updateAValDuringAChangeDetection = true; + bVal.set('newer'); + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toContain('new-newer'); + }); + + it('refreshes root view until it is no longer dirty', () => { + const val = signal(0); + let incrementAfterCheckedUntil = 0; + @Component({ + template: '', + selector: 'child', + standalone: true, + }) + class Child { + ngDoCheck() { + // Update signal in parent view every time we check the child view + // (ExpressionChangedAfterItWasCheckedError but not for signals) + if (val() < incrementAfterCheckedUntil) { + val.update(v => ++v); + } + } + } + @Component({template: '{{val()}}', standalone: true, imports: [Child]}) + class App { + val = val; + } + + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toContain('0'); + + incrementAfterCheckedUntil = 10; + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toContain('10'); + + incrementAfterCheckedUntil = Number.MAX_SAFE_INTEGER; + expect(() => fixture.detectChanges()).toThrowError(/Infinite/); + }); }); @@ -272,6 +353,37 @@ describe('OnPush components with signals', () => { expect(instance.numTemplateExecutions).toBe(1); }); + it('can read a signal in a host binding in root view', () => { + const useBlue = signal(false); + @Component({ + template: `{{incrementTemplateExecutions()}}`, + selector: 'child', + host: {'[class.blue]': 'useBlue()'}, + changeDetection: ChangeDetectionStrategy.OnPush, + standalone: true, + }) + class MyCmp { + useBlue = useBlue; + + numTemplateExecutions = 0; + incrementTemplateExecutions() { + this.numTemplateExecutions++; + return ''; + } + } + + const fixture = TestBed.createComponent(MyCmp); + + fixture.detectChanges(); + expect(fixture.nativeElement.outerHTML).not.toContain('blue'); + expect(fixture.componentInstance.numTemplateExecutions).not.toContain(1); + + useBlue.set(true); + fixture.detectChanges(); + expect(fixture.nativeElement.outerHTML).toContain('blue'); + expect(fixture.componentInstance.numTemplateExecutions).not.toContain(1); + }); + it('can read a signal in a host binding', () => { @Component({ template: `{{incrementTemplateExecutions()}}`, @@ -440,4 +552,206 @@ describe('OnPush components with signals', () => { .toHaveBeenCalledWith(jasmine.stringMatching( /will likely result in ExpressionChangedAfterItHasBeenChecked/)); }); + + it('does not refresh view if signal marked dirty but did not change', () => { + const val = signal('initial', {equal: () => true}); + + @Component({ + template: '{{val()}}{{incrementChecks()}}', + standalone: true, + changeDetection: ChangeDetectionStrategy.OnPush, + }) + class App { + val = val; + templateExecutions = 0; + incrementChecks() { + this.templateExecutions++; + } + } + + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + expect(fixture.componentInstance.templateExecutions).toBe(1); + expect(fixture.nativeElement.innerText).toContain('initial'); + + val.set('new'); + fixture.detectChanges(); + expect(fixture.componentInstance.templateExecutions).toBe(1); + expect(fixture.nativeElement.innerText).toContain('initial'); + }); + + describe('embedded views', () => { + it('refreshes an embedded view in a component', () => { + @Component({ + selector: 'signal-component', + changeDetection: ChangeDetectionStrategy.OnPush, + standalone: true, + imports: [NgIf], + template: `
{{value()}}
`, + }) + class SignalComponent { + value = signal('initial'); + } + + const fixture = TestBed.createComponent(SignalComponent); + fixture.detectChanges(); + fixture.componentInstance.value.set('new'); + fixture.detectChanges(); + expect(trim(fixture.nativeElement.textContent)).toEqual('new'); + }); + + it('refreshes multiple embedded views in a component', () => { + @Component({ + selector: 'signal-component', + changeDetection: ChangeDetectionStrategy.OnPush, + standalone: true, + imports: [NgFor], + template: `
{{value()}}
`, + }) + class SignalComponent { + value = signal('initial'); + } + + const fixture = TestBed.createComponent(SignalComponent); + fixture.detectChanges(); + fixture.componentInstance.value.set('new'); + fixture.detectChanges(); + expect(trim(fixture.nativeElement.textContent)).toEqual('new new new'); + }); + + + it('refreshes entire component, including embedded views, when signal updates', () => { + @Component({ + selector: 'signal-component', + changeDetection: ChangeDetectionStrategy.OnPush, + standalone: true, + imports: [NgIf], + template: ` + {{componentSignal()}} +
{{incrementExecutions()}}
+ `, + }) + class SignalComponent { + embeddedViewExecutions = 0; + componentSignal = signal('initial'); + incrementExecutions() { + this.embeddedViewExecutions++; + return ''; + } + } + + const fixture = TestBed.createComponent(SignalComponent); + fixture.detectChanges(); + expect(fixture.componentInstance.embeddedViewExecutions).toEqual(1); + + fixture.componentInstance.componentSignal.set('new'); + fixture.detectChanges(); + expect(trim(fixture.nativeElement.textContent)).toEqual('new'); + // OnPush/Default components are checked as a whole so the embedded view is also checked again + expect(fixture.componentInstance.embeddedViewExecutions).toEqual(2); + }); + + + it('re-executes deep embedded template if signal updates', () => { + @Component({ + selector: 'signal-component', + standalone: true, + changeDetection: ChangeDetectionStrategy.OnPush, + imports: [NgIf], + template: ` +
+
+
+ {{value()}} +
+
+
+ `, + }) + class SignalComponent { + value = signal('initial'); + } + + const fixture = TestBed.createComponent(SignalComponent); + fixture.detectChanges(); + + fixture.componentInstance.value.set('new'); + fixture.detectChanges(); + expect(trim(fixture.nativeElement.textContent)).toEqual('new'); + }); + }); + + describe('shielded by non-dirty OnPush', () => { + @Component({ + selector: 'signal-component', + changeDetection: ChangeDetectionStrategy.OnPush, + standalone: true, + template: `{{value()}}`, + }) + class SignalComponent { + value = signal('initial'); + afterViewCheckedRuns = 0; + constructor(readonly cdr: ChangeDetectorRef) {} + ngAfterViewChecked() { + this.afterViewCheckedRuns++; + } + } + + @Component({ + selector: 'on-push-parent', + template: `{{incrementChecks()}}`, + changeDetection: ChangeDetectionStrategy.OnPush, + standalone: true, + imports: [SignalComponent], + }) + class OnPushParent { + @ViewChild(SignalComponent) signalChild!: SignalComponent; + viewExecutions = 0; + + constructor(readonly cdr: ChangeDetectorRef) {} + incrementChecks() { + this.viewExecutions++; + } + } + + it('refreshes when signal changes, but does not refresh non-dirty parent', () => { + const fixture = TestBed.createComponent(OnPushParent); + fixture.detectChanges(); + expect(fixture.componentInstance.viewExecutions).toEqual(1); + fixture.componentInstance.signalChild.value.set('new'); + fixture.detectChanges(); + expect(fixture.componentInstance.viewExecutions).toEqual(1); + expect(trim(fixture.nativeElement.textContent)).toEqual('new'); + }); + + it('does not refresh when detached', () => { + const fixture = TestBed.createComponent(OnPushParent); + fixture.detectChanges(); + fixture.componentInstance.signalChild.value.set('new'); + fixture.componentInstance.signalChild.cdr.detach(); + fixture.detectChanges(); + expect(trim(fixture.nativeElement.textContent)).toEqual('initial'); + }); + + // Note: Design decision for signals because that's how the hooks work today + // We have considered actually running a component's `afterViewChecked` hook if it's refreshed + // in targeted mode (meaning the parent did not refresh) and could change this decision. + it('does not run afterViewChecked hooks because parent view was not dirty (those hooks are executed by the parent)', + () => { + const fixture = TestBed.createComponent(OnPushParent); + fixture.detectChanges(); + // hook run once on initialization + expect(fixture.componentInstance.signalChild.afterViewCheckedRuns).toBe(1); + fixture.componentInstance.signalChild.value.set('new'); + fixture.detectChanges(); + expect(trim(fixture.nativeElement.textContent)).toEqual('new'); + // hook did not run again because host view was not refreshed + expect(fixture.componentInstance.signalChild.afterViewCheckedRuns).toBe(1); + }); + }); }); + + +function trim(text: string|null): string { + return text ? text.replace(/[\s\n]+/gm, ' ').trim() : ''; +} diff --git a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts index 600b92b2f1871..51c8c0db8d94c 100644 --- a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts +++ b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts @@ -7,14 +7,18 @@ */ import {CommonModule} from '@angular/common'; -import {ChangeDetectionStrategy, ChangeDetectorRef, Component, DoCheck, inject, Input, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core'; +import {ChangeDetectionStrategy, ChangeDetectorRef, Component, computed, Directive, DoCheck, inject, Input, signal, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core'; import {AfterViewChecked, EmbeddedViewRef} from '@angular/core/src/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; +import {expect} from '@angular/platform-browser/testing/src/matchers'; describe('change detection for transplanted views', () => { describe('when declaration appears before insertion', () => { - const insertCompTemplate = ` - InsertComp({{greeting}}) + @Component({ + selector: 'onpush-insert-comp', + changeDetection: ChangeDetectionStrategy.OnPush, + template: ` + OnPushInsertComp({{greeting}})
{ [ngTemplateOutletContext]="{$implicit: greeting}">
- `; - @Component({ - selector: 'insert-comp', - changeDetection: ChangeDetectionStrategy.OnPush, - template: insertCompTemplate, + `, }) - class InsertComp implements DoCheck, AfterViewChecked { + abstract class OnPushInsertComp implements DoCheck, AfterViewChecked { get template(): TemplateRef { - return declareComp.myTmpl; + return templateRef; } greeting: string = 'Hello'; constructor(public changeDetectorRef: ChangeDetectorRef) { - if (!(this instanceof InsertForOnPushDeclareComp)) { - insertComp = this; - } + onPushInsertComp = this; } ngDoCheck(): void { logValue = 'Insert'; @@ -46,50 +44,42 @@ describe('change detection for transplanted views', () => { } } - @Component({ - selector: 'insert-for-onpush-declare-comp', - changeDetection: ChangeDetectionStrategy.OnPush, - template: insertCompTemplate, - }) - class InsertForOnPushDeclareComp extends InsertComp { - constructor(changeDetectorRef: ChangeDetectorRef) { - super(changeDetectorRef); - insertForOnPushDeclareComp = this; - } - override get template(): TemplateRef { - return onPushDeclareComp.myTmpl; - } - } - - @Component({ - selector: `declare-comp`, - template: ` - DeclareComp({{name}}) - - {{greeting}} {{logName()}}! - - ` - }) - class DeclareComp implements DoCheck, AfterViewChecked { + @Directive({}) + abstract class DeclareComp implements DoCheck, AfterViewChecked { @ViewChild('myTmpl') myTmpl!: TemplateRef; name: string = 'world'; - constructor(readonly changeDetector: ChangeDetectorRef) { - if (!(this instanceof OnPushDeclareComp)) { - declareComp = this; - } - } + constructor(readonly changeDetector: ChangeDetectorRef) {} ngDoCheck(): void { logValue = 'Declare'; } logName() { // This will log when the embedded view gets CD. The `logValue` will show if the CD was // from `Insert` or from `Declare` component. - log.push(logValue!); + viewExecutionLog.push(logValue!); return this.name; } ngAfterViewChecked(): void { logValue = null; } + ngAfterViewInit() { + templateRef = this.myTmpl; + } + } + + @Component({ + selector: `check-always-declare-comp`, + template: ` + DeclareComp({{name}}) + + {{greeting}} {{logName()}}! + + ` + }) + class CheckAlwaysDeclareComp extends DeclareComp { + constructor(changeDetector: ChangeDetectorRef) { + super(changeDetector); + declareComp = this; + } } @Component({ @@ -98,8 +88,7 @@ describe('change detection for transplanted views', () => { OnPushDeclareComp({{name}}) {{greeting}} {{logName()}}! - - `, + `, changeDetection: ChangeDetectionStrategy.OnPush }) class OnPushDeclareComp extends DeclareComp { @@ -109,180 +98,280 @@ describe('change detection for transplanted views', () => { } } + @Component({ + selector: `signal-onpush-declare-comp`, + template: ` + SignalOnPushDeclareComp({{name()}}) + + {{greeting}} {{surname()}}{{logExecutionContext()}}! + + `, + changeDetection: ChangeDetectionStrategy.OnPush + }) + class SignalOnPushDeclareComp { + @ViewChild('myTmpl') myTmpl!: TemplateRef; + + name = signal('world'); + templateName = signal('templateName'); + + surname = computed(() => { + const name = this.templateName(); + return name; + }); + + logExecutionContext() { + viewExecutionLog.push(logValue); + return ''; + } + + constructor() { + signalDeclareComp = this; + } + + ngAfterViewChecked() { + logValue = null; + } + ngAfterViewInit() { + templateRef = this.myTmpl; + } + } @Component({ template: ` - - - - + + + + + ` }) class AppComp { - showDeclare: boolean = false; - showOnPushDeclare: boolean = false; - showInsert: boolean = false; - showInsertForOnPushDeclare: boolean = false; + showCheckAlwaysDeclare = false; + showSignalOnPushDeclare = false; + showOnPushDeclare = false; + showOnPushInsert = false; constructor() { appComp = this; } } - let log!: Array; + let viewExecutionLog!: Array; let logValue!: string|null; let fixture!: ComponentFixture; let appComp!: AppComp; - let insertComp!: InsertComp; - let insertForOnPushDeclareComp!: InsertForOnPushDeclareComp; - let declareComp!: DeclareComp; + let onPushInsertComp!: OnPushInsertComp; + let declareComp!: CheckAlwaysDeclareComp; + let templateRef: TemplateRef; let onPushDeclareComp!: OnPushDeclareComp; + let signalDeclareComp!: SignalOnPushDeclareComp; beforeEach(() => { TestBed.configureTestingModule({ - declarations: - [InsertComp, DeclareComp, OnPushDeclareComp, InsertForOnPushDeclareComp, AppComp], + declarations: [ + OnPushInsertComp, SignalOnPushDeclareComp, CheckAlwaysDeclareComp, OnPushDeclareComp, + AppComp + ], imports: [CommonModule], }); - log = []; + viewExecutionLog = []; fixture = TestBed.createComponent(AppComp); }); + describe('and declaration component is Onpush with signals and insertion is OnPush', () => { + beforeEach(() => { + fixture.componentInstance.showSignalOnPushDeclare = true; + fixture.componentInstance.showOnPushInsert = true; + fixture.detectChanges(false); + viewExecutionLog.length = 0; + }); + + it('should set up the component under test correctly', () => { + expect(viewExecutionLog.length).toEqual(0); + expect(trim(fixture.nativeElement.textContent)) + .toEqual('SignalOnPushDeclareComp(world) OnPushInsertComp(Hello) Hello templateName!'); + }); + + it('should CD at insertion and declaration', () => { + signalDeclareComp.name.set('Angular'); + fixture.detectChanges(false); + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .withContext( + 'CD did not run on the transplanted template because it is inside an OnPush component and no signal changed') + .toEqual( + 'SignalOnPushDeclareComp(Angular) OnPushInsertComp(Hello) Hello templateName!'); + + onPushInsertComp.greeting = 'Hi'; + fixture.detectChanges(false); + expect(viewExecutionLog).toEqual([]); + viewExecutionLog.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .withContext('Insertion component is OnPush.') + .toEqual( + 'SignalOnPushDeclareComp(Angular) OnPushInsertComp(Hello) Hello templateName!'); + + onPushInsertComp.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('SignalOnPushDeclareComp(Angular) OnPushInsertComp(Hi) Hi templateName!'); + + // Destroy insertion should also destroy declaration + appComp.showOnPushInsert = false; + fixture.detectChanges(false); + expect(viewExecutionLog).toEqual([]); + viewExecutionLog.length = 0; + expect(trim(fixture.nativeElement.textContent)).toEqual('SignalOnPushDeclareComp(Angular)'); + + // Restore both + appComp.showOnPushInsert = true; + fixture.detectChanges(false); + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual( + 'SignalOnPushDeclareComp(Angular) OnPushInsertComp(Hello) Hello templateName!'); + }); + }); + describe('and declaration component is CheckAlways', () => { beforeEach(() => { - fixture.componentInstance.showDeclare = true; - fixture.componentInstance.showInsert = true; + fixture.componentInstance.showCheckAlwaysDeclare = true; + fixture.componentInstance.showOnPushInsert = true; fixture.detectChanges(false); - log.length = 0; + viewExecutionLog.length = 0; }); it('should set up the component under test correctly', () => { - expect(log.length).toEqual(0); + expect(viewExecutionLog.length).toEqual(0); expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(world) InsertComp(Hello) Hello world!'); + .toEqual('DeclareComp(world) OnPushInsertComp(Hello) Hello world!'); }); it('should CD at insertion point only', () => { declareComp.name = 'Angular'; fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; expect(trim(fixture.nativeElement.textContent)) .toEqual( - 'DeclareComp(Angular) InsertComp(Hello) Hello Angular!', + 'DeclareComp(Angular) OnPushInsertComp(Hello) Hello Angular!', 'Expect transplanted LView to be CD because the declaration is CD.'); - insertComp.greeting = 'Hi'; + onPushInsertComp.greeting = 'Hi'; fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; expect(trim(fixture.nativeElement.textContent)) .toEqual( - 'DeclareComp(Angular) InsertComp(Hello) Hello Angular!', + 'DeclareComp(Angular) OnPushInsertComp(Hello) Hello Angular!', 'expect no change because it is on push.'); - insertComp.changeDetectorRef.markForCheck(); + onPushInsertComp.changeDetectorRef.markForCheck(); fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(Angular) InsertComp(Hi) Hi Angular!'); + .toEqual('DeclareComp(Angular) OnPushInsertComp(Hi) Hi Angular!'); // Destroy insertion should also destroy declaration - appComp.showInsert = false; + appComp.showOnPushInsert = false; fixture.detectChanges(false); - expect(log).toEqual([]); - log.length = 0; + expect(viewExecutionLog).toEqual([]); + viewExecutionLog.length = 0; expect(trim(fixture.nativeElement.textContent)).toEqual('DeclareComp(Angular)'); // Restore both - appComp.showInsert = true; + appComp.showOnPushInsert = true; fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); + .toEqual('DeclareComp(Angular) OnPushInsertComp(Hello) Hello Angular!'); // Destroy declaration, But we should still be able to see updates in insertion - appComp.showDeclare = false; - insertComp.greeting = 'Hello'; - insertComp.changeDetectorRef.markForCheck(); + appComp.showCheckAlwaysDeclare = false; + onPushInsertComp.greeting = 'Hello'; + onPushInsertComp.changeDetectorRef.markForCheck(); fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; - expect(trim(fixture.nativeElement.textContent)).toEqual('InsertComp(Hello) Hello Angular!'); + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('OnPushInsertComp(Hello) Hello Angular!'); }); it('is not checked if detectChanges is called in declaration component', () => { declareComp.name = 'Angular'; declareComp.changeDetector.detectChanges(); - expect(log).toEqual([]); - log.length = 0; + expect(viewExecutionLog).toEqual([]); + viewExecutionLog.length = 0; expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello world!'); + .toEqual('DeclareComp(Angular) OnPushInsertComp(Hello) Hello world!'); }); it('is checked as part of CheckNoChanges pass', () => { fixture.detectChanges(true); - expect(log).toEqual(['Insert', null /* logName set to null afterViewChecked */]); - log.length = 0; + expect(viewExecutionLog) + .toEqual(['Insert', null /* logName set to null afterViewChecked */]); + viewExecutionLog.length = 0; expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(world) InsertComp(Hello) Hello world!'); + .toEqual('DeclareComp(world) OnPushInsertComp(Hello) Hello world!'); }); }); - describe('and declaration component is OnPush', () => { + describe('and declaration and insertion components are OnPush', () => { beforeEach(() => { fixture.componentInstance.showOnPushDeclare = true; - fixture.componentInstance.showInsertForOnPushDeclare = true; + fixture.componentInstance.showOnPushInsert = true; fixture.detectChanges(false); - log.length = 0; + viewExecutionLog.length = 0; }); it('should set up component under test correctly', () => { - expect(log.length).toEqual(0); + expect(viewExecutionLog.length).toEqual(0); expect(trim(fixture.nativeElement.textContent)) - .toEqual('OnPushDeclareComp(world) InsertComp(Hello) Hello world!'); + .toEqual('OnPushDeclareComp(world) OnPushInsertComp(Hello) Hello world!'); }); - it('should not check anything no views are dirty', () => { + it('should not check anything when no views are dirty', () => { fixture.detectChanges(false); - expect(log).toEqual([]); + expect(viewExecutionLog).toEqual([]); }); it('should CD at insertion point only', () => { onPushDeclareComp.name = 'Angular'; - insertForOnPushDeclareComp.greeting = 'Hi'; + onPushInsertComp.greeting = 'Hi'; // mark declaration point dirty onPushDeclareComp.changeDetector.markForCheck(); fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; expect(trim(fixture.nativeElement.textContent)) - .toEqual('OnPushDeclareComp(Angular) InsertComp(Hello) Hello Angular!'); + .toEqual('OnPushDeclareComp(Angular) OnPushInsertComp(Hello) Hello Angular!'); // mark insertion point dirty - insertForOnPushDeclareComp.changeDetectorRef.markForCheck(); + onPushInsertComp.changeDetectorRef.markForCheck(); fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; expect(trim(fixture.nativeElement.textContent)) - .toEqual('OnPushDeclareComp(Angular) InsertComp(Hi) Hi Angular!'); + .toEqual('OnPushDeclareComp(Angular) OnPushInsertComp(Hi) Hi Angular!'); // mark both insertion and declaration point dirty - insertForOnPushDeclareComp.changeDetectorRef.markForCheck(); + onPushInsertComp.changeDetectorRef.markForCheck(); onPushDeclareComp.changeDetector.markForCheck(); fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; + expect(viewExecutionLog).toEqual(['Insert']); + viewExecutionLog.length = 0; }); - it('is not checked if detectChanges is called in declaration component', () => { + it('is checked if detectChanges is called in declaration component', () => { onPushDeclareComp.name = 'Angular'; onPushDeclareComp.changeDetector.detectChanges(); - expect(log).toEqual([]); - log.length = 0; expect(trim(fixture.nativeElement.textContent)) - .toEqual('OnPushDeclareComp(Angular) InsertComp(Hello) Hello world!'); + .toEqual('OnPushDeclareComp(Angular) OnPushInsertComp(Hello) Hello world!'); }); // TODO(FW-1774): blocked by https://github.com/angular/angular/pull/34443 @@ -290,21 +379,22 @@ describe('change detection for transplanted views', () => { // mark declaration point dirty onPushDeclareComp.changeDetector.markForCheck(); fixture.detectChanges(false); - expect(log).toEqual(['Insert', null /* logName set to null in afterViewChecked */]); - log.length = 0; + expect(viewExecutionLog) + .toEqual(['Insert', null /* logName set to null in afterViewChecked */]); + viewExecutionLog.length = 0; // mark insertion point dirty - insertForOnPushDeclareComp.changeDetectorRef.markForCheck(); + onPushInsertComp.changeDetectorRef.markForCheck(); fixture.detectChanges(false); - expect(log).toEqual(['Insert', null]); - log.length = 0; + expect(viewExecutionLog).toEqual(['Insert', null]); + viewExecutionLog.length = 0; // mark both insertion and declaration point dirty - insertForOnPushDeclareComp.changeDetectorRef.markForCheck(); + onPushInsertComp.changeDetectorRef.markForCheck(); onPushDeclareComp.changeDetector.markForCheck(); fixture.detectChanges(false); - expect(log).toEqual(['Insert', null]); - log.length = 0; + expect(viewExecutionLog).toEqual(['Insert', null]); + viewExecutionLog.length = 0; }); it('does not cause infinite change detection if transplanted view is dirty and destroyed before refresh', @@ -312,13 +402,13 @@ describe('change detection for transplanted views', () => { // mark declaration point dirty onPushDeclareComp.changeDetector.markForCheck(); // detach insertion so the transplanted view doesn't get refreshed when CD runs - insertForOnPushDeclareComp.changeDetectorRef.detach(); + onPushInsertComp.changeDetectorRef.detach(); // run CD, which will set the `RefreshView` flag on the transplanted view fixture.detectChanges(false); // reattach insertion so the DESCENDANT_VIEWS counters update - insertForOnPushDeclareComp.changeDetectorRef.reattach(); + onPushInsertComp.changeDetectorRef.reattach(); // make it so the insertion is destroyed before getting refreshed - fixture.componentInstance.showInsertForOnPushDeclare = false; + fixture.componentInstance.showOnPushInsert = false; // run CD again. If we didn't clear the flag/counters when destroying the view, this // would cause an infinite CD because the counters will be >1 but we will never reach a // view to refresh and decrement the counters. @@ -614,9 +704,9 @@ describe('change detection for transplanted views', () => { constructor( readonly rootViewContainerRef: ViewContainerRef, readonly cdr: ChangeDetectorRef) {} - templateExecutions = 0; + checks = 0; incrementChecks() { - this.templateExecutions++; + this.checks++; } } @@ -639,13 +729,13 @@ describe('change detection for transplanted views', () => { component.cdr.detectChanges(); // The template should not have been refreshed because it was inserted "above" the component // so `detectChanges` will not refresh it. - expect(component.templateExecutions).toEqual(0); + expect(component.checks).toEqual(0); // Detach view, manually call `detectChanges`, and verify the template was refreshed component.rootViewContainerRef.detach(); viewRef.detectChanges(); // This view is a backwards reference so it's refreshed twice - expect(component.templateExecutions).toEqual(2); + expect(component.checks).toEqual(2); }); it('should work when change detecting detached transplanted view already marked for refresh', @@ -662,7 +752,7 @@ describe('change detection for transplanted views', () => { viewRef.detectChanges(); }).not.toThrow(); // This view is a backwards reference so it's refreshed twice - expect(component.templateExecutions).toEqual(2); + expect(component.checks).toEqual(2); }); it('should work when re-inserting a previously detached transplanted view marked for refresh', @@ -685,7 +775,7 @@ describe('change detection for transplanted views', () => { // The transplanted view gets refreshed twice because it's actually inserted "backwards" // The view is defined in AppComponent but inserted in its ViewContainerRef (as an // embedded view in AppComponent's host view). - expect(component.templateExecutions).toEqual(2); + expect(component.checks).toEqual(2); }); it('should work when detaching an attached transplanted view with the refresh flag', () => {