ExpressionChangedAfterItHasBeenCheckedError is not thrown if ChangeDetection.OnPush is used

[skrtheboss]

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]

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.

[atscott]

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

[mirobo]

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

[SLKnutson]

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]

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.