perf(core): Update LView consumer to only mark component for check #52302

atscott · 2023-10-20T19:17:39Z

This commit updates the reactive template and host binding consumers to only mark their declaration components for refresh, but not parents/ancestors.

This also updates the AfterViewChecked hook to run when a component is refreshed during change detection but its host is not. It is reasonable to expect that the ngAfterViewChecked lifecycle hook will run when a signal updates and the component is refreshed. The hooks are typically run when the host is refreshed so without this change, the update to not mark ancestors dirty would have caused ngAfterViewChecked to not run.

resolves #14628
resolves #22646

resolves #34347 - this is not the direct request of the issue but generally forcing change detection to run is necessary only because a value was updated that needs to be synced to the DOM. Values that use signals will mark the component for check automatically so accessing the ChangeDetectorRef of a child is not necessary. The other part of this request was to avoid the need to "mark all views for checking since it wouldn't affect anything but itself". This is directly addressed by this PR - updating a signal that's read in the child view's template will not cause ancestors/"all views" to be refreshed.

e-oz · 2023-10-21T04:32:07Z

I am excited to see this, amazing news!

Just out of curiosity, as was asked in the first referenced issue: how can Angular know from which component change detection should start? Currently, it just starts from the root and that's how it finds all the onPush components, marked for check.

Could you please explain the new algorithm?

It is just out of curiosity - I’m happy to see this work.

JeanMeche · 2023-10-21T17:55:54Z

@e-oz The answer lies in markAncestorsForTraversal, it stills starts from root but non-dirty components are skipped.

angular/packages/core/src/render3/util/view_utils.ts

Lines 215 to 222 in 3278966

    
           /** 
        
            * Ensures views above the given `lView` are traversed during change detection even when they are 
        
            * not dirty. 
        
            * 
        
            * This is done by setting the `HAS_CHILD_VIEWS_TO_REFRESH` flag up to the root, stopping when the 
        
            * flag is already `true` or the `lView` is detached. 
        
            */ 
        
           export function markAncestorsForTraversal(lView: LView) {

e-oz · 2023-10-21T18:11:21Z

Thanks a lot, @JeanMeche

packages/core/src/render3/instructions/change_detection.ts

packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts

packages/core/src/render3/util/view_utils.ts

This commit updates the reactive template and host binding consumers to only mark their declaration components for refresh, but not parents/ancestors. This also updates the `AfterViewChecked` hook to run when a component is refreshed during change detection but its host is not. It is reasonable to expect that the `ngAfterViewChecked` lifecycle hook will run when a signal updates and the component is refreshed. The hooks are typically run when the host is refreshed so without this change, the update to not mark ancestors dirty would have caused `ngAfterViewChecked` to not run. resolves angular#14628 resolves angular#22646 resolves angular#34347 - this is not the direct request of the issue but generally forcing change detection to run is necessary only because a value was updated that needs to be synced to the DOM. Values that use signals will mark the component for check automatically so accessing the `ChangeDetectorRef` of a child is not necessary. The other part of this request was to avoid the need to "mark all views for checking since it wouldn't affect anything but itself". This is directly addressed by this commit - updating a signal that's read in the view's template will not cause ancestors/"all views" to be refreshed.

packages/core/src/render3/instructions/change_detection.ts

alxhub · 2023-10-31T19:37:58Z

This PR was merged into the repository by commit ac2d0c6.

…52302) This commit updates the reactive template and host binding consumers to only mark their declaration components for refresh, but not parents/ancestors. This also updates the `AfterViewChecked` hook to run when a component is refreshed during change detection but its host is not. It is reasonable to expect that the `ngAfterViewChecked` lifecycle hook will run when a signal updates and the component is refreshed. The hooks are typically run when the host is refreshed so without this change, the update to not mark ancestors dirty would have caused `ngAfterViewChecked` to not run. resolves #14628 resolves #22646 resolves #34347 - this is not the direct request of the issue but generally forcing change detection to run is necessary only because a value was updated that needs to be synced to the DOM. Values that use signals will mark the component for check automatically so accessing the `ChangeDetectorRef` of a child is not necessary. The other part of this request was to avoid the need to "mark all views for checking since it wouldn't affect anything but itself". This is directly addressed by this commit - updating a signal that's read in the view's template will not cause ancestors/"all views" to be refreshed. PR Close #52302

…tion The significance of the combination of angular#51854 and angular#52302 went mostly unnoticed. The first removed a unidirectional data flow constraint for transplanted views and the second updated the signal implementation to share transplanted view logic. The result is that we automatically get behavior that (mostly) removes `ExpressionChangedAfterItWasCheckedError` when signals are used to drive application state to DOM synchronization. fixes angular#50320

…tion (#52476) The significance of the combination of #51854 and #52302 went mostly unnoticed. The first removed a unidirectional data flow constraint for transplanted views and the second updated the signal implementation to share transplanted view logic. The result is that we automatically get behavior that (mostly) removes `ExpressionChangedAfterItWasCheckedError` when signals are used to drive application state to DOM synchronization. fixes #50320 PR Close #52476

…tion (angular#52476) The significance of the combination of angular#51854 and angular#52302 went mostly unnoticed. The first removed a unidirectional data flow constraint for transplanted views and the second updated the signal implementation to share transplanted view logic. The result is that we automatically get behavior that (mostly) removes `ExpressionChangedAfterItWasCheckedError` when signals are used to drive application state to DOM synchronization. fixes angular#50320 PR Close angular#52476

angular-automatic-lock-bot · 2023-12-01T00:10:12Z

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.}

…ngular#52302) This commit updates the reactive template and host binding consumers to only mark their declaration components for refresh, but not parents/ancestors. This also updates the `AfterViewChecked` hook to run when a component is refreshed during change detection but its host is not. It is reasonable to expect that the `ngAfterViewChecked` lifecycle hook will run when a signal updates and the component is refreshed. The hooks are typically run when the host is refreshed so without this change, the update to not mark ancestors dirty would have caused `ngAfterViewChecked` to not run. resolves angular#14628 resolves angular#22646 resolves angular#34347 - this is not the direct request of the issue but generally forcing change detection to run is necessary only because a value was updated that needs to be synced to the DOM. Values that use signals will mark the component for check automatically so accessing the `ChangeDetectorRef` of a child is not necessary. The other part of this request was to avoid the need to "mark all views for checking since it wouldn't affect anything but itself". This is directly addressed by this commit - updating a signal that's read in the view's template will not cause ancestors/"all views" to be refreshed. PR Close angular#52302

atscott force-pushed the signalsPerComponentCd branch 3 times, most recently from 3f8ca6d to 18e3cc8 Compare October 20, 2023 21:07

atscott force-pushed the signalsPerComponentCd branch from 538caae to 9507dbc Compare October 25, 2023 00:04

atscott marked this pull request as ready for review October 26, 2023 20:37

atscott requested a review from alxhub October 26, 2023 20:46

atscott added target: rc This PR is targeted for the next release-candidate refactoring Issue that involves refactoring or code-cleanup area: core Issues related to the framework runtime core: change detection cross-cutting: signals labels Oct 26, 2023

ngbot bot modified the milestone: Backlog Oct 26, 2023

pkozlowski-opensource reviewed Oct 27, 2023

View reviewed changes

packages/core/src/render3/instructions/change_detection.ts Outdated Show resolved Hide resolved

atscott force-pushed the signalsPerComponentCd branch 3 times, most recently from 91dbfc0 to f9c8ba7 Compare October 27, 2023 16:04