Views attached inside OnPush components are not checked in some circumstences #52928

pkozlowski-opensource · 2023-11-15T11:23:51Z

Which @angular/* package(s) are the source of the bug?

core

Is this a regression?

No

Description

Currently attaching a view doesn't mark ancestor views for check. This means that there is no way to get to a new view, even if it is dirty initally.

Please provide a link to a minimal reproduction of the bug

https://stackblitz.com/edit/onpush-dynamic-creation-qxpe1m?file=src%2Fmain.ts

Please provide the exception or error you saw

There is no error but the newly inserted view is not change detected.

Please provide the environment you discovered this bug in (run `ng version`)

No response

Anything else?

No response

The text was updated successfully, but these errors were encountered:

Does not fix angular#52928 but `updateAncestorTraversalFlagsOnAttach` is called on view insertion and _should_ have made that work for views dirty from signals but it wasn't updated to read the `dirty` flag when we changed it from sharing the `RefreshView` flag. we've pretty much said working as expected in the past. The created view is `CheckAlways`. There is a question of whether we should automatically mark things for check when the attached view has the `Dirty` flag and/or has the `FirstLViewPass` flag set (or other flags that indicate it definitely needs to be prefreshed).

With this change, detached views no longer cause their parents to be marked dirty and subsequently refreshed. This is a small optimization to stop marking parent views dirty when a detached view is encountered. When running change detection, we stop traversing to children when the view is detached. This means a detached view isn't even reachable from parents so it's wasteful to mark them dirty. There is a risk, albeit _very_ small, that this could exacerbate the issue described in angular#52928 where attaching a dirty view does not mark parents dirty. This would be the case if a detached view is marked dirty and then synchronously attached again. Regardless, it may be prudent to address angular#52928 first or at the same time.

…d refresh Related to angular#52928 but `updateAncestorTraversalFlagsOnAttach` is called on view insertion and _should_ have made that work for views dirty from signals but it wasn't updated to read the `dirty` flag when we changed it from sharing the `RefreshView` flag. For angular#52928, we've traditionally worked under the assumption that this is working as expected. The created view is `CheckAlways`. There is a question of whether we should automatically mark things for check when the attached view has the `Dirty` flag and/or has the `FirstLViewPass` flag set (or other flags that indicate it definitely needs to be prefreshed).

…g CD Another fix related to angular#52928. When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. This commit does not actually fix the reproduction reported in angular#52928. However, that's arguably not something we should try to address because there's no clear indication that a `CheckAlways` view needs to be refreshed during change detection (aside from it being in creation mode/first update pass like the reported issue).

…g change detection When a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). fixes angular#52928

…d refresh (#53001) Related to #52928 but `updateAncestorTraversalFlagsOnAttach` is called on view insertion and _should_ have made that work for views dirty from signals but it wasn't updated to read the `dirty` flag when we changed it from sharing the `RefreshView` flag. For #52928, we've traditionally worked under the assumption that this is working as expected. The created view is `CheckAlways`. There is a question of whether we should automatically mark things for check when the attached view has the `Dirty` flag and/or has the `FirstLViewPass` flag set (or other flags that indicate it definitely needs to be prefreshed). PR Close #53001

…g CD Another fix related to angular#52928. When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. This commit does not actually fix the reproduction reported in angular#52928. However, that's arguably not something we should try to address because there's no clear indication that a `CheckAlways` view needs to be refreshed during change detection (aside from it being in creation mode/first update pass like the reported issue).

…g change detection When a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). fixes angular#52928

…g CD Another fix related to angular#52928. When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. This commit does not actually fix the reproduction reported in angular#52928. However, that's arguably not something we should try to address because there's no clear indication that a `CheckAlways` view needs to be refreshed during change detection (aside from it being in creation mode/first update pass like the reported issue).

…g change detection When a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. fixes angular#52928

…g CD Another fix related to angular#52928. When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. This commit does not actually fix the reproduction reported in angular#52928. However, that's arguably not something we should try to address because there's no clear indication that a `CheckAlways` view needs to be refreshed during change detection (aside from it being in creation mode/first update pass like the reported issue).

…g change detection When a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. fixes angular#52928

…d refresh (angular#53001) Related to angular#52928 but `updateAncestorTraversalFlagsOnAttach` is called on view insertion and _should_ have made that work for views dirty from signals but it wasn't updated to read the `dirty` flag when we changed it from sharing the `RefreshView` flag. For angular#52928, we've traditionally worked under the assumption that this is working as expected. The created view is `CheckAlways`. There is a question of whether we should automatically mark things for check when the attached view has the `Dirty` flag and/or has the `FirstLViewPass` flag set (or other flags that indicate it definitely needs to be prefreshed). PR Close angular#53001

…g CD Another fix related to angular#52928. When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. This commit does not actually fix the reproduction reported in angular#52928. However, that's arguably not something we should try to address because there's no clear indication that a `CheckAlways` view needs to be refreshed during change detection (aside from it being in creation mode/first update pass like the reported issue).

…g change detection When a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. fixes angular#52928

xliu-alterdomus · 2023-12-07T01:37:48Z

Which release can solve this issue? I face the same issue in Angular 16.

…g CD Another fix related to angular#52928. When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. This commit does not actually fix the reproduction reported in angular#52928. However, that's arguably not something we should try to address because there's no clear indication that a `CheckAlways` view needs to be refreshed during change detection (aside from it being in creation mode/first update pass like the reported issue).

…g change detection When a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. fixes angular#52928

…g CD Another fix related to angular#52928. When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. This commit does not actually fix the reproduction reported in angular#52928. However, that's arguably not something we should try to address because there's no clear indication that a `CheckAlways` view needs to be refreshed during change detection (aside from it being in creation mode/first update pass like the reported issue).

…g change detection When a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. fixes angular#52928

…g CD Another fix related to angular#52928. When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. This commit does not actually fix the reproduction reported in angular#52928. However, that's arguably not something we should try to address because there's no clear indication that a `CheckAlways` view needs to be refreshed during change detection (aside from it being in creation mode/first update pass like the reported issue).

…g change detection When a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. fixes angular#52928

…d during CD When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. In addition, when a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. However, this does happen to be a breaking change in some scenarios. The one identified internally was actually depending on change detection _not_ running immediately because it relied on an input value that was set using `ngModel`. Because `ngModel` sets its value in a `Promise`, it is not available until the _next_ change detection cycle. Ensuring created views run in the current change change detection will result in different behavior in this case. Making option the default is the solution to angular#52928. That will have to wait for a major version.

…g CD Another fix related to angular#52928. When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. This commit does not actually fix the reproduction reported in angular#52928. However, that's arguably not something we should try to address because there's no clear indication that a `CheckAlways` view needs to be refreshed during change detection (aside from it being in creation mode/first update pass like the reported issue).

…g change detection When a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. fixes angular#52928

…d during CD When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. In addition, when a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. However, this does happen to be a breaking change in some scenarios. The one identified internally was actually depending on change detection _not_ running immediately because it relied on an input value that was set using `ngModel`. Because `ngModel` sets its value in a `Promise`, it is not available until the _next_ change detection cycle. Ensuring created views run in the current change change detection will result in different behavior in this case. Making option the default is the solution to angular#52928. That will have to wait for a major version.

…d during CD (#53022) When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. In addition, when a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. However, this does happen to be a breaking change in some scenarios. The one identified internally was actually depending on change detection _not_ running immediately because it relied on an input value that was set using `ngModel`. Because `ngModel` sets its value in a `Promise`, it is not available until the _next_ change detection cycle. Ensuring created views run in the current change change detection will result in different behavior in this case. Making option the default is the solution to #52928. That will have to wait for a major version. PR Close #53022

…d refresh (angular#53001) Related to angular#52928 but `updateAncestorTraversalFlagsOnAttach` is called on view insertion and _should_ have made that work for views dirty from signals but it wasn't updated to read the `dirty` flag when we changed it from sharing the `RefreshView` flag. For angular#52928, we've traditionally worked under the assumption that this is working as expected. The created view is `CheckAlways`. There is a question of whether we should automatically mark things for check when the attached view has the `Dirty` flag and/or has the `FirstLViewPass` flag set (or other flags that indicate it definitely needs to be prefreshed). PR Close angular#53001

…d during CD (angular#53022) When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. In addition, when a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. However, this does happen to be a breaking change in some scenarios. The one identified internally was actually depending on change detection _not_ running immediately because it relied on an input value that was set using `ngModel`. Because `ngModel` sets its value in a `Promise`, it is not available until the _next_ change detection cycle. Ensuring created views run in the current change change detection will result in different behavior in this case. Making option the default is the solution to angular#52928. That will have to wait for a major version. PR Close angular#53022

…d refresh (angular#53001) Related to angular#52928 but `updateAncestorTraversalFlagsOnAttach` is called on view insertion and _should_ have made that work for views dirty from signals but it wasn't updated to read the `dirty` flag when we changed it from sharing the `RefreshView` flag. For angular#52928, we've traditionally worked under the assumption that this is working as expected. The created view is `CheckAlways`. There is a question of whether we should automatically mark things for check when the attached view has the `Dirty` flag and/or has the `FirstLViewPass` flag set (or other flags that indicate it definitely needs to be prefreshed). PR Close angular#53001

…d during CD (angular#53022) When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. In addition, when a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. However, this does happen to be a breaking change in some scenarios. The one identified internally was actually depending on change detection _not_ running immediately because it relied on an input value that was set using `ngModel`. Because `ngModel` sets its value in a `Promise`, it is not available until the _next_ change detection cycle. Ensuring created views run in the current change change detection will result in different behavior in this case. Making option the default is the solution to angular#52928. That will have to wait for a major version. PR Close angular#53022

…d refresh (angular#53001) Related to angular#52928 but `updateAncestorTraversalFlagsOnAttach` is called on view insertion and _should_ have made that work for views dirty from signals but it wasn't updated to read the `dirty` flag when we changed it from sharing the `RefreshView` flag. For angular#52928, we've traditionally worked under the assumption that this is working as expected. The created view is `CheckAlways`. There is a question of whether we should automatically mark things for check when the attached view has the `Dirty` flag and/or has the `FirstLViewPass` flag set (or other flags that indicate it definitely needs to be prefreshed). PR Close angular#53001

…d during CD (angular#53022) When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. In addition, when a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. However, this does happen to be a breaking change in some scenarios. The one identified internally was actually depending on change detection _not_ running immediately because it relied on an input value that was set using `ngModel`. Because `ngModel` sets its value in a `Promise`, it is not available until the _next_ change detection cycle. Ensuring created views run in the current change change detection will result in different behavior in this case. Making option the default is the solution to angular#52928. That will have to wait for a major version. PR Close angular#53022

…detection When a view has the `Dirty` flag and is reattached, we should ensure that it is reached and refreshed during the next change detection run from above. In addition, when a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. However, this does happen to be a breaking change in some scenarios. The one identified internally was actually depending on change detection _not_ running immediately because it relied on an input value that was set using `ngModel`. Because `ngModel` sets its value in a `Promise`, it is not available until the _next_ change detection cycle. Ensuring created views run in the current change change detection will result in different behavior in this case. fixes angular#52928 fixes angular#15634 BREAKING CHANGE: Newly created and views marked for check and reattached during change detection are now guaranteed to be refreshed in that same change detection cycle. Previously, if they were attached at a location in the view tree that was already checked, they would either throw `ExpressionChangedAfterItHasBeenCheckedError` or not be refreshed until some future round of change detection. In rare circumstances, this correction can cause issues. We identified one instance that relied on the previous behavior by reading a value on initialization which was queued to be updated in a microtask instead of being available in the current change detection round. The component only read this value during initialization and did not read it again after the microtask updated it.

angular-automatic-lock-bot · 2024-04-06T00:08:41Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pkozlowski-opensource added area: core Issues related to the framework runtime core: change detection core: dynamic view creation labels Nov 15, 2023

ngbot bot added this to the needsTriage milestone Nov 15, 2023

atscott mentioned this issue Nov 17, 2023

perf(core): Stop marking dirty to the root when a detached view is found #52999

Draft

atscott mentioned this issue Nov 17, 2023

fix(core): Reattached views that are dirty from a signal update should refresh #53001

Closed

JeanMeche mentioned this issue Nov 17, 2023

Ensure newly created or dirty and reattached views are reachable by change detection traversal #53022

Closed

angular deleted a comment Dec 14, 2023

JeanMeche mentioned this issue Dec 31, 2023

OnPush + ViewContainerRef.createComponent() + ng-container breaks #53731

Closed

atscott mentioned this issue Mar 6, 2024

fix(core): Ensure views marked for check are refreshed during change … #54735

Closed

atscott closed this as completed in ad045ef Mar 6, 2024

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Views attached inside OnPush components are not checked in some circumstences #52928

Views attached inside OnPush components are not checked in some circumstences #52928

pkozlowski-opensource commented Nov 15, 2023 •

edited by atscott

xliu-alterdomus commented Dec 7, 2023

angular-automatic-lock-bot bot commented Apr 6, 2024

Views attached inside OnPush components are not checked in some circumstences #52928

Views attached inside OnPush components are not checked in some circumstences #52928

Comments

pkozlowski-opensource commented Nov 15, 2023 • edited by atscott

Which @angular/* package(s) are the source of the bug?

Is this a regression?

Description

Please provide a link to a minimal reproduction of the bug

Please provide the exception or error you saw

Please provide the environment you discovered this bug in (run ng version)

Anything else?

xliu-alterdomus commented Dec 7, 2023

angular-automatic-lock-bot bot commented Apr 6, 2024

pkozlowski-opensource commented Nov 15, 2023 •

edited by atscott

Please provide the environment you discovered this bug in (run `ng version`)