fix(core): Ensure backwards-referenced transplanted views are refreshed #51537
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
atscott
force-pushed
the
AngularJSImproved
branch
from
a8d78fb
to
0b50fab
Compare
This commit updates the tracking of dirty child views to not specifically track child views to refresh at all. Instead, after the regular change detection cycle, a second pass is done to look for views with the `RefreshView` flag. This pass is done repeatedly until no views with the flag were found during the traversal. This is similar to the 2 runs of change detection in AngularJS but with a huge improvement: rather than actually running change detection twice on the whole app, the second pass is only a traversal to check for any dirty views. This commit ensures transplanted views that are inserted _before_ their declarations are refreshed as part of the same change detection cycle if the declaration is dirty. This is done by running change detection traversal multiple times while there are still transplanted views that need to be refreshed. This same approach can be used for signal-based views to eliminate the `ExpressionChangedAfterItWasCheckedError` when signal updates during change detection cause a view to become dirty (again or for the first time). Several options were identified for rerunning dirty views after they were already checked: 1. Keep track of the dirty views in a global set and just iterate through the set at the end of change detection 2. Rerun `detectChangesInView` immediately (right after it has run) 3. Rerun `detectChangesInView` at the top, as done in this commit The first option is a bit more complicated than it seems. Because of detached views, refreshing a view outside of traversal is difficult. It requires keeping track of "change detection roots" (either the actual root of the CD tree or the first parent/ancestor that is detached). So when running a change detection that causes other views to become dirty, we have to ensure that everything in the set has the same root and potentially even that the dirty view is a actually a descendant of the current change detection (that is, `ChangeDetectorRef.detectChanges` could start somewhere other than the root). Another issue that also exists with the second option is the possibility of potentially refreshing a view more times than necessary when this strategy is applied to signal-based views. Take a simple example of A->B->C where C is a child of B and B is a child of A. C is `CheckAlways` and both `A` and `B` are signal-based. If they are all dirty initially and traversal goes to A, then B, then C and C causes both A and B to be dirty again, if B re-executes first then it will also run C again. Then A reruns and if it causes B to be dirty again, B and C (once again) will refresh. In the above example, the third option (and what is proposed in this commit) performs better. Rerunning from the root ensures that C is only rechecked one additional time rather than 2. A reruns, B reruns, and then C because it is `CheckAlways`. wip
atscott
force-pushed
the
AngularJSImproved
branch
from
0b50fab
to
392855d
Compare
|
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.
This commit updates the tracking of dirty child views to not specifically track child views to refresh at all. Instead, after the regular change detection cycle, a second pass is done to look for views with the
RefreshViewflag. This pass is done repeatedly until no views with the flag were found during the traversal. This is similar to the 2 runs of change detection in AngularJS but with a huge improvement: rather than actually running change detection twice on the whole app, the second pass is only a traversal to check for any dirty views.(Draft for experimentation and to run TGP)