Skip to content

Commit fc6a7ee

Browse files
arturovtkirjs
arturovt
authored and
kirjs
committed
fix(zone.js): allow draining microtasks in Promise.then (through flag)
These changes are essentially the same as those introduced in #45273, but they include backward compatibility for applications that explicitly rely on the order in which microtasks are drained. This is critically important for our code and other third-party code, which is beyond our control, to work properly. If a microtask is scheduled within an event listener to be executed "later", it should indeed be executed later and not synchronously, as this would break the expected flow of code execution. The simple code that reproduces the behavior that exists now: ```ts Zone.current.fork({name: 'child'}).run(() => { const div = document.createElement('div'); div.style.height = '200px'; div.style.width = '200px'; div.style.backgroundColor = 'red'; document.body.appendChild(div); function listener() { Promise.resolve().then(() => { div.style.height = '400px'; }); } div.addEventListener('fakeEvent', listener); div.dispatchEvent(new Event('fakeEvent')); console.log(div.getBoundingClientRect().height); // 400 }); ``` The code above logs 400 as the height, but it should actually log 200 because the height is updated in a microtask within the event listener. When using Angular with microfrontend applications, especially when other apps might be using React, zone.js can disrupt the classical order of operations. For example, when using a `react-component/trigger`, it schedules a microtask within an event listener using `Promise.resolve().then(...)` to determine whether the event needs to be re-dispatched. The event is re-dispatched when the layout has changed, which is why a microtask is used. With this change, we introduce a global configuration flag, `__zone_symbol__enable_native_microtask_draining`, to allow consumers to enable microtask draining within a browser microtask. This flag is necessary to prevent any breaking changes resulting from this modification. The previous attempt to address this issue caused a significant number of failures in g3. Therefore, we are hiding that fix behind the configuration flag. Closes #44446 Closes #55590 Closes #51328
1 parent b2cff79 commit fc6a7ee

3 files changed

Lines changed: 259 additions & 38 deletions

File tree

‎packages/zone.js/lib/zone-impl.ts‎

Lines changed: 56 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1131,8 +1131,6 @@ export function initZone(): ZoneType {
11311131
'eventTask': 0,
11321132
};
11331133

1134-
private _parentDelegate: _ZoneDelegate | null;
1135-
11361134
private _forkDlgt: _ZoneDelegate | null;
11371135
private _forkZS: ZoneSpec | null;
11381136
private _forkCurrZone: Zone | null;
@@ -1168,7 +1166,6 @@ export function initZone(): ZoneType {
11681166

11691167
constructor(zone: Zone, parentDelegate: _ZoneDelegate | null, zoneSpec: ZoneSpec | null) {
11701168
this._zone = zone as ZoneImpl;
1171-
this._parentDelegate = parentDelegate;
11721169

11731170
this._forkZS = zoneSpec && (zoneSpec && zoneSpec.onFork ? zoneSpec : parentDelegate!._forkZS);
11741171
this._forkDlgt = zoneSpec && (zoneSpec.onFork ? parentDelegate : parentDelegate!._forkDlgt);
@@ -1438,8 +1435,8 @@ export function initZone(): ZoneType {
14381435
task.runCount++;
14391436
return task.zone.runTask(task, target, args);
14401437
} finally {
1441-
if (_numberOfNestedTaskFrames == 1) {
1442-
drainMicroTaskQueue();
1438+
if (_numberOfNestedTaskFrames === 1 && !global[enableNativeMicrotaskDraining]) {
1439+
drainMicroTaskQueueSynchronously();
14431440
}
14441441
_numberOfNestedTaskFrames--;
14451442
}
@@ -1503,54 +1500,75 @@ export function initZone(): ZoneType {
15031500
const symbolSetTimeout = __symbol__('setTimeout');
15041501
const symbolPromise = __symbol__('Promise');
15051502
const symbolThen = __symbol__('then');
1503+
// To prevent any breaking changes resulting from this change, given that
1504+
// it was already causing a significant number of failures in g3, we have hidden
1505+
// that behavior behind a global configuration flag. Consumers can enable this
1506+
// flag explicitly if they want the microtask queue to be drained as defined
1507+
// in the specification.
1508+
const enableNativeMicrotaskDraining = __symbol__('enable_native_microtask_draining');
15061509
let _microTaskQueue: Task[] = [];
1507-
let _isDrainingMicrotaskQueue: boolean = false;
1510+
let _isDrainingMicrotaskQueue = false;
15081511
let nativeMicroTaskQueuePromise: any;
15091512

15101513
function nativeScheduleMicroTask(func: Function) {
1511-
if (!nativeMicroTaskQueuePromise) {
1512-
if (global[symbolPromise]) {
1513-
nativeMicroTaskQueuePromise = global[symbolPromise].resolve(0);
1514-
}
1514+
if (!nativeMicroTaskQueuePromise && global[symbolPromise]) {
1515+
nativeMicroTaskQueuePromise = global[symbolPromise].resolve(0);
15151516
}
1517+
15161518
if (nativeMicroTaskQueuePromise) {
1517-
let nativeThen = nativeMicroTaskQueuePromise[symbolThen];
1518-
if (!nativeThen) {
1519-
// native Promise is not patchable, we need to use `then` directly
1520-
// issue 1078
1521-
nativeThen = nativeMicroTaskQueuePromise['then'];
1522-
}
1523-
nativeThen.call(nativeMicroTaskQueuePromise, func);
1519+
const thenFn = nativeMicroTaskQueuePromise[symbolThen] ?? nativeMicroTaskQueuePromise['then']; // fallback for non-patchable Promise
1520+
// Use the resolved native promise to schedule the microtask
1521+
thenFn.call(nativeMicroTaskQueuePromise, func);
15241522
} else {
1523+
// Fallback to setTimeout if native promise is unavailable
15251524
global[symbolSetTimeout](func, 0);
15261525
}
15271526
}
15281527

15291528
function scheduleMicroTask(task?: MicroTask) {
1530-
// if we are not running in any task, and there has not been anything scheduled
1531-
// we must bootstrap the initial task creation by manually scheduling the drain
1532-
if (_numberOfNestedTaskFrames === 0 && _microTaskQueue.length === 0) {
1533-
// We are not running in Task, so we need to kickstart the microtask queue.
1534-
nativeScheduleMicroTask(drainMicroTaskQueue);
1529+
const isNativeDrainingEnabled = global[enableNativeMicrotaskDraining];
1530+
const shouldDrainWithNative =
1531+
isNativeDrainingEnabled && _microTaskQueue.length === 0 && !_isDrainingMicrotaskQueue;
1532+
const shouldDrainWithoutNative =
1533+
!isNativeDrainingEnabled && _numberOfNestedTaskFrames === 0 && _microTaskQueue.length === 0;
1534+
1535+
if (shouldDrainWithNative || shouldDrainWithoutNative) {
1536+
// Start draining the microtask queue if:
1537+
// - Native draining is enabled and not currently in progress, or
1538+
// - Native draining is disabled, and there are no nested tasks and no queued microtasks.
1539+
nativeScheduleMicroTask(drainMicroTaskQueueSynchronously);
1540+
}
1541+
1542+
if (task) {
1543+
_microTaskQueue.push(task);
15351544
}
1536-
task && _microTaskQueue.push(task);
15371545
}
15381546

1539-
function drainMicroTaskQueue() {
1540-
if (!_isDrainingMicrotaskQueue) {
1541-
_isDrainingMicrotaskQueue = true;
1542-
while (_microTaskQueue.length) {
1543-
const queue = _microTaskQueue;
1544-
_microTaskQueue = [];
1545-
for (let i = 0; i < queue.length; i++) {
1546-
const task = queue[i];
1547-
try {
1548-
task.zone.runTask(task, null, null);
1549-
} catch (error) {
1550-
_api.onUnhandledError(error as Error);
1551-
}
1547+
function drainMicroTaskQueueSynchronously() {
1548+
if (_isDrainingMicrotaskQueue) {
1549+
return;
1550+
}
1551+
1552+
_isDrainingMicrotaskQueue = true;
1553+
1554+
while (_microTaskQueue.length) {
1555+
const queue = _microTaskQueue;
1556+
_microTaskQueue = [];
1557+
1558+
for (const task of queue) {
1559+
try {
1560+
task.zone.runTask(task, null, null);
1561+
} catch (error) {
1562+
_api.onUnhandledError(error as Error);
15521563
}
15531564
}
1565+
}
1566+
1567+
// The order matters!
1568+
if (global[enableNativeMicrotaskDraining]) {
1569+
_isDrainingMicrotaskQueue = false;
1570+
_api.microtaskDrainDone();
1571+
} else {
15541572
_api.microtaskDrainDone();
15551573
_isDrainingMicrotaskQueue = false;
15561574
}
@@ -1579,7 +1597,7 @@ export function initZone(): ZoneType {
15791597
currentZoneFrame: () => _currentZoneFrame,
15801598
onUnhandledError: noop,
15811599
microtaskDrainDone: noop,
1582-
scheduleMicroTask: scheduleMicroTask,
1600+
scheduleMicroTask,
15831601
showUncaughtError: () => !(ZoneImpl as any)[__symbol__('ignoreConsoleErrorUncaughtError')],
15841602
patchEventTarget: () => [],
15851603
patchOnProperties: noop,
@@ -1599,7 +1617,7 @@ export function initZone(): ZoneType {
15991617
attachOriginToPatched: () => noop,
16001618
_redefineProperty: () => noop,
16011619
patchCallbacks: () => noop,
1602-
nativeScheduleMicroTask: nativeScheduleMicroTask,
1620+
nativeScheduleMicroTask,
16031621
};
16041622
let _currentZoneFrame: ZoneFrame = {parent: null, zone: new ZoneImpl(null, null)};
16051623
let _currentTask: Task | null = null;

‎packages/zone.js/lib/zone.configurations.api.ts‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -555,6 +555,59 @@ declare global {
555555
* the user with a string returned from the event handler.
556556
*/
557557
__zone_symbol__enable_beforeunload?: boolean;
558+
559+
/**
560+
* https://github.com/angular/angular/issues/41506
561+
* https://github.com/angular/angular/issues/44446
562+
*
563+
* By default, `zone.js` maintains a microtask queue manually, which means the microtask
564+
* queue is drained whenever `zone.js` decides to do so under certain circumstances.
565+
* Typically, `zone.js` invokes a task (e.g., an event task) and, after invoking the task,
566+
* checks whether the number of nested task frames is equal to 1 before calling the microtask
567+
* queue draining.
568+
* As thus, there are cases when the microtask queue may be drained synchronously after an
569+
* event task is invoked (if it’s the very first task in the call stack).
570+
* Tasks may actually schedule other tasks, thereby incrementing the stack frame.
571+
* In that case, the microtask queue might be drained after the last task is invoked.
572+
*
573+
* Given that code:
574+
* ```js
575+
* Zone.current.fork({name: 'child'}).run(() => {
576+
* const div = document.createElement('div');
577+
* div.style.height = '200px';
578+
* div.style.width = '200px';
579+
* div.style.backgroundColor = 'red';
580+
* document.body.appendChild(div);
581+
*
582+
* function listener() {
583+
* Promise.resolve().then(() => {
584+
* div.style.height = '400px';
585+
* });
586+
* }
587+
*
588+
* div.addEventListener('fakeEvent', listener);
589+
* div.dispatchEvent(new Event('fakeEvent'));
590+
* console.log(div.getBoundingClientRect().height); // 400
591+
* });
592+
* ```
593+
*
594+
* We would assume that "200" would be logged. However, with `zone.js`, "400" will
595+
* be logged first because it drains the microtask queue too early, as the `fakeEvent`
596+
* event task is the very top task on the stack.
597+
*
598+
* https://promisesaplus.com/#the-then-method
599+
* According to the spec: `onFulfilled` or `onRejected` must not be called until the
600+
* execution context stack contains only platform code.
601+
*
602+
* You may consider enabling the flag below. This will ensure that microtask draining
603+
* does not happen synchronously and always occurs within a browser microtask.
604+
*
605+
* This is critically important for our code and other third-party code, which is
606+
* beyond our control, to work properly. If a microtask is scheduled within an event
607+
* listener to be executed "later", it should indeed be executed later and not synchronously,
608+
* as this would break the expected flow of code execution.
609+
*/
610+
__zone_symbol__enable_native_microtask_draining?: boolean;
558611
}
559612

560613
/**

‎packages/zone.js/test/common/zone.spec.ts‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
* Use of this source code is governed by an MIT-style license that can be
66
* found in the LICENSE file at https://angular.dev/license
77
*/
8+
9+
import {isNode} from '../../lib/common/utils';
10+
811
describe('Zone Common', function () {
912
it('should have a name', function () {
1013
expect(Zone.current.name).toBeDefined();
@@ -432,6 +435,153 @@ describe('Zone Common', function () {
432435
log = [];
433436
});
434437

438+
// https://github.com/angular/angular/issues/44446
439+
// https://github.com/angular/angular/issues/55590
440+
// https://github.com/angular/angular/issues/51328
441+
describe('__zone_symbol__enable_native_microtask_draining', () => {
442+
it('should not drain the microtask queue too early without task (if the flag is enabled)', (done) => {
443+
// Regression test for https://github.com/angular/angular/issues/44446.
444+
// Verifies that a microtask scheduled inside an event task is not drained
445+
// synchronously mid-stack when the native draining flag is enabled.
446+
const globalAny = global as any;
447+
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = true;
448+
const zone = Zone.current;
449+
const event = zone.scheduleEventTask(
450+
'test',
451+
() => {
452+
log.push('eventTask');
453+
zone.scheduleMicroTask('test', () => log.push('microTask'));
454+
},
455+
undefined,
456+
noop,
457+
noop,
458+
);
459+
log.push('after schedule eventTask');
460+
expect(log).toEqual(['after schedule eventTask']);
461+
event.invoke();
462+
// At this point, we should not have invoked the microtask.
463+
expect(log).toEqual(['after schedule eventTask', 'eventTask']);
464+
globalAny[Zone.__symbol__('setTimeout')](() => {
465+
expect(log).toEqual(['after schedule eventTask', 'eventTask', 'microTask']);
466+
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = false;
467+
done();
468+
});
469+
});
470+
471+
it('should not drain the microtask queue too early (if the flag is enabled)', (done) => {
472+
// We need `document` in this test.
473+
if (isNode) {
474+
done();
475+
return;
476+
}
477+
478+
// Regression test for https://github.com/angular/angular/issues/44446.
479+
// Verifies that a Promise.then() callback scheduled inside a DOM event listener
480+
// is not drained synchronously before the main stack unwinds.
481+
482+
const globalAny = global as any;
483+
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = true;
484+
const zone = Zone.current;
485+
486+
zone.run(() => {
487+
const listener = () => {
488+
Promise.resolve().then(() => log.push('promise.then'));
489+
};
490+
491+
document.body.addEventListener('click', listener);
492+
document.body.click();
493+
log.push('main stack');
494+
495+
globalAny[Zone.__symbol__('setTimeout')](() => {
496+
document.body.removeEventListener('click', listener);
497+
expect(log).toEqual(['main stack', 'promise.then']);
498+
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = false;
499+
done();
500+
});
501+
});
502+
});
503+
504+
it('should surface unhandled promise rejections via unhandledrejection event (if the flag is enabled)', async () => {
505+
// We need `window` in this test.
506+
if (isNode) {
507+
return;
508+
}
509+
510+
// Regression test for https://github.com/angular/angular/issues/55590.
511+
// Verifies that unhandled promise rejections originating outside zone.js-patched
512+
// code (e.g. a plain <script> tag) still surface via the native unhandledrejection
513+
// event when the native draining flag is enabled.
514+
515+
let rejectionEvent: PromiseRejectionEvent | null = null;
516+
517+
const onError = window.onerror;
518+
window.onerror = () => {};
519+
520+
const globalAny = global as any;
521+
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = true;
522+
523+
await jasmine.spyOnGlobalErrorsAsync(async () => {
524+
const handler = (event: PromiseRejectionEvent) => {
525+
window.removeEventListener('unhandledrejection', handler);
526+
rejectionEvent = event;
527+
};
528+
window.addEventListener('unhandledrejection', handler);
529+
530+
const script = document.createElement('script');
531+
script.innerHTML = "Promise.reject('Error happened :(')";
532+
document.body.append(script);
533+
534+
// Wait until the event task is dispatched.
535+
await new Promise((resolve) => setTimeout(resolve, 10));
536+
537+
window.onerror = onError;
538+
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = false;
539+
540+
expect(rejectionEvent).not.toBeNull();
541+
expect(rejectionEvent!.reason).toBe('Error happened :(');
542+
});
543+
});
544+
545+
it('should not surface a rejection as unhandled when catch is attached (if the flag is enabled)', async () => {
546+
// We need `window` in this test.
547+
if (isNode) {
548+
return;
549+
}
550+
551+
// Regression test for https://github.com/angular/angular/issues/51328.
552+
// Verifies that an internal `await Promise.reject()` inside an async IIFE does not
553+
// fire `unhandledrejection` when a `.catch()` is attached to the outer promise.
554+
555+
let rejectionEvent: PromiseRejectionEvent | null = null;
556+
557+
const onError = window.onerror;
558+
window.onerror = () => {};
559+
560+
const globalAny = global as any;
561+
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = true;
562+
563+
await jasmine.spyOnGlobalErrorsAsync(async () => {
564+
const handler = (event: PromiseRejectionEvent) => {
565+
window.removeEventListener('unhandledrejection', handler);
566+
rejectionEvent = event;
567+
};
568+
window.addEventListener('unhandledrejection', handler);
569+
570+
(async function () {
571+
await Promise.reject(2);
572+
})().catch(() => {});
573+
574+
// Wait until the event task is dispatched.
575+
await new Promise((resolve) => setTimeout(resolve, 10));
576+
577+
window.onerror = onError;
578+
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = false;
579+
580+
expect(rejectionEvent).toBeNull();
581+
});
582+
});
583+
});
584+
435585
it('should not drain the microtask queue too early', () => {
436586
const z = Zone.current;
437587
const event = z.scheduleEventTask('test', () => log.push('eventTask'), undefined, noop, noop);

0 commit comments

Comments
 (0)