From 8de3c20e5099906194d6530ed297ba53516fddbf Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 11 Jan 2020 16:23:11 +0100 Subject: [PATCH] fix(ivy): do not reset view dirty state in check no changes mode (#34495) Unlike in View Engine, we currently reset the dirty state of components in the check no changes change detection cycle. This means that components cannot be marked as dirty from view lifecycle hooks because the dirty state is reset and the lifecycle hooks do not run in the check no changes CD cycle. PR Close #34495 --- .../core/src/render3/instructions/shared.ts | 16 ++- .../test/acceptance/change_detection_spec.ts | 103 +++++++++++++++++- .../core/test/acceptance/integration_spec.ts | 1 - 3 files changed, 114 insertions(+), 6 deletions(-) diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index d5d7149930..fb57f88181 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -384,6 +384,7 @@ export function refreshView( const flags = lView[FLAGS]; if ((flags & LViewFlags.Destroyed) === LViewFlags.Destroyed) return; enterView(lView, lView[T_HOST]); + const checkNoChangesMode = getCheckNoChangesMode(); try { resetPreOrderHookFlags(lView); @@ -392,11 +393,10 @@ export function refreshView( executeTemplate(lView, templateFn, RenderFlags.Update, context); } - const checkNoChangesMode = getCheckNoChangesMode(); const hooksInitPhaseCompleted = (flags & LViewFlags.InitPhaseStateMask) === InitPhaseState.InitPhaseCompleted; - // execute pre-order hooks (OnInit, OnChanges, DoChanges) + // execute pre-order hooks (OnInit, OnChanges, DoCheck) // PERF WARNING: do NOT extract this to a separate function without running benchmarks if (!checkNoChangesMode) { if (hooksInitPhaseCompleted) { @@ -475,7 +475,17 @@ export function refreshView( if (tView.firstUpdatePass === true) { tView.firstUpdatePass = false; } - lView[FLAGS] &= ~(LViewFlags.Dirty | LViewFlags.FirstLViewPass); + + // Do not reset the dirty state when running in check no changes mode. We don't want components + // to behave differently depending on whether check no changes is enabled or not. For example: + // Marking an OnPush component as dirty from within the `ngAfterViewInit` hook in order to + // refresh a `NgClass` binding should work. If we would reset the dirty state in the check + // no changes cycle, the component would be not be dirty for the next update pass. This would + // be different in production mode where the component dirty state is not reset. + if (!checkNoChangesMode) { + lView[FLAGS] &= ~(LViewFlags.Dirty | LViewFlags.FirstLViewPass); + } + leaveViewProcessExit(); } } diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts index 10da2521b4..8f7216e803 100644 --- a/packages/core/test/acceptance/change_detection_spec.ts +++ b/packages/core/test/acceptance/change_detection_spec.ts @@ -9,10 +9,10 @@ import {CommonModule} from '@angular/common'; import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentFactoryResolver, ComponentRef, Directive, DoCheck, EmbeddedViewRef, ErrorHandler, Input, NgModule, OnInit, QueryList, TemplateRef, Type, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; -import {AfterContentChecked, AfterViewChecked} from '@angular/core/src/core'; +import {AfterViewChecked} from '@angular/core/src/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; -import {ivyEnabled, onlyInIvy} from '@angular/private/testing'; +import {ivyEnabled} from '@angular/private/testing'; import {BehaviorSubject} from 'rxjs'; describe('change detection', () => { @@ -1249,6 +1249,105 @@ describe('change detection', () => { }); }); + describe('OnPush markForCheck in lifecycle hooks', () => { + describe('with check no changes enabled', () => createOnPushMarkForCheckTests(true)); + describe('with check no changes disabled', () => createOnPushMarkForCheckTests(false)); + + function createOnPushMarkForCheckTests(checkNoChanges: boolean) { + const detectChanges = (f: ComponentFixture) => f.detectChanges(checkNoChanges); + + // 1. ngAfterViewInit and ngAfterViewChecked lifecycle hooks run after "OnPushComp" has + // been refreshed. They can mark the component as dirty. Meaning that the "OnPushComp" + // can be checked/refreshed in a subsequent change detection cycle. + // 2. ngDoCheck and ngAfterContentChecked lifecycle hooks run before "OnPushComp" is + // refreshed. This means that those hooks cannot leave the component as dirty because + // the dirty state is reset afterwards. Though these hooks run every change detection + // cycle before "OnPushComp" is considered for refreshing. Hence marking as dirty from + // within such a hook can cause the component to checked/refreshed as intended. + ['ngAfterViewInit', 'ngAfterViewChecked', 'ngAfterContentChecked', 'ngDoCheck'].forEach( + hookName => { + it(`should be able to mark component as dirty from within ${hookName}`, () => { + @Component({ + selector: 'on-push-comp', + changeDetection: ChangeDetectionStrategy.OnPush, + template: `

{{text}}

`, + }) + class OnPushComp { + text = 'initial'; + + constructor(private _cdRef: ChangeDetectorRef){} + + [hookName]() { + this._cdRef.markForCheck(); + } + } + + @Component({template: ``}) + class TestApp { + @ViewChild(OnPushComp) onPushComp !: OnPushComp; + } + + TestBed.configureTestingModule( + {declarations: [TestApp, OnPushComp], imports: [CommonModule]}); + const fixture = TestBed.createComponent(TestApp); + const pElement = fixture.nativeElement.querySelector('p') as HTMLElement; + + detectChanges(fixture); + expect(pElement.textContent).toBe('initial'); + + // "OnPushComp" component should be re-checked since it has been left dirty + // in the first change detection (through the lifecycle hook). Hence, setting + // a programmatic value and triggering a new change detection cycle should cause + // the text to be updated in the view. + fixture.componentInstance.onPushComp.text = 'new'; + detectChanges(fixture); + expect(pElement.textContent).toBe('new'); + }); + }); + + // ngOnInit and ngAfterContentInit lifecycle hooks run once before "OnPushComp" is + // refreshed/checked. This means they cannot mark the component as dirty because the + // component dirty state will immediately reset after these hooks complete. + ['ngOnInit', 'ngAfterContentInit'].forEach(hookName => { + it(`should not be able to mark component as dirty from within ${hookName}`, () => { + @Component({ + selector: 'on-push-comp', + changeDetection: ChangeDetectionStrategy.OnPush, + template: `

{{text}}

`, + }) + class OnPushComp { + text = 'initial'; + + constructor(private _cdRef: ChangeDetectorRef){} + + [hookName]() { + this._cdRef.markForCheck(); + } + } + + @Component({template: ``}) + class TestApp { + @ViewChild(OnPushComp) onPushComp !: OnPushComp; + } + + TestBed.configureTestingModule( + {declarations: [TestApp, OnPushComp], imports: [CommonModule]}); + const fixture = TestBed.createComponent(TestApp); + const pElement = fixture.nativeElement.querySelector('p') as HTMLElement; + + detectChanges(fixture); + expect(pElement.textContent).toBe('initial'); + + fixture.componentInstance.onPushComp.text = 'new'; + // this is a noop since the "OnPushComp" component is not marked as dirty. The + // programmatically updated value will not be reflected in the rendered view. + detectChanges(fixture); + expect(pElement.textContent).toBe('initial'); + }); + }); + } + }); + describe('ExpressionChangedAfterItHasBeenCheckedError', () => { @Component({template: '...'}) class MyApp { diff --git a/packages/core/test/acceptance/integration_spec.ts b/packages/core/test/acceptance/integration_spec.ts index 1a8762a838..ca44ccf073 100644 --- a/packages/core/test/acceptance/integration_spec.ts +++ b/packages/core/test/acceptance/integration_spec.ts @@ -9,7 +9,6 @@ import {CommonModule} from '@angular/common'; import {Component, ContentChild, Directive, ElementRef, EventEmitter, HostBinding, HostListener, Input, NgModule, OnInit, Output, Pipe, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; import {TVIEW} from '@angular/core/src/render3/interfaces/view'; import {getLView} from '@angular/core/src/render3/state'; -import {loadLContext} from '@angular/core/src/render3/util/discovery_utils'; import {ngDevModeResetPerfCounters} from '@angular/core/src/util/ng_dev_mode'; import {TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser';