From f16ecd0a987d36e373b6a1def4fcf0f08675f1c9 Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Mon, 15 Feb 2021 09:16:12 +0800 Subject: [PATCH] feat(core): ChangeDetectorRef.detectChanges should detect bootstrap and onPush not dirty component Close #36667 Calling `componentRef.detectChanges()` sometimes not work as expected. ``` @Component({ selector: 'dynamic', template: `{{name}}`, changeDetection: ChangeDetectionStrategy.OnPush }) export class Dynamic { name = 'initial name'; } const componetRef = cfr.createComponent(Dynamic); componentRef.changeDetectorRef // host view componentRef.injector.get(ChangeDetectorRef) // component view componentRef.instance.cdRef // componentView componentRef.instance.name = 'something new'; // componentRef.instance.cdRef.markForCheck(); componentRef.detectChanges(); // does not work without `markForCheck` componentRef.changeDetectorRef.detectChanges() // Users expect that Bar gets refreshed. componentRef.injector.get(ChangeDetectorRef).detectChanges() componentRef.instance.cdRef.detectChanges() // Example: https://stackblitz.com/edit/angular-drqzhw?file=src%2Fapp%2Fapp.module.ts ngOnInit() { const cmptRef = this._cfr .resolveComponentFactory(Dynamic) .create(this._injector); this._vcRef.insert(cmptRef.hostView); setTimeout(() => { cmptRef.instance.name = "changed name"; console.log("changing name"); cmptRef.changeDetectorRef.detectChanges(); // does not work }); setTimeout(() => { cmptRef.instance.name = "second changed name"; console.log("changing name"); cmptRef.injector.get(ChangeDetectorRef).detectChanges(); // works }, 2000); } ``` Since when the component is `boostrap` or `dynamic`, we created a hostView to host the component. And the `componentRef.changeDetectorRef` references to that hostView instead of the componentView itself. And when the component is `OnPush` and not dirty, calling `changeDetectorRef.detectChanges()` will bypass the component which is confusing. So this commit mark the root component dirty when calling `changeDetectorRef.detectChanges()` from the root hostView. --- packages/core/src/render3/view_ref.ts | 20 ++++ .../test/render3/view_container_ref_spec.ts | 99 ++++++++++++++++++- 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index b88e8b51ba802c..6d4915befd1f20 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -16,6 +16,7 @@ import {CONTAINER_HEADER_OFFSET, VIEW_REFS} from './interfaces/container'; import {isLContainer} from './interfaces/type_checks'; import {CONTEXT, FLAGS, LView, LViewFlags, PARENT, TVIEW} from './interfaces/view'; import {destroyLView, detachView, renderDetachView} from './node_manipulation'; +import {getComponentLViewByIndex} from './util/view_utils'; @@ -308,6 +309,25 @@ export class RootViewRef extends ViewRef { } detectChanges(): void { + // Mark all root children components dirty to ensure these components run change detection. + // The current RootViewRef is the hostView, when the component is + // + // 1. `bootstrap` or `dynamic` + // 2. Using `ChangeDetectionStrategy.OnPush` + // 3. Not dirty + // + // The change detection will bypass the root component. + // So we need to mark the component dirty. + const tView = this._view[TVIEW]; + const components = tView.components; + if (components) { + for (let i = 0; i < components.length; i++) { + const componentView = getComponentLViewByIndex(components[i], this._view); + if (!(componentView[FLAGS] & (LViewFlags.CheckAlways | LViewFlags.Dirty))) { + componentView[FLAGS] |= LViewFlags.Dirty; + } + } + } detectChangesInRootView(this._view); } diff --git a/packages/core/test/render3/view_container_ref_spec.ts b/packages/core/test/render3/view_container_ref_spec.ts index 877bec2de8accc..3626c28157ad86 100644 --- a/packages/core/test/render3/view_container_ref_spec.ts +++ b/packages/core/test/render3/view_container_ref_spec.ts @@ -8,7 +8,7 @@ import {QueryFlags} from '@angular/core/src/render3/interfaces/query'; import {HEADER_OFFSET} from '@angular/core/src/render3/interfaces/view'; -import {ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, ElementRef, QueryList, TemplateRef, ViewContainerRef, ViewRef} from '../../src/core'; +import {ChangeDetectionStrategy, ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, ElementRef, QueryList, TemplateRef, ViewContainerRef, ViewRef} from '../../src/core'; import {ViewEncapsulation} from '../../src/metadata'; import {injectComponentFactoryResolver, ɵɵdefineComponent, ɵɵdefineDirective, ɵɵlistener, ɵɵloadQuery, ɵɵqueryRefresh, ɵɵviewQuery} from '../../src/render3/index'; import {ɵɵdirectiveInject, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵtemplate, ɵɵtext} from '../../src/render3/instructions/all'; @@ -163,6 +163,7 @@ describe('ViewContainerRef', () => { class DynamicComp { doCheckCount = 0; + doRenderCount = 0; ngDoCheck() { this.doCheckCount++; @@ -175,7 +176,10 @@ describe('ViewContainerRef', () => { selectors: [['dynamic-comp']], decls: 0, vars: 0, - template: (rf: RenderFlags, cmp: DynamicComp) => {} + template: + (rf: RenderFlags, cmp: DynamicComp) => { + cmp.doRenderCount++; + } }); } @@ -186,11 +190,13 @@ describe('ViewContainerRef', () => { const ref = fixture.component.vcr.createComponent(dynamicCompFactory); fixture.update(); expect(dynamicComp.doCheckCount).toEqual(1); + expect(dynamicComp.doRenderCount).toEqual(2); // The change detector ref should be attached to the root view that contains // DynamicComp, so the doCheck hook for DynamicComp should run upon ref.detectChanges(). ref.changeDetectorRef.detectChanges(); expect(dynamicComp.doCheckCount).toEqual(2); + expect(dynamicComp.doRenderCount).toEqual(3); expect((ref.changeDetectorRef as any).context).toBeNull(); }); @@ -202,12 +208,14 @@ describe('ViewContainerRef', () => { const ref = fixture.component.vcr.createComponent(dynamicCompFactory); fixture.update(); expect(dynamicComp.doCheckCount).toEqual(1); + expect(dynamicComp.doRenderCount).toEqual(2); // The injector should retrieve the change detector ref for DynamicComp. As such, // the doCheck hook for DynamicComp should NOT run upon ref.detectChanges(). const changeDetector = ref.injector.get(ChangeDetectorRef); changeDetector.detectChanges(); expect(dynamicComp.doCheckCount).toEqual(1); + expect(dynamicComp.doRenderCount).toEqual(3); expect((changeDetector as any).context).toEqual(dynamicComp); }); @@ -227,6 +235,93 @@ describe('ViewContainerRef', () => { }); }); + describe('createComponent OnPush', () => { + describe('ComponentRef', () => { + let dynamicComp!: DynamicComp; + + class AppComp { + constructor(public vcr: ViewContainerRef, public cfr: ComponentFactoryResolver) {} + + static ɵfac = + () => { + return new AppComp( + ɵɵdirectiveInject(ViewContainerRef as any), injectComponentFactoryResolver()); + } + + static ɵcmp = ɵɵdefineComponent({ + type: AppComp, + selectors: [['app-comp']], + decls: 0, + vars: 0, + template: (rf: RenderFlags, cmp: AppComp) => {} + }); + } + + class DynamicComp { + doCheckCount = 0; + doRenderCount = 0; + + ngDoCheck() { + this.doCheckCount++; + } + + static ɵfac = () => dynamicComp = new DynamicComp(); + + static ɵcmp = ɵɵdefineComponent({ + type: DynamicComp, + selectors: [['dynamic-comp']], + decls: 0, + vars: 0, + changeDetection: ChangeDetectionStrategy.OnPush, + template: + (rf: RenderFlags, cmp: DynamicComp) => { + cmp.doRenderCount++; + } + }); + } + + it('should return ComponentRef with ChangeDetectorRef attached to root view', () => { + const fixture = new ComponentFixture(AppComp); + + const dynamicCompFactory = fixture.component.cfr.resolveComponentFactory(DynamicComp); + const ref = fixture.component.vcr.createComponent(dynamicCompFactory); + fixture.update(); + expect(dynamicComp.doCheckCount).toEqual(1); + expect(dynamicComp.doRenderCount).toEqual(2); + + // The change detector ref should be attached to the root view that contains + // DynamicComp, so the doCheck hook for DynamicComp should run upon ref.detectChanges(). + ref.changeDetectorRef.detectChanges(); + expect(dynamicComp.doCheckCount).toEqual(2); + // The DynamicComp is OnPush, and not dirty, the template function should also be + // invoked. + expect(dynamicComp.doRenderCount).toEqual(3); + expect((ref.changeDetectorRef as any).context).toBeNull(); + }); + + it('should return ComponentRef that can retrieve component ChangeDetectorRef through its injector', + () => { + const fixture = new ComponentFixture(AppComp); + + const dynamicCompFactory = fixture.component.cfr.resolveComponentFactory(DynamicComp); + const ref = fixture.component.vcr.createComponent(dynamicCompFactory); + fixture.update(); + expect(dynamicComp.doCheckCount).toEqual(1); + expect(dynamicComp.doRenderCount).toEqual(2); + + // The injector should retrieve the change detector ref for DynamicComp. As such, + // the doCheck hook for DynamicComp should NOT run upon ref.detectChanges(). + const changeDetector = ref.injector.get(ChangeDetectorRef); + changeDetector.detectChanges(); + expect(dynamicComp.doCheckCount).toEqual(1); + // Since the change detector ref is for the DynamicComp, so the template function + // should be invoked. + expect(dynamicComp.doRenderCount).toEqual(3); + expect((changeDetector as any).context).toEqual(dynamicComp); + }); + }); + }); + describe('getters', () => { it('should work on elements', () => { function createTemplate() {