afterRender hooks should support updating state
#54074
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Errors during change detection are terminal and we do not generally attempt to continue processing or recover after one occurs. This helps clean up the `tick` implementation with respect to running `afterRender` hooks.
This change updates the `checkNoChanges` pass to only run once after both view refresh and `afterRender` hooks execute rather than both before and after the hooks. The original motivation was to specifically ensure that the application was in a "clean state" before running the `afterRender` hooks and ensure that `afterRender` hooks don't "fix" `ExpressionChanged...` errors. This, however, adds to the complexity and cost of running change detection in dev mode. Instead, the `checkNoChanges` pass runs once we have done all render-related work and want to ensure that the application state has been synced to the DOM (without additional changes).
atscott
added
area: core
atscott
force-pushed
the
afterRenderChanges
branch
from
86b434e
to
4d20b4b
Compare
atscott
force-pushed
the
afterRenderChanges
branch
2 times, most recently
from
5a26275
to
8282645
Compare
It was always the intent to have `afterRender` hooks allow updating state, as long as those state updates specifically mark views for (re)check. This commit delivers that behavior. Developers can now use `afterRender` hooks to perform further updates within the same change detection cycle rather than needing to defer this work to another round (i.e. `queueMicrotask(() => <<updateState>>)`). This is an important change to support migrating from the `queueMicrotask`-style deferred updates, which are not entirely compatible with zoneless or event `NgZone` run coalescing. When using a microtask to update state after a render, it doesn't work with coalescing because the render may not have happened yet (coalescing and zoneless use `requestAnimationFrame` while the default for `NgZone` is effectively a microtask-based change detection scheduler). Second, when using a `microtask` _during_ change detection to defer updates to the next cycle, this can cause updates to be split across multiple frames with run coalescing and with zoneless (again, because of `requestAnimationFrame`/`macrotask` change detection scheduling).
atscott
force-pushed
the
afterRenderChanges
branch
from
8282645
to
130bf2a
Compare
atscott
added
the
action: global presubmit
atscott
commented
Jan 30, 2024
atscott
commented
Jan 30, 2024
atscott
commented
Jan 30, 2024
atscott
removed
the
action: global presubmit
jessicajaniuk
pushed a commit
that referenced
this pull request
Jan 31, 2024
This change updates the `checkNoChanges` pass to only run once after both view refresh and `afterRender` hooks execute rather than both before and after the hooks. The original motivation was to specifically ensure that the application was in a "clean state" before running the `afterRender` hooks and ensure that `afterRender` hooks don't "fix" `ExpressionChanged...` errors. This, however, adds to the complexity and cost of running change detection in dev mode. Instead, the `checkNoChanges` pass runs once we have done all render-related work and want to ensure that the application state has been synced to the DOM (without additional changes). PR Close #54074
jessicajaniuk
pushed a commit
that referenced
this pull request
Jan 31, 2024
It was always the intent to have `afterRender` hooks allow updating state, as long as those state updates specifically mark views for (re)check. This commit delivers that behavior. Developers can now use `afterRender` hooks to perform further updates within the same change detection cycle rather than needing to defer this work to another round (i.e. `queueMicrotask(() => <<updateState>>)`). This is an important change to support migrating from the `queueMicrotask`-style deferred updates, which are not entirely compatible with zoneless or event `NgZone` run coalescing. When using a microtask to update state after a render, it doesn't work with coalescing because the render may not have happened yet (coalescing and zoneless use `requestAnimationFrame` while the default for `NgZone` is effectively a microtask-based change detection scheduler). Second, when using a `microtask` _during_ change detection to defer updates to the next cycle, this can cause updates to be split across multiple frames with run coalescing and with zoneless (again, because of `requestAnimationFrame`/`macrotask` change detection scheduling). PR Close #54074
jessicajaniuk
pushed a commit
that referenced
this pull request
Jan 31, 2024
This change updates the `checkNoChanges` pass to only run once after both view refresh and `afterRender` hooks execute rather than both before and after the hooks. The original motivation was to specifically ensure that the application was in a "clean state" before running the `afterRender` hooks and ensure that `afterRender` hooks don't "fix" `ExpressionChanged...` errors. This, however, adds to the complexity and cost of running change detection in dev mode. Instead, the `checkNoChanges` pass runs once we have done all render-related work and want to ensure that the application state has been synced to the DOM (without additional changes). PR Close #54074
jessicajaniuk
pushed a commit
that referenced
this pull request
Jan 31, 2024
It was always the intent to have `afterRender` hooks allow updating state, as long as those state updates specifically mark views for (re)check. This commit delivers that behavior. Developers can now use `afterRender` hooks to perform further updates within the same change detection cycle rather than needing to defer this work to another round (i.e. `queueMicrotask(() => <<updateState>>)`). This is an important change to support migrating from the `queueMicrotask`-style deferred updates, which are not entirely compatible with zoneless or event `NgZone` run coalescing. When using a microtask to update state after a render, it doesn't work with coalescing because the render may not have happened yet (coalescing and zoneless use `requestAnimationFrame` while the default for `NgZone` is effectively a microtask-based change detection scheduler). Second, when using a `microtask` _during_ change detection to defer updates to the next cycle, this can cause updates to be split across multiple frames with run coalescing and with zoneless (again, because of `requestAnimationFrame`/`macrotask` change detection scheduling). PR Close #54074
|
This PR was merged into the repository by commit 432afd1. |
atscott
added a commit
to atscott/angular
that referenced
this pull request
Feb 8, 2024
…y reverting angular#54074 In some situations, calling `markForCheck` can result in an infinite loop in seemingly valid scenarios. When a transplanted view is inserted before its declaration, it gets refreshed in the retry loop of `detectChanges`. At this point, the `Dirty` flag has been cleared from all parents. Calling `markForCheck` marks the insertion tree up to the root `Dirty`. If the declaration is checked again as a result (i.e. because it has default change detection) and is reachable because its parent was marked `Dirty`, this can cause an infinite loop. The declaration is refreshed again, so the insertion is marked for refresh (again). We enter an infinite loop if the insertion tree always calls `markForCheck` for some reason (i.e. `{{createReplayObservable() | async}}`). While the case above does fall into an infinite loop, it also truly is a problem in the application. While it's not an infinite synchronous loop, the declaration and insertion are infinitely dirty and will be refreshed on every change detection round.
atscott
added a commit
to atscott/angular
that referenced
this pull request
Feb 8, 2024
…y reverting angular#54074 In some situations, calling `markForCheck` can result in an infinite loop in seemingly valid scenarios. When a transplanted view is inserted before its declaration, it gets refreshed in the retry loop of `detectChanges`. At this point, the `Dirty` flag has been cleared from all parents. Calling `markForCheck` marks the insertion tree up to the root `Dirty`. If the declaration is checked again as a result (i.e. because it has default change detection) and is reachable because its parent was marked `Dirty`, this can cause an infinite loop. The declaration is refreshed again, so the insertion is marked for refresh (again). We enter an infinite loop if the insertion tree always calls `markForCheck` for some reason (i.e. `{{createReplayObservable() | async}}`). While the case above does fall into an infinite loop, it also truly is a problem in the application. While it's not an infinite synchronous loop, the declaration and insertion are infinitely dirty and will be refreshed on every change detection round. Usually `markForCheck` does not have this problem because the `Dirty` flag is not cleared until the very end of change detection. However, if the view did not already have the `Dirty` flag set, it is never cleared because we never entered view refresh. One solution to this problem could be to clear the `Dirty` flag even after skipping view refresh but traversing to children.
atscott
added a commit
to atscott/angular
that referenced
this pull request
Feb 8, 2024
…y reverting angular#54074 In some situations, calling `markForCheck` can result in an infinite loop in seemingly valid scenarios. When a transplanted view is inserted before its declaration, it gets refreshed in the retry loop of `detectChanges`. At this point, the `Dirty` flag has been cleared from all parents. Calling `markForCheck` marks the insertion tree up to the root `Dirty`. If the declaration is checked again as a result (i.e. because it has default change detection) and is reachable because its parent was marked `Dirty`, this can cause an infinite loop. The declaration is refreshed again, so the insertion is marked for refresh (again). We enter an infinite loop if the insertion tree always calls `markForCheck` for some reason (i.e. `{{createReplayObservable() | async}}`). While the case above does fall into an infinite loop, it also truly is a problem in the application. While it's not an infinite synchronous loop, the declaration and insertion are infinitely dirty and will be refreshed on every change detection round. Usually `markForCheck` does not have this problem because the `Dirty` flag is not cleared until the very end of change detection. However, if the view did not already have the `Dirty` flag set, it is never cleared because we never entered view refresh. One solution to this problem could be to clear the `Dirty` flag even after skipping view refresh but traversing to children.
jessicajaniuk
pushed a commit
that referenced
this pull request
Feb 8, 2024
…y reverting #54074 (#54329) In some situations, calling `markForCheck` can result in an infinite loop in seemingly valid scenarios. When a transplanted view is inserted before its declaration, it gets refreshed in the retry loop of `detectChanges`. At this point, the `Dirty` flag has been cleared from all parents. Calling `markForCheck` marks the insertion tree up to the root `Dirty`. If the declaration is checked again as a result (i.e. because it has default change detection) and is reachable because its parent was marked `Dirty`, this can cause an infinite loop. The declaration is refreshed again, so the insertion is marked for refresh (again). We enter an infinite loop if the insertion tree always calls `markForCheck` for some reason (i.e. `{{createReplayObservable() | async}}`). While the case above does fall into an infinite loop, it also truly is a problem in the application. While it's not an infinite synchronous loop, the declaration and insertion are infinitely dirty and will be refreshed on every change detection round. Usually `markForCheck` does not have this problem because the `Dirty` flag is not cleared until the very end of change detection. However, if the view did not already have the `Dirty` flag set, it is never cleared because we never entered view refresh. One solution to this problem could be to clear the `Dirty` flag even after skipping view refresh but traversing to children. PR Close #54329
jessicajaniuk
pushed a commit
that referenced
this pull request
Feb 8, 2024
…y reverting #54074 (#54329) In some situations, calling `markForCheck` can result in an infinite loop in seemingly valid scenarios. When a transplanted view is inserted before its declaration, it gets refreshed in the retry loop of `detectChanges`. At this point, the `Dirty` flag has been cleared from all parents. Calling `markForCheck` marks the insertion tree up to the root `Dirty`. If the declaration is checked again as a result (i.e. because it has default change detection) and is reachable because its parent was marked `Dirty`, this can cause an infinite loop. The declaration is refreshed again, so the insertion is marked for refresh (again). We enter an infinite loop if the insertion tree always calls `markForCheck` for some reason (i.e. `{{createReplayObservable() | async}}`). While the case above does fall into an infinite loop, it also truly is a problem in the application. While it's not an infinite synchronous loop, the declaration and insertion are infinitely dirty and will be refreshed on every change detection round. Usually `markForCheck` does not have this problem because the `Dirty` flag is not cleared until the very end of change detection. However, if the view did not already have the `Dirty` flag set, it is never cleared because we never entered view refresh. One solution to this problem could be to clear the `Dirty` flag even after skipping view refresh but traversing to children. PR Close #54329
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It was always the intent to have
afterRenderhooks allow updatingstate, as long as those state updates specifically mark views for
(re)check. This commit delivers that behavior. Developers can now use
afterRenderhooks to perform further updates within the same changedetection cycle rather than needing to defer this work to another round
(i.e.
queueMicrotask(() => <<updateState>>)). This is an importantchange to support migrating from the
queueMicrotask-style deferredupdates, which are not entirely compatible with zoneless or event
NgZoneruncoalescing.
When using a microtask to update state after a render, it
doesn't work with coalescing because the render may not have happened
yet (coalescing and zoneless use
requestAnimationFramewhile thedefault for
NgZoneis effectively a microtask-based change detectionscheduler). Second, when using a
microtaskduring change detection to deferupdates to the next cycle, this can cause updates to be split across
multiple frames with run coalescing and with zoneless (again, because of
requestAnimationFrame/macrotaskchange detection scheduling).