From 72dba59b594b04bb2841d57239579875b7163176 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 17 Oct 2019 08:48:37 -0700 Subject: [PATCH] revert: feat: add a flag in bootstrap to enable coalesce event change detection to improve performance (#30533) (#33230) This reverts commit 21c1e14385a61b4fdd3c5973abcaa3eace1303c1. PR Close #33230 --- integration/_payload-limits.json | 2 +- .../side-effects/snapshots/core/esm5.js | 17 ------ packages/core/src/application_ref.ts | 33 ++--------- packages/core/src/util/raf.ts | 29 ---------- packages/core/src/zone/ng_zone.ts | 55 ++----------------- packages/core/test/fake_async_spec.ts | 2 +- packages/core/testing/src/ng_zone_mock.ts | 2 +- packages/core/testing/src/test_bed.ts | 3 +- .../test/dom/events/event_manager_spec.ts | 41 ++------------ .../testing/src/browser_util.ts | 2 +- tools/public_api_guard/core/core.d.ts | 6 +- 11 files changed, 21 insertions(+), 171 deletions(-) delete mode 100644 packages/core/src/util/raf.ts diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index f78f88d5ca3f4..614fd48e36da5 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -21,7 +21,7 @@ "master": { "uncompressed": { "runtime": 1440, - "main": 125178, + "main": 123904, "polyfills": 45340 } } diff --git a/integration/side-effects/snapshots/core/esm5.js b/integration/side-effects/snapshots/core/esm5.js index 16cfc7127979c..e969f1091b4af 100644 --- a/integration/side-effects/snapshots/core/esm5.js +++ b/integration/side-effects/snapshots/core/esm5.js @@ -14,23 +14,6 @@ var __global = "undefined" !== typeof global && global; var _global = __globalThis || __global || __window || __self; -function getNativeRequestAnimationFrame() { - var nativeRequestAnimationFrame = _global["requestAnimationFrame"]; - var nativeCancelAnimationFrame = _global["cancelAnimationFrame"]; - if ("undefined" !== typeof Zone && nativeRequestAnimationFrame && nativeCancelAnimationFrame) { - var unpatchedRequestAnimationFrame = nativeRequestAnimationFrame[Zone.__symbol__("OriginalDelegate")]; - if (unpatchedRequestAnimationFrame) nativeRequestAnimationFrame = unpatchedRequestAnimationFrame; - var unpatchedCancelAnimationFrame = nativeCancelAnimationFrame[Zone.__symbol__("OriginalDelegate")]; - if (unpatchedCancelAnimationFrame) nativeCancelAnimationFrame = unpatchedCancelAnimationFrame; - } - return { - nativeRequestAnimationFrame: nativeRequestAnimationFrame, - nativeCancelAnimationFrame: nativeCancelAnimationFrame - }; -} - -var nativeRequestAnimationFrame = getNativeRequestAnimationFrame().nativeRequestAnimationFrame; - if (ngDevMode) _global.$localize = _global.$localize || function() { throw new Error("It looks like your application or one of its dependencies is using i18n.\n" + "Angular 9 introduced a global `$localize()` function that needs to be loaded.\n" + "Please add `import '@angular/localize/init';` to your polyfills.ts file."); }; diff --git a/packages/core/src/application_ref.ts b/packages/core/src/application_ref.ts index 520cd2e2e914e..e70537188faff 100644 --- a/packages/core/src/application_ref.ts +++ b/packages/core/src/application_ref.ts @@ -219,27 +219,6 @@ export interface BootstrapOptions { * - `noop` - Use `NoopNgZone` which does nothing. */ ngZone?: NgZone|'zone.js'|'noop'; - - /** - * Optionally specify coalescing event change detections or not. - * Consider the following case. - * - *
- * - *
- * - * When button is clicked, because of the event bubbling, both - * event handlers will be called and 2 change detections will be - * triggered. We can colesce such kind of events to only trigger - * change detection only once. - * - * By default, this option will be false. So the events will not be - * colesced and the change detection will be triggered multiple times. - * And if this option be set to true, the change detection will be - * triggered async by scheduling a animation frame. So in the case above, - * the change detection will only be trigged once. - */ - ngZoneEventCoalescing?: boolean; } /** @@ -290,8 +269,7 @@ export class PlatformRef { // So we create a mini parent injector that just contains the new NgZone and // pass that as parent to the NgModuleFactory. const ngZoneOption = options ? options.ngZone : undefined; - const ngZoneEventCoalescing = (options && options.ngZoneEventCoalescing) || false; - const ngZone = getNgZone(ngZoneOption, ngZoneEventCoalescing); + const ngZone = getNgZone(ngZoneOption); const providers: StaticProvider[] = [{provide: NgZone, useValue: ngZone}]; // Attention: Don't use ApplicationRef.run here, // as we want to be sure that all possible constructor calls are inside `ngZone.run`! @@ -387,17 +365,14 @@ export class PlatformRef { get destroyed() { return this._destroyed; } } -function getNgZone( - ngZoneOption: NgZone | 'zone.js' | 'noop' | undefined, ngZoneEventCoalescing: boolean): NgZone { +function getNgZone(ngZoneOption?: NgZone | 'zone.js' | 'noop'): NgZone { let ngZone: NgZone; if (ngZoneOption === 'noop') { ngZone = new NoopNgZone(); } else { - ngZone = (ngZoneOption === 'zone.js' ? undefined : ngZoneOption) || new NgZone({ - enableLongStackTrace: isDevMode(), - shouldCoalesceEventChangeDetection: ngZoneEventCoalescing - }); + ngZone = (ngZoneOption === 'zone.js' ? undefined : ngZoneOption) || + new NgZone({enableLongStackTrace: isDevMode()}); } return ngZone; } diff --git a/packages/core/src/util/raf.ts b/packages/core/src/util/raf.ts deleted file mode 100644 index db4e242f5b8ba..0000000000000 --- a/packages/core/src/util/raf.ts +++ /dev/null @@ -1,29 +0,0 @@ -/** - * @license - * Copyright Google Inc. All Rights Reserved. - * - * Use of this source code is governed by an MIT-style license that can be - * found in the LICENSE file at https://angular.io/license - */ -import {global} from './global'; - -export function getNativeRequestAnimationFrame() { - let nativeRequestAnimationFrame: (callback: FrameRequestCallback) => number = - global['requestAnimationFrame']; - let nativeCancelAnimationFrame: (handle: number) => void = global['cancelAnimationFrame']; - if (typeof Zone !== 'undefined' && nativeRequestAnimationFrame && nativeCancelAnimationFrame) { - // use unpatched version of requestAnimationFrame(native delegate) if possible - // to avoid another Change detection - const unpatchedRequestAnimationFrame = - (nativeRequestAnimationFrame as any)[(Zone as any).__symbol__('OriginalDelegate')]; - if (unpatchedRequestAnimationFrame) { - nativeRequestAnimationFrame = unpatchedRequestAnimationFrame; - } - const unpatchedCancelAnimationFrame = - (nativeCancelAnimationFrame as any)[(Zone as any).__symbol__('OriginalDelegate')]; - if (unpatchedCancelAnimationFrame) { - nativeCancelAnimationFrame = unpatchedCancelAnimationFrame; - } - } - return {nativeRequestAnimationFrame, nativeCancelAnimationFrame}; -} diff --git a/packages/core/src/zone/ng_zone.ts b/packages/core/src/zone/ng_zone.ts index 14321fe03200a..85130297c18b3 100644 --- a/packages/core/src/zone/ng_zone.ts +++ b/packages/core/src/zone/ng_zone.ts @@ -7,9 +7,6 @@ */ import {EventEmitter} from '../event_emitter'; -import {global} from '../util/global'; -import {getNativeRequestAnimationFrame} from '../util/raf'; - /** * An injectable service for executing work inside or outside of the Angular zone. @@ -86,11 +83,8 @@ import {getNativeRequestAnimationFrame} from '../util/raf'; * @publicApi */ export class NgZone { - readonly hasPendingZoneMicrotasks: boolean = false; - readonly lastRequestAnimationFrameId: number = -1; - readonly shouldCoalesceEventChangeDetection: boolean = true; - readonly hasPendingMacrotasks: boolean = false; readonly hasPendingMicrotasks: boolean = false; + readonly hasPendingMacrotasks: boolean = false; /** * Whether there are no outstanding microtasks or macrotasks. @@ -121,8 +115,7 @@ export class NgZone { */ readonly onError: EventEmitter = new EventEmitter(false); - - constructor({enableLongStackTrace = false, shouldCoalesceEventChangeDetection = false}) { + constructor({enableLongStackTrace = false}) { if (typeof Zone == 'undefined') { throw new Error(`In this configuration Angular requires Zone.js`); } @@ -145,7 +138,6 @@ export class NgZone { self._inner = self._inner.fork((Zone as any)['longStackTraceZoneSpec']); } - self.shouldCoalesceEventChangeDetection = shouldCoalesceEventChangeDetection; forkInnerZoneWithAngularBehavior(self); } @@ -229,19 +221,16 @@ export class NgZone { function noop() {} const EMPTY_PAYLOAD = {}; -const {nativeRequestAnimationFrame} = getNativeRequestAnimationFrame(); + interface NgZonePrivate extends NgZone { _outer: Zone; _inner: Zone; _nesting: number; - _hasPendingMicrotasks: boolean; - hasPendingMacrotasks: boolean; hasPendingMicrotasks: boolean; - lastRequestAnimationFrameId: number; + hasPendingMacrotasks: boolean; isStable: boolean; - shouldCoalesceEventChangeDetection: boolean; } function checkStable(zone: NgZonePrivate) { @@ -262,35 +251,16 @@ function checkStable(zone: NgZonePrivate) { } } -function delayChangeDetectionForEvents(zone: NgZonePrivate) { - if (zone.lastRequestAnimationFrameId !== -1) { - return; - } - zone.lastRequestAnimationFrameId = nativeRequestAnimationFrame.call(global, () => { - zone.lastRequestAnimationFrameId = -1; - updateMicroTaskStatus(zone); - checkStable(zone); - }); - updateMicroTaskStatus(zone); -} - function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate) { - const delayChangeDetectionForEventsDelegate = () => { delayChangeDetectionForEvents(zone); }; - const maybeDelayChangeDetection = !!zone.shouldCoalesceEventChangeDetection && - nativeRequestAnimationFrame && delayChangeDetectionForEventsDelegate; zone._inner = zone._inner.fork({ name: 'angular', - properties: - {'isAngularZone': true, 'maybeDelayChangeDetection': maybeDelayChangeDetection}, + properties: {'isAngularZone': true}, onInvokeTask: (delegate: ZoneDelegate, current: Zone, target: Zone, task: Task, applyThis: any, applyArgs: any): any => { try { onEnter(zone); return delegate.invokeTask(target, task, applyThis, applyArgs); } finally { - if (maybeDelayChangeDetection && task.type === 'eventTask') { - maybeDelayChangeDetection(); - } onLeave(zone); } }, @@ -313,8 +283,7 @@ function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate) { // We are only interested in hasTask events which originate from our zone // (A child hasTask event is not interesting to us) if (hasTaskState.change == 'microTask') { - zone._hasPendingMicrotasks = hasTaskState.microTask; - updateMicroTaskStatus(zone); + zone.hasPendingMicrotasks = hasTaskState.microTask; checkStable(zone); } else if (hasTaskState.change == 'macroTask') { zone.hasPendingMacrotasks = hasTaskState.macroTask; @@ -330,15 +299,6 @@ function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate) { }); } -function updateMicroTaskStatus(zone: NgZonePrivate) { - if (zone._hasPendingMicrotasks || - (zone.shouldCoalesceEventChangeDetection && zone.lastRequestAnimationFrameId !== -1)) { - zone.hasPendingMicrotasks = true; - } else { - zone.hasPendingMicrotasks = false; - } -} - function onEnter(zone: NgZonePrivate) { zone._nesting++; if (zone.isStable) { @@ -357,8 +317,6 @@ function onLeave(zone: NgZonePrivate) { * to framework to perform rendering. */ export class NoopNgZone implements NgZone { - readonly hasPendingZoneMicrotasks: boolean = false; - readonly lastRequestAnimationFrameId = -1; readonly hasPendingMicrotasks: boolean = false; readonly hasPendingMacrotasks: boolean = false; readonly isStable: boolean = true; @@ -366,7 +324,6 @@ export class NoopNgZone implements NgZone { readonly onMicrotaskEmpty: EventEmitter = new EventEmitter(); readonly onStable: EventEmitter = new EventEmitter(); readonly onError: EventEmitter = new EventEmitter(); - readonly shouldCoalesceEventChangeDetection: boolean = false; run(fn: (...args: any[]) => any, applyThis?: any, applyArgs?: any): any { return fn.apply(applyThis, applyArgs); diff --git a/packages/core/test/fake_async_spec.ts b/packages/core/test/fake_async_spec.ts index 036cabef965c2..ffbaeb82a691b 100644 --- a/packages/core/test/fake_async_spec.ts +++ b/packages/core/test/fake_async_spec.ts @@ -95,7 +95,7 @@ const ProxyZoneSpec: {assertPresent: () => void} = (Zone as any)['ProxyZoneSpec' resolvedPromise.then((_) => { throw new Error('async'); }); flushMicrotasks(); })(); - }).toThrow(); + }).toThrowError(/Uncaught \(in promise\): Error: async/); }); it('should complain if a test throws an exception', () => { diff --git a/packages/core/testing/src/ng_zone_mock.ts b/packages/core/testing/src/ng_zone_mock.ts index cad1508c9bf5a..a3356d8e6b219 100644 --- a/packages/core/testing/src/ng_zone_mock.ts +++ b/packages/core/testing/src/ng_zone_mock.ts @@ -16,7 +16,7 @@ import {EventEmitter, Injectable, NgZone} from '@angular/core'; export class MockNgZone extends NgZone { onStable: EventEmitter = new EventEmitter(false); - constructor() { super({enableLongStackTrace: false, shouldCoalesceEventChangeDetection: false}); } + constructor() { super({enableLongStackTrace: false}); } run(fn: Function): any { return fn(); } diff --git a/packages/core/testing/src/test_bed.ts b/packages/core/testing/src/test_bed.ts index 469d981dfe1df..bc1f3bc98d76a 100644 --- a/packages/core/testing/src/test_bed.ts +++ b/packages/core/testing/src/test_bed.ts @@ -402,8 +402,7 @@ export class TestBedViewEngine implements TestBed { overrideComponentView(component, compFactory); } - const ngZone = - new NgZone({enableLongStackTrace: true, shouldCoalesceEventChangeDetection: false}); + const ngZone = new NgZone({enableLongStackTrace: true}); const providers: StaticProvider[] = [{provide: NgZone, useValue: ngZone}]; const ngZoneInjector = Injector.create({ providers: providers, 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 b3713ed6af389..dd8b6af096fa2 100644 --- a/packages/platform-browser/test/dom/events/event_manager_spec.ts +++ b/packages/platform-browser/test/dom/events/event_manager_spec.ts @@ -20,6 +20,7 @@ import {createMouseEvent, el} from '../../../testing/src/browser_util'; let zone: NgZone; describe('EventManager', () => { + beforeEach(() => { doc = getDOM().supportsDOMEvents() ? document : getDOM().createHtmlDocument(); zone = new NgZone({}); @@ -295,7 +296,7 @@ import {createMouseEvent, el} from '../../../testing/src/browser_util'; expect(receivedEvents).toEqual([]); }); - it('should run blackListedEvents handler outside of ngZone', () => { + it('should run blockListedEvents handler outside of ngZone', () => { const Zone = (window as any)['Zone']; const element = el('
'); doc.body.appendChild(element); @@ -311,45 +312,13 @@ import {createMouseEvent, el} from '../../../testing/src/browser_util'; let remover = manager.addEventListener(element, 'scroll', handler); getDOM().dispatchEvent(element, dispatchedEvent); expect(receivedEvent).toBe(dispatchedEvent); - expect(receivedZone.name).not.toEqual('angular'); + expect(receivedZone.name).toBe(Zone.root.name); receivedEvent = null; remover && remover(); getDOM().dispatchEvent(element, dispatchedEvent); expect(receivedEvent).toBe(null); }); - - it('should only trigger one Change detection when bubbling', (done: DoneFn) => { - doc = getDOM().supportsDOMEvents() ? document : getDOM().createHtmlDocument(); - zone = new NgZone({shouldCoalesceEventChangeDetection: true}); - domEventPlugin = new DomEventsPlugin(doc, zone, null); - const element = el('
'); - const child = el('
'); - element.appendChild(child); - doc.body.appendChild(element); - const dispatchedEvent = createMouseEvent('click'); - let receivedEvents: any = []; - let stables: any = []; - const handler = (e: any) => { receivedEvents.push(e); }; - const manager = new EventManager([domEventPlugin], zone); - let removerChild: any; - let removerParent: any; - - zone.run(() => { - removerChild = manager.addEventListener(child, 'click', handler); - removerParent = manager.addEventListener(element, 'click', handler); - }); - zone.onStable.subscribe((isStable: any) => { stables.push(isStable); }); - getDOM().dispatchEvent(child, dispatchedEvent); - requestAnimationFrame(() => { - expect(receivedEvents.length).toBe(2); - expect(stables.length).toBe(1); - - removerChild && removerChild(); - removerParent && removerParent(); - done(); - }); - }); }); })(); @@ -363,12 +332,12 @@ class FakeEventManagerPlugin extends EventManagerPlugin { addEventListener(element: any, eventName: string, handler: Function) { this.eventHandler[eventName] = handler; - return () => { delete this.eventHandler[eventName]; }; + return () => { delete (this.eventHandler[eventName]); }; } } class FakeNgZone extends NgZone { - constructor() { super({enableLongStackTrace: false, shouldCoalesceEventChangeDetection: true}); } + constructor() { super({enableLongStackTrace: false}); } run(fn: (...args: any[]) => T, applyThis?: any, applyArgs?: any[]): T { return fn(); } runOutsideAngular(fn: Function) { return fn(); } } diff --git a/packages/platform-browser/testing/src/browser_util.ts b/packages/platform-browser/testing/src/browser_util.ts index 2a71ef380bc1f..41fed84a72eba 100644 --- a/packages/platform-browser/testing/src/browser_util.ts +++ b/packages/platform-browser/testing/src/browser_util.ts @@ -175,7 +175,7 @@ export function stringifyElement(el: any /** TODO #9100 */): string { } export function createNgZone(): NgZone { - return new NgZone({enableLongStackTrace: true, shouldCoalesceEventChangeDetection: false}); + return new NgZone({enableLongStackTrace: true}); } export function isCommentNode(node: Node): boolean { diff --git a/tools/public_api_guard/core/core.d.ts b/tools/public_api_guard/core/core.d.ts index 2451c41339acd..0350326531c0e 100644 --- a/tools/public_api_guard/core/core.d.ts +++ b/tools/public_api_guard/core/core.d.ts @@ -624,17 +624,13 @@ export declare class NgProbeToken { export declare class NgZone { readonly hasPendingMacrotasks: boolean; readonly hasPendingMicrotasks: boolean; - readonly hasPendingZoneMicrotasks: boolean; readonly isStable: boolean; - readonly lastRequestAnimationFrameId: number; readonly onError: EventEmitter; readonly onMicrotaskEmpty: EventEmitter; readonly onStable: EventEmitter; readonly onUnstable: EventEmitter; - readonly shouldCoalesceEventChangeDetection: boolean; - constructor({ enableLongStackTrace, shouldCoalesceEventChangeDetection }: { + constructor({ enableLongStackTrace }: { enableLongStackTrace?: boolean | undefined; - shouldCoalesceEventChangeDetection?: boolean | undefined; }); run(fn: (...args: any[]) => T, applyThis?: any, applyArgs?: any[]): T; runGuarded(fn: (...args: any[]) => T, applyThis?: any, applyArgs?: any[]): T;