Skip to content

Commit

Permalink
revert: feat: add a flag in bootstrap to enable coalesce event change…
Browse files Browse the repository at this point in the history
… detection to improve performance (angular#30533) (angular#33230)

This reverts commit 21c1e14.

PR Close angular#33230
  • Loading branch information
matsko authored and AndrusGerman committed Oct 22, 2019
1 parent 217dc23 commit 72dba59
Show file tree
Hide file tree
Showing 11 changed files with 21 additions and 171 deletions.
2 changes: 1 addition & 1 deletion integration/_payload-limits.json
Expand Up @@ -21,7 +21,7 @@
"master": {
"uncompressed": {
"runtime": 1440,
"main": 125178,
"main": 123904,
"polyfills": 45340
}
}
Expand Down
17 changes: 0 additions & 17 deletions integration/side-effects/snapshots/core/esm5.js
Expand Up @@ -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.");
};
33 changes: 4 additions & 29 deletions packages/core/src/application_ref.ts
Expand Up @@ -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.
*
* <div (click)="doSomething()">
* <button (click)="doSomethingElse()"></button>
* </div>
*
* 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;
}

/**
Expand Down Expand Up @@ -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`!
Expand Down Expand Up @@ -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;
}
Expand Down
29 changes: 0 additions & 29 deletions packages/core/src/util/raf.ts

This file was deleted.

55 changes: 6 additions & 49 deletions packages/core/src/zone/ng_zone.ts
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -121,8 +115,7 @@ export class NgZone {
*/
readonly onError: EventEmitter<any> = 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`);
}
Expand All @@ -145,7 +138,6 @@ export class NgZone {
self._inner = self._inner.fork((Zone as any)['longStackTraceZoneSpec']);
}

self.shouldCoalesceEventChangeDetection = shouldCoalesceEventChangeDetection;
forkInnerZoneWithAngularBehavior(self);
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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:
<any>{'isAngularZone': true, 'maybeDelayChangeDetection': maybeDelayChangeDetection},
properties: <any>{'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);
}
},
Expand All @@ -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;
Expand All @@ -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) {
Expand All @@ -357,16 +317,13 @@ 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;
readonly onUnstable: EventEmitter<any> = new EventEmitter();
readonly onMicrotaskEmpty: EventEmitter<any> = new EventEmitter();
readonly onStable: EventEmitter<any> = new EventEmitter();
readonly onError: EventEmitter<any> = new EventEmitter();
readonly shouldCoalesceEventChangeDetection: boolean = false;

run(fn: (...args: any[]) => any, applyThis?: any, applyArgs?: any): any {
return fn.apply(applyThis, applyArgs);
Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/fake_async_spec.ts
Expand Up @@ -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', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/testing/src/ng_zone_mock.ts
Expand Up @@ -16,7 +16,7 @@ import {EventEmitter, Injectable, NgZone} from '@angular/core';
export class MockNgZone extends NgZone {
onStable: EventEmitter<any> = new EventEmitter(false);

constructor() { super({enableLongStackTrace: false, shouldCoalesceEventChangeDetection: false}); }
constructor() { super({enableLongStackTrace: false}); }

run(fn: Function): any { return fn(); }

Expand Down
3 changes: 1 addition & 2 deletions packages/core/testing/src/test_bed.ts
Expand Up @@ -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,
Expand Down
41 changes: 5 additions & 36 deletions packages/platform-browser/test/dom/events/event_manager_spec.ts
Expand Up @@ -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({});
Expand Down Expand Up @@ -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('<div><div></div></div>');
doc.body.appendChild(element);
Expand All @@ -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('<div></div>');
const child = el('<div></div>');
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();
});
});
});
})();

Expand All @@ -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<T>(fn: (...args: any[]) => T, applyThis?: any, applyArgs?: any[]): T { return fn(); }
runOutsideAngular(fn: Function) { return fn(); }
}
2 changes: 1 addition & 1 deletion packages/platform-browser/testing/src/browser_util.ts
Expand Up @@ -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 {
Expand Down
6 changes: 1 addition & 5 deletions tools/public_api_guard/core/core.d.ts
Expand Up @@ -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<any>;
readonly onMicrotaskEmpty: EventEmitter<any>;
readonly onStable: EventEmitter<any>;
readonly onUnstable: EventEmitter<any>;
readonly shouldCoalesceEventChangeDetection: boolean;
constructor({ enableLongStackTrace, shouldCoalesceEventChangeDetection }: {
constructor({ enableLongStackTrace }: {
enableLongStackTrace?: boolean | undefined;
shouldCoalesceEventChangeDetection?: boolean | undefined;
});
run<T>(fn: (...args: any[]) => T, applyThis?: any, applyArgs?: any[]): T;
runGuarded<T>(fn: (...args: any[]) => T, applyThis?: any, applyArgs?: any[]): T;
Expand Down

0 comments on commit 72dba59

Please sign in to comment.