8000 ExpressionChangedAfterItHasBeenCheckedError is not thrown if ChangeDetection.OnPush is used · Issue #45612 · angular/angular · GitHub
[go: up one dir, main page]
Skip to content

ExpressionChangedAfterItHasBeenCheckedError is not thrown if ChangeDetection.OnPush is used #45612

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
Closed
skrtheboss opened this issue Apr 13, 2022 · 5 comments
Closed

ExpressionChangedAfterItHasBeenCheckedError is not thrown if ChangeDetection.OnPush is used #45612

skrtheboss opened this issue Apr 13, 2022 · 5 comments
Labels
area: core Issues related to the framework runtime breaking changes core: change detection P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Milestone
Backlog

Comments

@skrtheboss
Copy link
Contributor

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

core

Is this a regression?

No

Description

When changing a template value while the change detection is taking place, the user is warned with an ExpressionChangedAfterItHasBeenCheckedError.
This is not the case for components which use the ChangeDetectionStrategy.OnPush .

By doing some debugging I found the problem:

  1. The async pipe triggers a markForCheck for the component, while change detection is running.
  2. The refreshView clears the dirty state of the component after the view has been checked.
  3. The checkNoChanges is performed, and the refreshComponent function does not perform a refreshView for that component, since it is no longer dirty.

This results in no ExpressionChangedAfterItHasBeenCheckedError because the view is not checked, since its dirty status has been cleared in the change detection process

This can be really hard to debug without the warning. So I found a workaround for now, that is to add || isInCheckNoChangesMode() to the refreshView check so that it becomes if (componentView[FLAGS] & (LViewFlags.CheckAlways | LViewFlags.Dirty) || isInCheckNoChangesMode()) {. This is very ugly and imperformant, but at least it helps detect those errors.

Please provide a link to a minimal reproduction of the bug

https://stackblitz.com/edit/angular-ivy-ked7hw?file=src/app/app.component.ts

Please provide the exception or error you saw

No response

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

Angular CLI: 13.3.2
Node: 16.14.2
Package Manager: yarn 1.22.18
OS: linux x64

Angular: 13.3.2
... animations, cdk, cdk-experimental, cli, common, compiler
... compiler-cli, core, forms, material, material-moment-adapter
... platform-browser, platform-browser-dynamic, router

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1303.2
@angular-devkit/build-angular   13.3.2
@angular-devkit/core            13.3.2
@angular-devkit/schematics      13.3.2
@schematics/angular             13.3.2
ng-packagr                      13.3.0
rxjs                            7.5.5
typescript                      4.6.3
webpack                         5.72.0

Anything else?

No response
@JoostK
Copy link
Member
JoostK commented Apr 13, 2022

Indeed, this is a known issue. It would be very breaky to enable this check for OnPush components though, which is why this hasn't been fixed as of now.

@JoostK JoostK added area: core Issues related to the framework runtime core: change detection P4 A relatively minor issue that is not relevant to core functions labels Apr 13, 2022
@ngbot ngbot bot modified the milestone: Backlog Apr 13, 2022
@atscott
Copy link
Contributor
atscott commented Apr 13, 2022

For reference, the fix was attempted in #34443 and was found to be massively breaky, as @JoostK mentioned.

@atscott atscott mentioned this issue Apr 15, 2022
Closed
Closed
@mirobo
Copy link
mirobo commented Jul 5, 2022

What is the progress on this issue? I found this issue only because I couldn't believe that suddenly ALL "ExpressionChangedAfterItHasBeenCheckedError" were fixed in our app after changing all components CDS to OnPush. All those errors just magically disappeared from our application... too good to be true :D

@JoostK JoostK mentioned this issue Sep 30, 2022
Closed
@alxhub alxhub added breaking changes P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed P4 A relatively minor issue that is not relevant to core functions labels May 17, 2023
@SLKnutson
Copy link

As I understand it, this isn't just about the warning. The warning is telling the developer that the code isn't working the way they intended. In our case, we rearranged some code resulting in a fetch being triggered by an @input setter. Since the fetch was async, it still worked, and we didn't realize right away that we had lost the loading spinner.

It's interesting that calling checkNoChanges caused a lot of breakages. I wonder how many of those are really bugs. Can it at least be main opt-in?

Thanks!

@JoostK
Copy link
Member
JoostK commented May 31, 2023

I wonder how many of those are really bugs

All of them are bugs! The sad thing is that projects have actually used OnPush change detection to suppress ECAIHBC errors.

If this will be fixed this will indeed have to be opt-in, to avoid a large scale breaking change. We are reevaluating change detection details in support of the signal based components RFC, but there are not yet concrete plans on how OnPush behavior will be affected.

atscott added a commit to atscott/angular that referenced this issue Feb 8, 2024
@atscott
perf(core): Avoid traversal when markForCheck is called on a view b…
c83f7ca 
…eing refreshed

This commit updates the internal `markViewDirty` logic to avoid marking
views dirty up to the root when we encounter a view that is already
being refreshed. This is wasteful because we clear the dirty bit at the
end of refreshing a view anyways so it really has no affect on that
view.

This change should reduce the number of tree traversals for replay
observables and async pipe. For these cases, when we execute change
detection on the view for the first time, the async pipe subscribes to
the observable and its value emits synchronously. `markForCheck` is then
called inside `AsyncPipe`, which traverses all the way up to the root
view. This happens for _every_ `AsyncPipe` binding that emits
synchronously on subscription.

Sometimes we can reach a view and refresh it when the parents have been
skipped. For example, this happens with views that have updated signals.
Ancestors are only traversed and not refreshed. Marking dirty all the
way up to the root in this case can result in those ancestor views being
marked with the dirty flag and it not being cleared.

This only became problematic when we started to support backwards
referenced transplanted views and changed signal change detection to
not mark ancestors for check. In addition to that, the loop in change
detection support to remove `ExpressionChanged...` errors for these cases
can potentially cause infinite loops in rare cases when these views are
refreshed at a time when parents have only been traversed and
`markForCheck` is called. Prior to the loop, if this `markForCheck` was
combined with an actual change to binding values, this would cause
`ExpressionChangedAfterItWasCheckedError`.

For regular cases where we are refreshing a view, update a binding, and call
`markForCheck`, this is either `ExpressionChanged...` in views with
default change detection or no error at all (but without any state
update) in views that are `OnPush` since the `Dirty` flag is cleared at
the end of `refreshView` and `checkNoChanges` uses the same `Dirty` bit for
traversal (angular#45612).

The only case where this might cause different and unexpected behavior
is if change detection starts in the middle of a view tree via
`ChangeDetectorRef.detectChanges` _and_ a state update happens where the
state is read in a view above the root of the traversal along with `markForCheck`.
This might be the case for updating host bindings during change
detection. That said, we have never really intended to support updating
state during change detection, especially in a location above the
current spot in the current view tree. This violates unidirectional data
flow, a principle we have always intended to enforce outside of using
signals for state.
@atscott atscott mentioned this issue Feb 8, 2024
Closed
@JoostK JoostK mentioned this issue Apr 9, 2024
Closed
@atscott atscott added P4 A relatively minor issue that is not relevant to core functions P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful P4 A relatively minor issue that is n 8000 ot relevant to core functions labels May 3, 2024
atscott added a commit to atscott/angular that referenced this issue May 7, 2024
@atscott
refactor(core): feature for potential zoneless-compatibility debug check
c06bb1c 
This commit adds a feature that is useful for determining if an
application is zoneless-ready. The way this works is generally only
useful right now when zoneless is enabled. Some version of this may be useful in
the future as a general configuration option to change detection to make
`checkNoChanges` pass always exhaustive as an opt-in to address angular#45612.

Because this is an experimental, debug-only feature, it is okay to merge
during the RC period.
AndrewKushnir pushed a commit that referenced this issue May 7, 2024
@atscott @AndrewKushnir
refactor(core): feature for potential zoneless-compatibility debug ch…
ca6cdcd 
…eck (#55663)

This commit adds a feature that is useful for determining if an
application is zoneless-ready. The way this works is generally only
useful right now when zoneless is enabled. Some version of this may be useful in
the future as a general configuration option to change detection to make
`checkNoChanges` pass always exhaustive as an opt-in to address #45612.

Because this is an experimental, debug-only feature, it is okay to merge
during the RC period.

PR Close #55663
AndrewKushnir pushed a commit that referenced this issue May 7, 2024
@atscott @AndrewKushnir
refactor(core): feature for potential zoneless-compatibility debug ch…
6b97aec 
…eck (#55663)

This commit adds a feature that is useful for determining if an
application is zoneless-ready. The way this works is generally only
useful right now when zoneless is enabled. Some version of this may be useful in
the future as a general configuration option to change detection to make
`checkNoChanges` pass always exhaustive as an opt-in to address #45612.

Because this is an experimental, debug-only feature, it is okay to merge
during the RC period.

PR Close #55663
@atscott atscott mentioned this issue Apr 17, 2025
Closed
atscott added a commit to atscott/angular that referenced this issue Apr 23, 2025
@atscott
feat(core): move provideExperimentalCheckNoChangesForDebug to `prov…
452a945 
…ideCheckNoChangesConfig`

This commit makes several changes changes to the
`provideExperimentalCheckNoChangesForDebug` API:

* Rename it
* Promote to dev preview
* Apply the exhaustive behavior to _all_ checkNoChanges runs
* Remove `useNgZoneOnStable` option. This wasn't found to be generally
  more useful than `interval`

fixes angular#45612

BREAKING CHANGE: `provideExperimentalCheckNoChangesForDebug` has several
breaking changes:
* It is renamed to `provideCheckNoChangesConfig`
* The behavior applies to _all_ checkNoChanges runs
* The `useNgZoneOnStable` option is removed. This wasn't found to be generally
  more useful than `interval`
atscott added a commit to atscott/components that referenced this issue May 9, 2025
@atscott
refactor: Update tests to use exhaustive checkNoChanges by default
514f244 
When components use `OnPush`, they prevent `ExpressionHasChanged...`
from being thrown when it should (angular/angular#45612).
The exhaustive option ignores dirty
flags and traverses the entire view tree checking for changes. This
commit configures component tests to do this by default and opts out any
tests with existing issues.
atscott added a commit to atscott/components that referenced this issue May 9, 2025
@atscott
test(multiple): Update tests to use exhaustive checkNoChanges by default
82c4484 
When components use `OnPush`, they prevent `ExpressionHasChanged...`
from being thrown when it should (angular/angular#45612).
The exhaustive option ignores dirty
flags and traverses the entire view tree checking for changes. This
commit configures component tests to do this by default and opts out any
tests with existing issues.
@atscott atscott mentioned this issue May 9, 2025
Merged
mmalerba pushed a commit to angular/components that referenced this issue May 13, 2025
@atscott
test(multiple): Update tests to use exhaustive checkNoChanges by defa…
839dfba 
…ult (#31069)

When components use `OnPush`, they prevent `ExpressionHasChanged...`
from being thrown when it should (angular/angular#45612).
The exhaustive option ignores dirty
flags and traverses the entire view tree checking for changes. This
commit configures component tests to do this by default and opts out any
tests with existing issues.
mmalerba pushed a commit to angular/components that referenced this issue May 13, 2025
@atscott @mmalerba
test(multiple): Update tests to use exhaustive checkNoChanges by defa…
a0a0dfd 
…ult (#31069)

When components use `OnPush`, they prevent `ExpressionHasChanged...`
from being thrown when it should (angular/angular#45612).
The exhaustive option ignores dirty
flags and traverses the entire view tree checking for changes. This
commit configures component tests to do this by default and opts out any
tests with existing issues.

(cherry picked from commit 839dfba)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area: core Issues related to the framework runtime breaking changes core: change detection P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Milestone
Backlog
6 participants
@JoostK @atscott @alxhub @skrtheboss @SLKnutson @mirobo
0