Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(core): Ensure backwards-referenced transplanted views are refreshed #51854

Closed
wants to merge 1 commit into from
Closed

fix(core): Ensure backwards-referenced transplanted views are refreshed #51854

wants to merge 1 commit into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Sep 21, 2023

Based off of https://github.com/angular/angular/pull/51515This commit runs change detection in a loop while there are still dirty
views to be refreshed in the tree. At the moment, this only applies to
transplanted views but will also apply to views with changed signals.

fixes #49801

dylhunn
dylhunn approved these changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: public-api

jessicajaniuk
jessicajaniuk approved these changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: public-api

@dylhunn dylhunn added area: core Issues related to the framework runtime core: reactivity Work related to fine-grained reactivity in the core framework labels Sep 27, 2023
@ngbot ngbot bot added this to the Backlog milestone Sep 27, 2023
alxhub
alxhub approved these changes Sep 29, 2023
View reviewed changes
packages/core/src/render3/instructions/change_detection.ts Outdated Show resolved Hide resolved
packages/core/src/linker/view_container_ref.ts Outdated Show resolved Hide resolved
pkozlowski-opensource
pkozlowski-opensource reviewed Oct 11, 2023
View reviewed changes
packages/core/src/render3/instructions/change_detection.ts
/**
* The maximum number of times the change detection traversal will rerun before throwing an error.
*/
const MAXIMUM_REFRESH_RERUNS = 100;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I remember we were discussing lowering this number?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not recall this conversation. But the number is an arbitrary decision by me. I have no opinions on what it should be.

@atscott atscott added the action: global presubmit The PR is in need of a google3 global presubmit label Oct 11, 2023
@atscott
fix(core): Ensure backwards-referenced transplanted views are refreshed
14734ac 
This commit runs change detection in a loop while there are still dirty
views to be refreshed in the tree. At the moment, this only applies to
transplanted views but will also apply to views with changed signals.

fixes angular#49801
@atscott
Copy link
Contributor Author

atscott commented Oct 12, 2023

green TGP

@atscott atscott added action: review The PR is still awaiting reviews from at least one requested reviewer action: merge The PR is ready for merge by the caretaker and removed action: global presubmit The PR is in need of a google3 global presubmit action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 16, 2023
@atscott atscott added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 24, 2023
@atscott
Copy link
Contributor Author

atscott commented Oct 24, 2023

caretaker: green TGP, current failures are existing failures

@atscott atscott added the target: rc This PR is targeted for the next release-candidate label Oct 24, 2023
@dylhunn dylhunn removed the request for review from AndrewKushnir October 24, 2023 21:37
@dylhunn
Copy link
Contributor

dylhunn commented Oct 24, 2023

This PR was merged into the repository by commit 76152a5.

@dylhunn dylhunn closed this in 76152a5 Oct 24, 2023
dylhunn pushed a commit that referenced this pull request Oct 24, 2023
@atscott @dylhunn
fix(core): Ensure backwards-referenced transplanted views are refresh…
1dd8558 
…ed (#51854)

This commit runs change detection in a loop while there are still dirty
views to be refreshed in the tree. At the moment, this only applies to
transplanted views but will also apply to views with changed signals.

fixes #49801

PR Close #51854
atscott added a commit to atscott/angular that referenced this pull request Oct 31, 2023
@atscott
refactor(core): Remove warning about signal set during template execu…
92db061 
…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
@atscott atscott mentioned this pull request Oct 31, 2023
Closed
atscott added a commit to atscott/angular that referenced this pull request Oct 31, 2023
@atscott
refactor(core): Remove warning about signal set during template execu…
4f45dba 
…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
atscott added a commit to atscott/angular that referenced this pull request Oct 31, 2023
@atscott
refactor(core): Remove warning about signal set during template execu…
e09a2cf 
…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
atscott added a commit to atscott/angular that referenced this pull request Oct 31, 2023
@atscott
refactor(core): Remove warning about signal set during template execu…
5a13203 
…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
atscott added a commit to atscott/angular that referenced this pull request Nov 2, 2023
@atscott
refactor(core): Remove warning about signal set during template execu…
133b9ee 
…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
alxhub pushed a commit that referenced this pull request Nov 2, 2023
@atscott @alxhub
refactor(core): Remove warning about signal set during template execu…
58d74a2 
…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
alxhub pushed a commit that referenced this pull request Nov 2, 2023
@atscott @alxhub
refactor(core): Remove warning about signal set during template execu…
2ce2ad1 
…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
Abseil-byte pushed a commit to Abseil-byte/angular that referenced this pull request Nov 5, 2023
@atscott @Abseil-byte
refactor(core): Remove warning about signal set during template execu…
f78b64e 
…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
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 24, 2023
tbondwilkinson pushed a commit to tbondwilkinson/angular that referenced this pull request Dec 6, 2023
@atscott @tbondwilkinson
fix(core): Ensure backwards-referenced transplanted views are refresh…
7e99798 
…ed (angular#51854)

This commit runs change detection in a loop while there are still dirty
views to be refreshed in the tree. At the moment, this only applies to
transplanted views but will also apply to views with changed signals.

fixes angular#49801

PR Close angular#51854
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@atscott @ChellappanRajan
fix(core): Ensure backwards-referenced transplanted views are refresh…
7768246 
…ed (angular#51854)

This commit runs change detection in a loop while there are still dirty
views to be refreshed in the tree. At the moment, this only applies to
transplanted views but will also apply to views with changed signals.

fixes angular#49801

PR Close angular#51854
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource left review comments

@AndrewKushnir AndrewKushnir AndrewKushnir left review comments

@alxhub alxhub alxhub approved these changes

@dylhunn dylhunn dylhunn approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime core: reactivity Work related to fine-grained reactivity in the core framework merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: rc This PR is targeted for the next release-candidate
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

Backward-referenced transplanted views and ChangeDetectionStrategy.OnPush are not fully supported
6 participants
@atscott @dylhunn @pkozlowski-opensource @alxhub @AndrewKushnir @jessicajaniuk