From 4a8d5ae9703f08cc0a07070465bd81229853c33f Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 17 Oct 2020 22:56:09 +0900 Subject: [PATCH] fix(core): markDirty() should only mark flags when really scheduling tick. (#39316) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Close #39296 Fix an issue that `markDirty()` will not trigger change detection. The case is for example we have the following component. ``` export class AppComponent implements OnInit { constructor(private router: Router) {} ngOnInit() { this.router.events .pipe(filter((e) => e instanceof NavigationEnd)) .subscribe(() => ɵmarkDirty(this)); } } export class CounterComponent implements OnInit, OnDestroy { ngOnInit() { this.countSubject.pipe(takeUntil(this.destroy)).subscribe((count) => { this.count = count; ɵmarkDirty(this); }); } ``` Then the app navigate from `AppComponent` to `CounterComponent`, so there are 2 `markDirty()` call at in a row. The `1st` call is from `AppComponent` when router changed, the `2nd` call is from `CounterComponent.ngOnInit()`. And the `markDirty()->scheduleTick()` code look like this ``` function scheduleTick(rootContext, flags) { const nothingScheduled = rootContext.flags === 0 /* Empty */; rootContext.flags |= flags; if (nothingScheduled && rootContext.clean == _CLEAN_PROMISE) { rootContext.schedule(() => { ... if (rootContext.flags & RootContextFlags.DetectChanges) rootContext.flags &= ~RootContextFlags.DetectChanges; tickContext(); rootContext.clean = _CLEAN_PROMISE; ... }); ``` So in this case, the `1st` markDirty() will 1. set rootContext.flags = 1 2. before `tickContext()`, reset rootContext.flags = 0 3. inside `tickContext()`, it will call `CounterComponent.ngOnint()`, so the `2nd` markDirty() is called. 4. and the `2nd` scheduleTick is called, `nothingScheduled` is true, but rootContext.clean is not `_CLEAN_PROMISE` yet, since the `1st` markDirty tick is still running. 5. So nowhere will reset the `rootContext.flags`. 6. then in the future, any other `markDirty()` call will not trigger the tick, since `nothingScheduled` is always false. So `nothingScheduled` means no tick is scheduled, `rootContext.clean === _CLEAN_PROMISE` means no tick is running. So we should set the flags to `rootContext` only when `no tick is scheudled or running`. PR Close #39316 --- .../core/src/render3/instructions/shared.ts | 5 +- .../test/render3/change_detection_spec.ts | 104 +++++++++++++++++- 2 files changed, 104 insertions(+), 5 deletions(-) diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 294dc3e977..16836af524 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -1851,9 +1851,10 @@ export function markViewDirty(lView: LView): LView|null { */ export function scheduleTick(rootContext: RootContext, flags: RootContextFlags) { const nothingScheduled = rootContext.flags === RootContextFlags.Empty; - rootContext.flags |= flags; - if (nothingScheduled && rootContext.clean == _CLEAN_PROMISE) { + // https://github.com/angular/angular/issues/39296 + // should only attach the flags when really scheduling a tick + rootContext.flags |= flags; let res: null|((val: null) => void); rootContext.clean = new Promise((r) => res = r); rootContext.scheduler(() => { diff --git a/packages/core/test/render3/change_detection_spec.ts b/packages/core/test/render3/change_detection_spec.ts index 6f878d8d20..65a67aa43f 100644 --- a/packages/core/test/render3/change_detection_spec.ts +++ b/packages/core/test/render3/change_detection_spec.ts @@ -8,18 +8,20 @@ import {withBody} from '@angular/private/testing'; -import {ChangeDetectionStrategy, DoCheck} from '../../src/core'; +import {ChangeDetectionStrategy, DoCheck, OnInit} from '../../src/core'; import {whenRendered} from '../../src/render3/component'; -import {getRenderedText, LifecycleHooksFeature, ɵɵadvance, ɵɵdefineComponent, ɵɵgetCurrentView, ɵɵproperty, ɵɵtextInterpolate1, ɵɵtextInterpolate2} from '../../src/render3/index'; -import {detectChanges, markDirty, tick, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵlistener, ɵɵtext, ɵɵtextInterpolate} from '../../src/render3/instructions/all'; +import {AttributeMarker, getRenderedText, LifecycleHooksFeature, ɵɵadvance, ɵɵdefineComponent, ɵɵgetCurrentView, ɵɵproperty, ɵɵtextInterpolate1, ɵɵtextInterpolate2} from '../../src/render3/index'; +import {detectChanges, markDirty, tick, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵlistener, ɵɵtemplate, ɵɵtext, ɵɵtextInterpolate} from '../../src/render3/instructions/all'; import {RenderFlags} from '../../src/render3/interfaces/definition'; import {Renderer3, RendererFactory3} from '../../src/render3/interfaces/renderer'; import {FLAGS, LViewFlags} from '../../src/render3/interfaces/view'; +import {NgIf} from './common_with_def'; import {containerEl, createComponent, renderComponent, requestAnimationFrame} from './render_util'; describe('change detection', () => { describe('markDirty, detectChanges, whenRendered, getRenderedText', () => { + let mycompOninit: MyComponentWithOnInit; class MyComponent implements DoCheck { value: string = 'works'; doCheckCount = 0; @@ -48,6 +50,84 @@ describe('change detection', () => { }); } + class MyComponentWithOnInit implements OnInit, DoCheck { + value: string = 'works'; + doCheckCount = 0; + + ngOnInit() { + markDirty(this); + } + + ngDoCheck(): void { + this.doCheckCount++; + } + + click() { + this.value = 'click works'; + markDirty(this); + } + + static ɵfac = () => mycompOninit = new MyComponentWithOnInit(); + static ɵcmp = ɵɵdefineComponent({ + type: MyComponentWithOnInit, + selectors: [['my-comp-oninit']], + decls: 2, + vars: 1, + template: + (rf: RenderFlags, ctx: MyComponentWithOnInit) => { + if (rf & RenderFlags.Create) { + ɵɵelementStart(0, 'span'); + ɵɵtext(1); + ɵɵelementEnd(); + } + if (rf & RenderFlags.Update) { + ɵɵadvance(1); + ɵɵtextInterpolate(ctx.value); + } + } + }); + } + + class MyParentComponent implements OnInit { + show = false; + value = 'parent'; + mycomp: any = undefined; + + ngOnInit() {} + + click() { + this.show = true; + markDirty(this); + } + + static ɵfac = () => new MyParentComponent(); + static ɵcmp = ɵɵdefineComponent({ + type: MyParentComponent, + selectors: [['my-parent-comp']], + decls: 2, + vars: 1, + directives: [NgIf, MyComponentWithOnInit], + consts: [[AttributeMarker.Template, 'ngIf']], + template: + (rf: RenderFlags, ctx: MyParentComponent) => { + if (rf & RenderFlags.Create) { + ɵɵtext(0, ' -->\n'); + ɵɵtemplate(1, (rf, ctx) => { + if (rf & RenderFlags.Create) { + ɵɵelementStart(0, 'div'); + ɵɵelement(1, 'my-comp-oninit'); + ɵɵelementEnd(); + } + }, 2, 0, 'div', 0); + } + if (rf & RenderFlags.Update) { + ɵɵadvance(1); + ɵɵproperty('ngIf', ctx.show); + } + } + }); + } + it('should mark a component dirty and schedule change detection', withBody('my-comp', () => { const myComp = renderComponent(MyComponent, {hostFeatures: [LifecycleHooksFeature]}); expect(getRenderedText(myComp)).toEqual('works'); @@ -66,6 +146,24 @@ describe('change detection', () => { expect(getRenderedText(myComp)).toEqual('updated'); })); + it('should detectChanges after markDirty is called multiple times within ngOnInit', + withBody('my-comp-oninit', () => { + const myParentComp = + renderComponent(MyParentComponent, {hostFeatures: [LifecycleHooksFeature]}); + expect(myParentComp.show).toBe(false); + myParentComp.click(); + requestAnimationFrame.flush(); + expect(myParentComp.show).toBe(true); + const myComp = mycompOninit; + expect(getRenderedText(myComp)).toEqual('works'); + expect(myComp.doCheckCount).toBe(1); + myComp.click(); + expect(getRenderedText(myComp)).toEqual('works'); + requestAnimationFrame.flush(); + expect(getRenderedText(myComp)).toEqual('click works'); + expect(myComp.doCheckCount).toBe(2); + })); + it('should detectChanges only once if markDirty is called multiple times', withBody('my-comp', () => { const myComp = renderComponent(MyComponent, {hostFeatures: [LifecycleHooksFeature]});