New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(core): Avoid refreshing a host view twice when using transplanted… #53021
Conversation
345594b
to
e0cc73b
Compare
e0cc73b
to
dd64c6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about instead, everywhere we call refreshView
, we switch to using detectChangesInternal
but set the Dirty
flag beforehand?
07b6876
to
cd92806
Compare
@@ -38,5 +38,6 @@ export function applyChanges(component: {}): void { | |||
*/ | |||
function detectChanges(component: {}): void { | |||
const view = getComponentViewByInstance(component); | |||
view[FLAGS] |= LViewFlags.RefreshView; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the commit message talks about the importance of RefreshView
as opposed to Dirty
here, can you document that in a comment?
… views This change fixes and issue where the expectation was that change detection always goes through `detectChangesInView`. In reality, `detectChangesInternal` directly calls `refreshView` and refreshes a view directly without checking if it was dirty (to my discontent). This update changes the implementation of `detectChangesInternal` to actually be "detect changes" not "force refresh of root view and detect changes". In addition, it adds the refresh flag to APIs that were previously calling `detectChangesInternal` so we get the same behavior as before (host view is forced to refresh). Note that the use of `RefreshView` instead of `Dirty` is _intentional_ here. The `RefreshView` flag is cleared before refreshing the view while the `Dirty` flag is cleared at the very end. Using the `Dirty` flag could have consequences because it is a more long-lasting change to the view flags. Because `detectChangesInView` will immediately clear the `RefreshView` flag, this change is much more limited and does not result in a different set of flags during the view refresh.
cd92806
to
3fac113
Compare
This PR was merged into the repository by commit 2565121. |
… views (#53021) This change fixes and issue where the expectation was that change detection always goes through `detectChangesInView`. In reality, `detectChangesInternal` directly calls `refreshView` and refreshes a view directly without checking if it was dirty (to my discontent). This update changes the implementation of `detectChangesInternal` to actually be "detect changes" not "force refresh of root view and detect changes". In addition, it adds the refresh flag to APIs that were previously calling `detectChangesInternal` so we get the same behavior as before (host view is forced to refresh). Note that the use of `RefreshView` instead of `Dirty` is _intentional_ here. The `RefreshView` flag is cleared before refreshing the view while the `Dirty` flag is cleared at the very end. Using the `Dirty` flag could have consequences because it is a more long-lasting change to the view flags. Because `detectChangesInView` will immediately clear the `RefreshView` flag, this change is much more limited and does not result in a different set of flags during the view refresh. PR Close #53021
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. |
… views (angular#53021) This change fixes and issue where the expectation was that change detection always goes through `detectChangesInView`. In reality, `detectChangesInternal` directly calls `refreshView` and refreshes a view directly without checking if it was dirty (to my discontent). This update changes the implementation of `detectChangesInternal` to actually be "detect changes" not "force refresh of root view and detect changes". In addition, it adds the refresh flag to APIs that were previously calling `detectChangesInternal` so we get the same behavior as before (host view is forced to refresh). Note that the use of `RefreshView` instead of `Dirty` is _intentional_ here. The `RefreshView` flag is cleared before refreshing the view while the `Dirty` flag is cleared at the very end. Using the `Dirty` flag could have consequences because it is a more long-lasting change to the view flags. Because `detectChangesInView` will immediately clear the `RefreshView` flag, this change is much more limited and does not result in a different set of flags during the view refresh. PR Close angular#53021
… views (angular#53021) This change fixes and issue where the expectation was that change detection always goes through `detectChangesInView`. In reality, `detectChangesInternal` directly calls `refreshView` and refreshes a view directly without checking if it was dirty (to my discontent). This update changes the implementation of `detectChangesInternal` to actually be "detect changes" not "force refresh of root view and detect changes". In addition, it adds the refresh flag to APIs that were previously calling `detectChangesInternal` so we get the same behavior as before (host view is forced to refresh). Note that the use of `RefreshView` instead of `Dirty` is _intentional_ here. The `RefreshView` flag is cleared before refreshing the view while the `Dirty` flag is cleared at the very end. Using the `Dirty` flag could have consequences because it is a more long-lasting change to the view flags. Because `detectChangesInView` will immediately clear the `RefreshView` flag, this change is much more limited and does not result in a different set of flags during the view refresh. PR Close angular#53021
… views (angular#53021) This change fixes and issue where the expectation was that change detection always goes through `detectChangesInView`. In reality, `detectChangesInternal` directly calls `refreshView` and refreshes a view directly without checking if it was dirty (to my discontent). This update changes the implementation of `detectChangesInternal` to actually be "detect changes" not "force refresh of root view and detect changes". In addition, it adds the refresh flag to APIs that were previously calling `detectChangesInternal` so we get the same behavior as before (host view is forced to refresh). Note that the use of `RefreshView` instead of `Dirty` is _intentional_ here. The `RefreshView` flag is cleared before refreshing the view while the `Dirty` flag is cleared at the very end. Using the `Dirty` flag could have consequences because it is a more long-lasting change to the view flags. Because `detectChangesInView` will immediately clear the `RefreshView` flag, this change is much more limited and does not result in a different set of flags during the view refresh. PR Close angular#53021
… views
This change fixes and issue where the expectation was that change detection always goes through
detectChangesInView
. In reality,detectChangesInternal
directly callsrefreshView
and refreshes a view directly without checking if it was dirty (to my discontent).This update clears the refresh flags at the top of
refreshView
as well as before traversing child views inTargeted
mode. The issue only partially exists with signals in that the flags aren't cleared but we don't end up refreshing the view a second time because theconsumerPollProducersForChange
returnsfalse
the second time we enterdetectChangesInView
.