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.

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): Ensure views marked for check are refreshed during change … #54735

Closed
wants to merge 1 commit into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Mar 6, 2024

…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 #52928
fixes #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.

…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.
@atscott atscott added the target: major This PR is targeted for the next major release label Mar 6, 2024
@angular-robot angular-robot bot added the detected: breaking change PR contains a commit with a breaking change label Mar 6, 2024
@atscott atscott added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 6, 2024
@atscott
Copy link
Contributor Author

atscott commented Mar 6, 2024

caretaker note: This flag is patched to true already internally so the patch should be removed when syncing

@atscott
Copy link
Contributor Author

atscott commented Mar 6, 2024

This PR was merged into the repository by commit ad045ef.

@atscott atscott closed this in ad045ef Mar 6, 2024
@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 Apr 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker detected: breaking change PR contains a commit with a breaking change merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
2 participants