diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index d5d71499305..fb57f881814 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 10da2521b44..8f7216e8032 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 1a8762a838b..ca44ccf0733 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';