fix(zone.js): allow draining microtasks in Promise.then (through flag)#68134
Conversation
| // To prevent any breaking changes resulting from this change, given that | ||
| // it was already causing a significant number of failures in g3, we have hidden | ||
| // that behavior behind a global configuration flag. Consumers can enable this | ||
| // flag explicitly if they want the microtask queue to be drained as defined | ||
| // in the specification. |
There was a problem hiding this comment.
nit: it wasn't a significant number (IIRC, like 10) but more that there were a few and it's really hard to tell if something just isn't covered by tests and is going to cause a production outage.
| // The order matters! | ||
| if (global[enableNativeMicrotaskDraining]) { | ||
| _isDrainingMicrotaskQueue = false; | ||
| _api.microtaskDrainDone(); |
There was a problem hiding this comment.
I don't specifically recall why order matters and it does look odd that it's different from the other case. do you remember why and can you add more details?
There was a problem hiding this comment.
I don't recall too, it didn't have any issues tbh. I think it should always be the following:
_isDrainingMicrotaskQueue = false;
_api.microtaskDrainDone();Regardless whether microtaskDrainDone would throw.
|
@atscott should The flag eliminates a whole class of subtle bugs — flaky ordering in router navigation, unexpected Promise.then sequencing in event handlers, etc. |
948ccc7 to
acf8233
Compare
|
@atscott I also added more regression tests for other issues being closed with this fix. |
These changes are essentially the same as those introduced in angular#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 angular#44446 Closes angular#55590 Closes angular#51328
acf8233 to
45a4650
Compare
|
Woah, looks like you've opened a lot of issues/PRs recently. While we appreciate contributions from the community, triaging and reviewing a large influx of content in a short time period takes time away from other ongoing projects. As a result, we're closing these issues/PRs to maintain the team's focus. Note that this is not necessarily a rejection of the goals or direction of any of these contributions in particular, so much as a reflection of the team's current capacity and priorities. You are welcome to open a smaller subset of issues/PRs in accordance with our policy focused on the most important and impactful contributions and we will do our best to prioritize a response as soon as possible. |
I wouldn't make a hard push for it. While I agree that it should be the default, it's still likely not worth doing anything about for existing apps, especially given that very few apps are actually at risk of hitting this issue. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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:
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 usingPromise.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