diff --git a/packages/core/src/zone/ng_zone.ts b/packages/core/src/zone/ng_zone.ts index bb230c27f7..74d1748116 100644 --- a/packages/core/src/zone/ng_zone.ts +++ b/packages/core/src/zone/ng_zone.ts @@ -243,6 +243,17 @@ interface NgZonePrivate extends NgZone { hasPendingMacrotasks: boolean; hasPendingMicrotasks: boolean; lastRequestAnimationFrameId: number; + /** + * A flag to indicate if NgZone is currently inside + * checkStable and to prevent re-entry. The flag is + * needed because it is possible to invoke the change + * detection from within change detection leading to + * incorrect behavior. + * + * For detail, please refer here, + * https://github.com/angular/angular/pull/40540 + */ + isCheckStableRunning: boolean; isStable: boolean; /** * Optionally specify coalescing event change detections or not. @@ -291,6 +302,21 @@ interface NgZonePrivate extends NgZone { } function checkStable(zone: NgZonePrivate) { + // TODO: @JiaLiPassion, should check zone.isCheckStableRunning to prevent + // re-entry. The case is: + // + // @Component({...}) + // export class AppComponent { + // constructor(private ngZone: NgZone) { + // this.ngZone.onStable.subscribe(() => { + // this.ngZone.run(() => console.log('stable');); + // }); + // } + // + // The onStable subscriber run another function inside ngZone + // which causes `checkStable()` re-entry. + // But this fix causes some issues in g3, so this fix will be + // launched in another PR. if (zone._nesting == 0 && !zone.hasPendingMicrotasks && !zone.isStable) { try { zone._nesting++; @@ -309,7 +335,20 @@ function checkStable(zone: NgZonePrivate) { } function delayChangeDetectionForEvents(zone: NgZonePrivate) { - if (zone.lastRequestAnimationFrameId !== -1) { + /** + * We also need to check _nesting here + * Consider the following case with shouldCoalesceRunChangeDetection = true + * + * ngZone.run(() => {}); + * ngZone.run(() => {}); + * + * We want the two `ngZone.run()` only trigger one change detection + * when shouldCoalesceRunChangeDetection is true. + * And because in this case, change detection run in async way(requestAnimationFrame), + * so we also need to check the _nesting here to prevent multiple + * change detections. + */ + if (zone.isCheckStableRunning || zone.lastRequestAnimationFrameId !== -1) { return; } zone.lastRequestAnimationFrameId = zone.nativeRequestAnimationFrame.call(global, () => { @@ -326,7 +365,9 @@ function delayChangeDetectionForEvents(zone: NgZonePrivate) { zone.fakeTopEventTask = Zone.root.scheduleEventTask('fakeTopEventTask', () => { zone.lastRequestAnimationFrameId = -1; updateMicroTaskStatus(zone); + zone.isCheckStableRunning = true; checkStable(zone); + zone.isCheckStableRunning = false; }, undefined, () => {}, () => {}); } zone.fakeTopEventTask.invoke(); diff --git a/packages/core/test/zone/ng_zone_spec.ts b/packages/core/test/zone/ng_zone_spec.ts index 3f11518a0e..dd9b6cc5ad 100644 --- a/packages/core/test/zone/ng_zone_spec.ts +++ b/packages/core/test/zone/ng_zone_spec.ts @@ -982,6 +982,7 @@ function commonTests() { describe('shouldCoalesceEventChangeDetection = true, shouldCoalesceRunChangeDetection = false', () => { let nativeRequestAnimationFrame: (fn: FrameRequestCallback) => void; + let nativeSetTimeout: any = (global as any)[Zone.__symbol__('setTimeout')]; if (!(global as any).requestAnimationFrame) { nativeRequestAnimationFrame = function(fn: Function) { (global as any)[Zone.__symbol__('setTimeout')](fn, 16); @@ -1042,6 +1043,32 @@ function commonTests() { }); }); + it('should only emit onMicroTaskEmpty once within the same event loop for ngZone.run in onMicrotaskEmpty subscription', + (done: DoneFn) => { + const tasks: Task[] = []; + coalesceZone.onMicrotaskEmpty.subscribe(() => { + coalesceZone.run(() => {}); + }); + coalesceZone.run(() => { + tasks.push(Zone.current.scheduleEventTask('myEvent', () => { + logs.push('eventTask1'); + }, undefined, () => {})); + }); + coalesceZone.run(() => { + tasks.push(Zone.current.scheduleEventTask('myEvent', () => { + logs.push('eventTask2'); + }, undefined, () => {})); + }); + tasks.forEach(t => t.invoke()); + expect(logs).toEqual(['microTask empty', 'microTask empty', 'eventTask1', 'eventTask2']); + nativeSetTimeout(() => { + expect(logs).toEqual([ + 'microTask empty', 'microTask empty', 'eventTask1', 'eventTask2', 'microTask empty' + ]); + done(); + }, 100); + }); + it('should emit onMicroTaskEmpty once within the same event loop for not only event tasks, but event tasks are before other tasks', (done: DoneFn) => { const tasks: Task[] = []; @@ -1094,6 +1121,7 @@ function commonTests() { describe('shouldCoalesceRunChangeDetection = true', () => { let nativeRequestAnimationFrame: (fn: FrameRequestCallback) => void; + let nativeSetTimeout: any = (global as any)[Zone.__symbol__('setTimeout')]; if (!(global as any).requestAnimationFrame) { nativeRequestAnimationFrame = function(fn: Function) { (global as any)[Zone.__symbol__('setTimeout')](fn, 16); @@ -1139,6 +1167,20 @@ function commonTests() { }); }); + it('should only emit onMicroTaskEmpty once within the same event loop for ngZone.run in onMicrotaskEmpty subscription', + (done: DoneFn) => { + coalesceZone.onMicrotaskEmpty.subscribe(() => { + coalesceZone.run(() => {}); + }); + coalesceZone.run(() => {}); + coalesceZone.run(() => {}); + expect(logs).toEqual([]); + nativeSetTimeout(() => { + expect(logs).toEqual(['microTask empty']); + done(); + }, 100); + }); + it('should only emit onMicroTaskEmpty once within the same event loop for multiple tasks', (done: DoneFn) => { const tasks: Task[] = []; diff --git a/packages/platform-browser/test/dom/events/event_manager_spec.ts b/packages/platform-browser/test/dom/events/event_manager_spec.ts index 245ca5bf9f..0369cd615c 100644 --- a/packages/platform-browser/test/dom/events/event_manager_spec.ts +++ b/packages/platform-browser/test/dom/events/event_manager_spec.ts @@ -436,13 +436,13 @@ describe('EventManager', () => { removerChildFocus = manager.addEventListener(child, 'blur', blurHandler); }); const sub = zone.onStable.subscribe(() => { + sub.unsubscribe(); logs.push('begin'); Promise.resolve().then(() => { logs.push('promise resolved'); }); element.appendChild(child); getDOM().dispatchEvent(child, dispatchedBlurEvent); - sub.unsubscribe(); logs.push('end'); }); getDOM().dispatchEvent(element, dispatchedClickEvent); @@ -482,13 +482,13 @@ describe('EventManager', () => { removerChildFocus = manager.addEventListener(child, 'blur', blurHandler); }); const sub = zone.onStable.subscribe(() => { + sub.unsubscribe(); logs.push('begin'); Promise.resolve().then(() => { logs.push('promise resolved'); }); element.appendChild(child); getDOM().dispatchEvent(child, dispatchedBlurEvent); - sub.unsubscribe(); logs.push('end'); }); getDOM().dispatchEvent(element, dispatchedClickEvent);