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

input signal effects are not run when attaching a directive to an IonButton #55644

Open
Eraldo opened this issue May 2, 2024 · 6 comments · May be fixed by #56501
Open

input signal effects are not run when attaching a directive to an IonButton #55644

Eraldo opened this issue May 2, 2024 · 6 comments · May be fixed by #56501
Labels
area: core Issues related to the framework runtime core: change detection core: reactivity Work related to fine-grained reactivity in the core framework cross-cutting: signals
Milestone

Comments

@Eraldo
Copy link

Eraldo commented May 2, 2024

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

core, elements

Description

Expected:
Running an effect on an input signal within a directive that is attached to an IonButton should trigger/run the effect code.

Actual:
The effect is not run.

It works just fine if attached to other html tags.

  1. Open the reproduction url.
  2. Follow on screen instructions.

Insight:
This is where the current behavior is probably sourced => https://github.com/ionic-team/ionic-framework/blob/ba5cebf2542c8fbd6c29af593a31a742e0caba1e/packages/angular/src/directives/proxies.ts#L357

Please provide a link to a minimal reproduction of the bug

https://stackblitz.com/edit/stackblitz-starters-gkckxf?file=src%2Fmain.ts

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

Angular version: 17.3.4

Anything else?

Issue has been confirmed by "JB Nizet" and "Matthieu Riegler" and I was encouraged to create a bug report here.

@JeanMeche JeanMeche added core: reactivity Work related to fine-grained reactivity in the core framework cross-cutting: signals labels May 2, 2024
@Eraldo
Copy link
Author

Eraldo commented May 2, 2024

Referenced in Ionic: ionic-team/ionic-framework#29444

@Eraldo Eraldo closed this as completed May 2, 2024
@JeanMeche JeanMeche reopened this May 2, 2024
@Flo0806
Copy link

Flo0806 commented May 6, 2024

Hello. We found the same issue in a new project inside our company. It looks like the problem is really the changeDetectorRef.detach. This simple component isn't shown in effect too, if you add the same directive:

import { ChangeDetectionStrategy, ChangeDetectorRef, Component } from '@angular/core';

@Component({
  selector: 'app-test-button',
  standalone: true,
  imports: [],
  templateUrl: './test-button.component.html',
  styleUrl: './test-button.component.css',
  changeDetection: ChangeDetectionStrategy.OnPush
})
export class TestButtonComponent {
constructor(changeRef: ChangeDetectorRef) {
  changeRef.detach();
}
}

Component and directive:

@Directive({
  selector: '[test]',
  standalone: true,
})
export class TestDirective {
  test = input.required<string>();
  test2 = computed(() => {
    return this.test() + '2';
  });

  constructor() {
    effect(() => {
      console.log('>> EFFECT', this.test2());
    });
  }
}

.........

<app-test-button [test]="inputText">
    Button with Directive
</app-test-button>

@JeanMeche
Copy link
Member

The root cause is the injection of the ChangeDetectorRef in directives. When the directive is applied to a component, the CDR injected in the directive is the components CDR.

The effect is scheduled to run first when the component is init and uses the CDR to do that. In this case, the component is never init (because of running detach in the constructor) so the effect is never started.

A workaround for this would be to let the component finish its initialization and detach only in the ngAfterViewInit hook. This way the effect will

@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime core: change detection labels May 15, 2024
@ngbot ngbot bot added this to the needsTriage milestone May 15, 2024
@pkozlowski-opensource
Copy link
Member

Here is a minimal repro: https://stackblitz.com/edit/stackblitz-starters-7zd1fv?file=src%2Fmain.ts

As @JeanMeche pointed out, the root cause here is a component detaching from the change detection tree. Having said this I do recognize that this is very non-obvious - this is one of the cases we want to look into before stabilizing effects API.

@pkozlowski-opensource
Copy link
Member

I've created #55808 to capture the root cause without any external dependencies. Let's continue the discussion in there.

Duplicate of #55808

@pkozlowski-opensource pkozlowski-opensource closed this as not planned Won't fix, can't repro, duplicate, stale May 15, 2024
@alxhub
Copy link
Member

alxhub commented May 17, 2024

Reopening as this is a separate issue than #55808.

@alxhub alxhub reopened this May 17, 2024
alxhub added a commit to alxhub/angular that referenced this issue Jun 18, 2024
The original effect design for Angular had one "bucket" of effects, which
are scheduled on the microtask queue. This approach got us pretty far, but
as developers have built more complex reactive systems, we've hit the
limitations of this design.

This commit changes the nature of effects significantly. In particular,
effects created in components have a completely new scheduling system, which
executes them as a part of the change detection cycle. This results in
behavior similar to that of nested effects in other reactive frameworks. The
scheduling behavior here uses the "mark for traversal" flag
(`HasChildViewsToRefresh`). This has really nice behavior:

 * if the component is dirty already, effects run following preorder hooks
   (ngOnInit, etc).
 * if the component isn't dirty, it doesn't get change detected only because
   of the dirty effect.

This is not a breaking change, since `effect()` is in developer preview (and
it remains so).

Fixes angular#55311
Fixes angular#55808
Fixes angular#55644
@alxhub alxhub linked a pull request Jun 18, 2024 that will close this issue
alxhub added a commit to alxhub/angular that referenced this issue Jul 9, 2024
The original effect design for Angular had one "bucket" of effects, which
are scheduled on the microtask queue. This approach got us pretty far, but
as developers have built more complex reactive systems, we've hit the
limitations of this design.

This commit changes the nature of effects significantly. In particular,
effects created in components have a completely new scheduling system, which
executes them as a part of the change detection cycle. This results in
behavior similar to that of nested effects in other reactive frameworks. The
scheduling behavior here uses the "mark for traversal" flag
(`HasChildViewsToRefresh`). This has really nice behavior:

 * if the component is dirty already, effects run following preorder hooks
   (ngOnInit, etc).
 * if the component isn't dirty, it doesn't get change detected only because
   of the dirty effect.

This is not a breaking change, since `effect()` is in developer preview (and
it remains so).

As a part of this redesigned `effect()` behavior, the `allowSignalWrites`
flag was removed. Effects no longer prohibit writing to signals at all. This
decision was taken in response to feedback / observations of usage patterns,
which showed the benefit of the restriction did not justify the DX cost.

Fixes angular#55311
Fixes angular#55808
Fixes angular#55644
Fixes angular#56863
alxhub added a commit to alxhub/angular that referenced this issue Jul 9, 2024
The original effect design for Angular had one "bucket" of effects, which
are scheduled on the microtask queue. This approach got us pretty far, but
as developers have built more complex reactive systems, we've hit the
limitations of this design.

This commit changes the nature of effects significantly. In particular,
effects created in components have a completely new scheduling system, which
executes them as a part of the change detection cycle. This results in
behavior similar to that of nested effects in other reactive frameworks. The
scheduling behavior here uses the "mark for traversal" flag
(`HasChildViewsToRefresh`). This has really nice behavior:

 * if the component is dirty already, effects run following preorder hooks
   (ngOnInit, etc).
 * if the component isn't dirty, it doesn't get change detected only because
   of the dirty effect.

This is not a breaking change, since `effect()` is in developer preview (and
it remains so).

As a part of this redesigned `effect()` behavior, the `allowSignalWrites`
flag was removed. Effects no longer prohibit writing to signals at all. This
decision was taken in response to feedback / observations of usage patterns,
which showed the benefit of the restriction did not justify the DX cost.

Fixes angular#55311
Fixes angular#55808
Fixes angular#55644
Fixes angular#56863
alxhub added a commit to alxhub/angular that referenced this issue Jul 12, 2024
The original effect design for Angular had one "bucket" of effects, which
are scheduled on the microtask queue. This approach got us pretty far, but
as developers have built more complex reactive systems, we've hit the
limitations of this design.

This commit changes the nature of effects significantly. In particular,
effects created in components have a completely new scheduling system, which
executes them as a part of the change detection cycle. This results in
behavior similar to that of nested effects in other reactive frameworks. The
scheduling behavior here uses the "mark for traversal" flag
(`HasChildViewsToRefresh`). This has really nice behavior:

 * if the component is dirty already, effects run following preorder hooks
   (ngOnInit, etc).
 * if the component isn't dirty, it doesn't get change detected only because
   of the dirty effect.

This is not a breaking change, since `effect()` is in developer preview (and
it remains so).

As a part of this redesigned `effect()` behavior, the `allowSignalWrites`
flag was removed. Effects no longer prohibit writing to signals at all. This
decision was taken in response to feedback / observations of usage patterns,
which showed the benefit of the restriction did not justify the DX cost.

Fixes angular#55311
Fixes angular#55808
Fixes angular#55644
Fixes angular#56863
alxhub added a commit to alxhub/angular that referenced this issue Jul 17, 2024
The original effect design for Angular had one "bucket" of effects, which
are scheduled on the microtask queue. This approach got us pretty far, but
as developers have built more complex reactive systems, we've hit the
limitations of this design.

This commit changes the nature of effects significantly. In particular,
effects created in components have a completely new scheduling system, which
executes them as a part of the change detection cycle. This results in
behavior similar to that of nested effects in other reactive frameworks. The
scheduling behavior here uses the "mark for traversal" flag
(`HasChildViewsToRefresh`). This has really nice behavior:

 * if the component is dirty already, effects run following preorder hooks
   (ngOnInit, etc).
 * if the component isn't dirty, it doesn't get change detected only because
   of the dirty effect.

This is not a breaking change, since `effect()` is in developer preview (and
it remains so).

As a part of this redesigned `effect()` behavior, the `allowSignalWrites`
flag was removed. Effects no longer prohibit writing to signals at all. This
decision was taken in response to feedback / observations of usage patterns,
which showed the benefit of the restriction did not justify the DX cost.

Fixes angular#55311
Fixes angular#55808
Fixes angular#55644
Fixes angular#56863
alxhub added a commit to alxhub/angular that referenced this issue Jul 17, 2024
The original effect design for Angular had one "bucket" of effects, which
are scheduled on the microtask queue. This approach got us pretty far, but
as developers have built more complex reactive systems, we've hit the
limitations of this design.

This commit changes the nature of effects significantly. In particular,
effects created in components have a completely new scheduling system, which
executes them as a part of the change detection cycle. This results in
behavior similar to that of nested effects in other reactive frameworks. The
scheduling behavior here uses the "mark for traversal" flag
(`HasChildViewsToRefresh`). This has really nice behavior:

 * if the component is dirty already, effects run following preorder hooks
   (ngOnInit, etc).
 * if the component isn't dirty, it doesn't get change detected only because
   of the dirty effect.

This is not a breaking change, since `effect()` is in developer preview (and
it remains so).

As a part of this redesigned `effect()` behavior, the `allowSignalWrites`
flag was removed. Effects no longer prohibit writing to signals at all. This
decision was taken in response to feedback / observations of usage patterns,
which showed the benefit of the restriction did not justify the DX cost.

Fixes angular#55311
Fixes angular#55808
Fixes angular#55644
Fixes angular#56863
alxhub added a commit to alxhub/angular that referenced this issue Jul 17, 2024
The original effect design for Angular had one "bucket" of effects, which
are scheduled on the microtask queue. This approach got us pretty far, but
as developers have built more complex reactive systems, we've hit the
limitations of this design.

This commit changes the nature of effects significantly. In particular,
effects created in components have a completely new scheduling system, which
executes them as a part of the change detection cycle. This results in
behavior similar to that of nested effects in other reactive frameworks. The
scheduling behavior here uses the "mark for traversal" flag
(`HasChildViewsToRefresh`). This has really nice behavior:

 * if the component is dirty already, effects run following preorder hooks
   (ngOnInit, etc).
 * if the component isn't dirty, it doesn't get change detected only because
   of the dirty effect.

This is not a breaking change, since `effect()` is in developer preview (and
it remains so).

As a part of this redesigned `effect()` behavior, the `allowSignalWrites`
flag was removed. Effects no longer prohibit writing to signals at all. This
decision was taken in response to feedback / observations of usage patterns,
which showed the benefit of the restriction did not justify the DX cost.

Fixes angular#55311
Fixes angular#55808
Fixes angular#55644
Fixes angular#56863
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues related to the framework runtime core: change detection core: reactivity Work related to fine-grained reactivity in the core framework cross-cutting: signals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants