Template of an OnPush component is no longer updated after a signal change in some cases

[divdavem]

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

core

Is this a regression?

No

Description

Go to this stackblitz
Click on the Toggle state button.
item is visible appears
Click again on the Toggle state button.
item is visible disappears
Click again on the Toggle state button.
Nothing happens! I would expect the item is visible element to appear again.

Here is the code of the stackblitz:

import 'zone.js/dist/zone';
import {
  ChangeDetectionStrategy,
  Component,
  Directive,
  effect,
  signal,
  Input,
  OnChanges,
  OnDestroy,
  SimpleChanges,
  computed,
} from '@angular/core';
import { CommonModule } from '@angular/common';
import { bootstrapApplication } from '@angular/platform-browser';

interface State {
  isInDom: boolean;
  shouldBeInDom: boolean;
}

const createModel = () => {
  const state = signal<State>({
    isInDom: false,
    shouldBeInDom: false,
  });
  const updateState = (changes: Partial<State>) =>
    state.update((value) => ({
      ...value,
      ...changes,
    }));
  const toggle = () => updateState({ shouldBeInDom: !state().shouldBeInDom });
  const setDomState = (isInDom: boolean) => updateState({ isInDom });
  return { state, toggle, setDomState };
};

@Directive({
  standalone: true,
  selector: '[myDirective]',
})
export class MyDirective implements OnChanges, OnDestroy {
  @Input({ required: true })
  myDirective!: ReturnType<typeof createModel>;

  async ngOnChanges(changes: SimpleChanges) {
    // Comment the following line to have the issue on display:
    await 0;
    this.myDirective.setDomState(true);
  }

  async ngOnDestroy() {
    // Uncomment the following line as a work-around to fix the issue:
    // await 0;
    this.myDirective.setDomState(false);
  }
}

@Component({
  selector: 'my-component',
  standalone: true,
  // Comment the following line as a work-around to fix the issue:
  changeDetection: ChangeDetectionStrategy.OnPush,
  imports: [CommonModule, MyDirective],
  template: `
    <div *ngIf="state().shouldBeInDom" [myDirective]="model">item is visible</div>
  `,
})
export class MyComponent {
  model = createModel();
  // Uncomment the following line as a work-around to fix the issue:
  // state = this.model.state;
  state = computed(() => this.model.state());

  constructor() {
    // Uncomment the following line as a work-around to fix the issue:
    // effect(() => console.log(this.state()));
  }
}

@Component({
  selector: 'my-app',
  standalone: true,
  imports: [CommonModule, MyComponent],
  template: `
    <button (click)="c.model.toggle()">Toggle state</button>
    <my-component #c></my-component>
  `,
})
export class App {}

bootstrapApplication(App);

Please provide a link to a minimal reproduction of the bug

https://stackblitz.com/edit/angular-nltu6v?file=src/main.ts

Please provide the exception or error you saw

There is no exception, no explanation.

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

Angular CLI: 16.0.1
Node: 18.16.0
Package Manager: npm 9.5.1
OS: linux x64

Angular: 16.0.1
... animations, cli, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1600.1
@angular-devkit/build-angular   16.0.1
@angular-devkit/core            16.0.1
@angular-devkit/schematics      16.0.1
@schematics/angular             16.0.1
ng-packagr                      16.0.1
rxjs                            7.8.1
typescript                      5.0.4

Anything else?

What triggers the bug seems to be the fact that the signal is synchronously changed either from ngOnChanges or from ngOnDestroy in the directive. If this is done asynchronously, the problem does not happen.

This makes me think about the famous ExpressionChangedAfterItHasBeenCheckedError issue (even if there is no exception displayed in this case), the idea is the same: there is an issue when updating data at some specific times.

As mentioned in the comments of the code, if any of the following modifications is done (alone), the bug does not happen:

• if state is a direct reference to this.model.state in MyComponent instead of being a computed
• if an effect that reads this.state() is added in the constructor of MyComponent
• if MyComponent is not OnPush

[pkozlowski-opensource]

This is an interesting case, thnx for sharing!

What is (roughly, working theory) going on here is that a computed property is marked as "stale" (by the update in ngOnChanges) after the reactive context of the OnPush component accessed this value (and marked it as "clean"). This means that subsequent signal change notification do not reach the computed.

Ultimately the update in ngOnChanges is the "culprit" here but we do need better guidance / guide-rails here.

Trying to narrow down the reproduction to a simpler case.

[pkozlowski-opensource]

Here is a simpler repro: https://stackblitz.com/edit/angular-hu21mk?file=src%2Fmain.ts

This is ExpressionChangedAfterItHasBeenCheckedError (as seen by commenting out OnPush that "masks" this error).

[zip-fa]

I have the same bug here with ExpressionChanged error when changing signal value in ngOnDestroy. Will give stackblitz soon, maybe it will help

[divdavem]

@pkozlowski-opensource Thank you for your answers. I don't see ExpressionChangedAfterItHasBeenCheckedError in your stackblitz.

I find updating signals in ngOnChanges or ngOnDestroy quite useful. I am a bit afraid when you say:

Ultimately the update in ngOnChanges is the "culprit" here but we do need better guidance / guide-rails here.

Do you mean that you are planning to only add a clear error and better documentation to "fix" this bug?

Ideally, it should be possible to update signals at any time (without having to add an asynchronous step), with no error, with the new values correctly taken into account inside templates, and without any resurgence of the old ExpressionChangedAfterItHasBeenCheckedError error (or new variants of it).

[swami-sanapathi]

Yes, I encountered a similar issue and I described it in an RFC as well.

#49684 (comment)

[divdavem]

Do you have any news about this bug?
Note that, as shown this stackblitz, since version 16.2.4 of Angular, adding an effect that reads this.state() in the constructor of MyComponent is no longer a workaround for this issue.

[pkozlowski-opensource]

I'm still trying to wrap my head about the details of what is going on here and my working theory is the same as previously: modifying signals (during CD) that is also marking a view as dirty puts in trouble. Essentially we are marking a signals as dirty and it is trying to mark a view as dirty, but the very same view is being change detected.

Here is a simplified reproduce scenario, still working on making it smaller: https://stackblitz.com/edit/stackblitz-starters-t5x8cv?file=src%2Fmain.ts

[devversion]

Here is a more up to date reproduction using Angular v17 next and some ReactiveNode debugging starting points: https://stackblitz.com/edit/vdjh5r?file=package.json,angular.json,src%2Findex.html,src%2Fmain.ts

[pkozlowski-opensource]

Another simplification of the reproduction scenario, this time with the latest Angular version (17.x next) https://stackblitz.com/edit/vdjh5r-pe334b?file=package.json,angular.json,src%2Findex.html,src%2Fmain.ts

[pkozlowski-opensource]

@divdavem please have a look at the latest, trimmed down reproduction (which I think corresponds to the initial use-case).

What we need to observe there is that the situation involves writing to a signal that is read by a template being change detected. Currently the Angular doesn't have any mechanism for "going back" to the already change detected views and it is logically equivalent to the ExpressionChangedAfterItHasBeenCheckedError.

We are going to make this situation an explicit error in v17. Having said this we more than understand that is is hard for a developer to know what is going on with the change detection - it requires deep awareness of the fwk internals and this is this not great. This is why we are working on the updated change detection algorithm that will handle the situation described here without any errors and proper UI refresh.

[angular-automatic-lock-bot]

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