Skip to content

fix(core): effects wait for ngOnInit for their first run #52473

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

Closed
wants to merge 1 commit into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Oct 31, 2023

When an effect is created in a component constructor, it might read signals which are derived from component inputs. These signals may be unreliable or (in the case of the proposed input signals) may throw if accessed before the component is first change detected (which is what makes required inputs available).

Depending on the scenario involved, the effect may or may not run before this initialization takes place, which isn't a great developer experience. In particular, effects created during CD (e.g. via control flow) work fine, as do effects created in bootstrap thanks to the sync CD it performs. When an effect is created through dynamic component creation outside of CD though (such as on router navigations), it runs before the component is first CD'd, causing the issue.

In fact, in the signal components RFC we described how effects would wait until ngOnInit for their first execution for exactly this reason, but this behavior was never implemented as it was thought our effect scheduling design made it unnecessary. This is true of the regular execution of effects but the above scenario shows that creation of the effect is still vulnerable. Thus, this logic is needed.

This commit makes effects sensitive to their creation context, by injecting ChangeDetectorRef optionally. An effect created with an injector that's tied to a component will wait until that component is initialized before initially being scheduled. TestBed effect flushing is also adjusted to account for the additional interaction with change detection.

@alxhub alxhub requested a review from atscott October 31, 2023 19:44
@alxhub alxhub force-pushed the effects/ngoninit branch 3 times, most recently from 95442f4 to 1f1ad34 Compare October 31, 2023 21:09
@alxhub alxhub force-pushed the effects/ngoninit branch 3 times, most recently from d0e9663 to befa229 Compare October 31, 2023 22:46
When an effect is created in a component constructor, it might read signals
which are derived from component inputs. These signals may be unreliable or
(in the case of the proposed input signals) may throw if accessed before the
component is first change detected (which is what makes required inputs
available).

Depending on the scenario involved, the effect may or may not run before
this initialization takes place, which isn't a great developer experience.
In particular, effects created during CD (e.g. via control flow) work fine,
as do effects created in bootstrap thanks to the sync CD it performs. When
an effect is created through dynamic component creation outside of CD though
(such as on router navigations), it runs before the component is first CD'd,
causing the issue.

In fact, in the signal components RFC we described how effects would wait
until ngOnInit for their first execution for exactly this reason, but this
behavior was never implemented as it was thought our effect scheduling
design made it unnecessary. This is true of the regular execution of effects
but the above scenario shows that *creation* of the effect is still
vulnerable. Thus, this logic is needed.

This commit makes effects sensitive to their creation context, by injecting
`ChangeDetectorRef` optionally. An effect created with an injector that's
tied to a component will wait until that component is initialized before
initially being scheduled. TestBed effect flushing is also adjusted to
account for the additional interaction with change detection.
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker target: rc This PR is targeted for the next release-candidate labels Oct 31, 2023
@alxhub
Copy link
Member Author

alxhub commented Nov 1, 2023

This PR was merged into the repository by commit ee9605f.

@alxhub alxhub closed this in ee9605f Nov 1, 2023
alxhub added a commit that referenced this pull request Nov 1, 2023
When an effect is created in a component constructor, it might read signals
which are derived from component inputs. These signals may be unreliable or
(in the case of the proposed input signals) may throw if accessed before the
component is first change detected (which is what makes required inputs
available).

Depending on the scenario involved, the effect may or may not run before
this initialization takes place, which isn't a great developer experience.
In particular, effects created during CD (e.g. via control flow) work fine,
as do effects created in bootstrap thanks to the sync CD it performs. When
an effect is created through dynamic component creation outside of CD though
(such as on router navigations), it runs before the component is first CD'd,
causing the issue.

In fact, in the signal components RFC we described how effects would wait
until ngOnInit for their first execution for exactly this reason, but this
behavior was never implemented as it was thought our effect scheduling
design made it unnecessary. This is true of the regular execution of effects
but the above scenario shows that *creation* of the effect is still
vulnerable. Thus, this logic is needed.

This commit makes effects sensitive to their creation context, by injecting
`ChangeDetectorRef` optionally. An effect created with an injector that's
tied to a component will wait until that component is initialized before
initially being scheduled. TestBed effect flushing is also adjusted to
account for the additional interaction with change detection.

PR Close #52473
@simeyla
Copy link

simeyla commented Nov 14, 2023

@alxhub Is there a tracking issue for this and other related developer preview features?

The specific concern I have is related to toObservable, given that it is now subject to this new timing (it uses effect).

Here's an oversimplified example where this may cause unexpected behavior.

 // settings from config service
const config = signal({ color: 'red' };  

ngOnInit() 
{
   // Angular 16 - subscribe runs immediately   
   // Angular 17 - subscribe runs after ngOnInit
   toObservable(this.config).subscribe((config) =>
   {
       this.service.initialize(config.color);
   });

   if (this.service.color() == 'red')
   {
      // this will never be reached in Angular 17
   }
}

Maybe toObservable should simply set the initial value immediately and not rely on the effect?

@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 Dec 27, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
When an effect is created in a component constructor, it might read signals
which are derived from component inputs. These signals may be unreliable or
(in the case of the proposed input signals) may throw if accessed before the
component is first change detected (which is what makes required inputs
available).

Depending on the scenario involved, the effect may or may not run before
this initialization takes place, which isn't a great developer experience.
In particular, effects created during CD (e.g. via control flow) work fine,
as do effects created in bootstrap thanks to the sync CD it performs. When
an effect is created through dynamic component creation outside of CD though
(such as on router navigations), it runs before the component is first CD'd,
causing the issue.

In fact, in the signal components RFC we described how effects would wait
until ngOnInit for their first execution for exactly this reason, but this
behavior was never implemented as it was thought our effect scheduling
design made it unnecessary. This is true of the regular execution of effects
but the above scenario shows that *creation* of the effect is still
vulnerable. Thus, this logic is needed.

This commit makes effects sensitive to their creation context, by injecting
`ChangeDetectorRef` optionally. An effect created with an injector that's
tied to a component will wait until that component is initialized before
initially being scheduled. TestBed effect flushing is also adjusted to
account for the additional interaction with change detection.

PR Close angular#52473
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 target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants