fix(core): NgZone coaleascing options should trigger onStable correctly#40540
fix(core): NgZone coaleascing options should trigger onStable correctly#40540JiaLiPassion wants to merge 1 commit intoangular:masterfrom
Conversation
3f882d8 to
76a579d
Compare
mhevery
left a comment
There was a problem hiding this comment.
Also can you add some jsdocs explaining the purpose of these flags?
76a579d to
ef0548a
Compare
|
@mhevery , I updated the PR and added the comment to explain the flag, also added a comment here #40540 (comment), please review, thanks. |
ef0548a to
53cb493
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
mhevery
left a comment
There was a problem hiding this comment.
Yes, I like this approach much better. Way cleaner and easier to follow.
f6ecbc8 to
453790f
Compare
453790f to
2b7f044
Compare
2ed45f9 to
b39bbbe
Compare
|
@mhevery , I have rebased the PR, please review. Thanks! |
b39bbbe to
71346fd
Compare
fix angular/components#21674 When setting `ngZoneRunCoalescing` to true, `onStable` is not emitted correctly. the reason is before this commit, the code looks like this: ``` // application code call `ngZone.run()` ngzone.run(() => {}); // step 1 // inside NgZone, in the OnInvoke hook, NgZone try to delay the checkStable() function delayChangeDetectionForEvents(zone: NgZonePrivate) { if (zone.lastRequestAnimationFrameId !== -1) { // step 9 return; } zone.lastRequestAnimationFrameId = zone.nativeRequestAnimationFrame.call(global, () => { // step 2 if (!zone.fakeTopEventTask) { zone.fakeTopEventTask = Zone.root.scheduleEventTask('fakeTopEventTask', () => { zone.lastRequestAnimationFrameId = -1; // step 3 updateMicroTaskStatus(zone); // step 4 checkStable(zone); // step 6 }, undefined, () => {}, () => {}); } zone.fakeTopEventTask.invoke(); }); updatemicroTaskStatus(zone); } function updateMicroTaskStatus(zone: NgZonePrivate, ignoreCheckRAFId = false) { if (zone._hasPendingMicrotasks || ((zone.shouldCoalesceEventChangeDetection || zone.shouldCoalesceRunChangeDetection) && zone.lastRequestAnimationFrameId !== -1)) { // step 5 zone.hasPendingMicrotasks = true; } else { zone.hasPendingMicrotasks = false; } } function checkStable(zone: NgZonePrivate) { if (zone._nesting == 0 && !zone.hasPendingMicrotasks && !zone.isStable) { // step 7 try { zone._nesting++; zone.onMicrotaskEmpty.emit(null); ... } // application ref subscribe onMicroTaskEmpty ngzone.onMicroTaskEmpty.subscribe(() => { ngzone.run(() => { // step 8 tick(); }); }); ``` and the process is: 1. step 1: application call ngZone.run() 2. step 2: NgZone delay the checkStable() call in a requestAnimationFrame, and also set zone.lastRequestAnimationFrameId 3. step 3: Inside the requestAnimationFrame callback, reset zone.lastRequestAnimationFrameId first 4. step 4: update microTask status 5, step 5: if zone.lastRequestAnimationFrameId is -1, that means no microTask pending. 6. step 6: checkStable and trigger onMicrotaskEmpty emitter. 7. step 7: ApplicationRef subscribed onMicrotaskEmpty, so it will call another `ngZone.run()` to process tick() 8. step 8: And this new `ngZone.run()` will try to check `zone.lastRequestAnimationFrameId` in `step 9` when trying to delay the checkStable(), and since the zone.lastRequestAnimationFrameId is already reset to -1 in step 3, so this ngZone.run() will run into step 2 again. 9. and become a infinite loop..., so onStable is never emit in this commit, there is a new flag `zone.isCheckStableRunning` added to prevent re-entry when `shouldCoaleascing` flag is enabled.
71346fd to
1827b05
Compare
|
FYI - I reverted the change to the size tracking golden since there was only a newline change at the end of the file after a rebase. That removes the need for the size tracking approval. |
|
caretaker note: Zones change that was previously reverted. Merge & sync with caution |
|
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. |
fix angular/components#21674
When setting
ngZoneRunCoalescingto true,onStableis not emitted correctly.The reason is before this commit, the code looks like is,
And the process is:
zone.lastRequestAnimationFrameId
5, step 5: if zone.lastRequestAnimationFrameId is -1, that means no microTask pending.
ngZone.run()to processtick()
ngZone.run()will try to checkzone.lastRequestAnimationFrameIdinstep 9when trying to delay the checkStable(), and since the zone.lastRequestAnimationFrameId is already reset
to -1 in step 3, so this ngZone.run() will run into step 2 again.
In this commit, the
zone.lastRequestAnimationFrameIdreset is moved aftercheckStable()call.