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

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

packages/core/src/render3/instructions/change_detection.ts

@@ -13,7 +13,7 @@ import {getComponentViewByInstance} from '../context_discovery';
 import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks';
 import {CONTAINER_HEADER_OFFSET, HAS_CHILD_VIEWS_TO_REFRESH, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container';
 import {ComponentTemplate, RenderFlags} from '../interfaces/definition';
-import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view';
+import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view';
 import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state';
 import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils';
 import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
@@ -250,6 +250,16 @@ export function refreshView<T>(
       tView.firstUpdatePass = false;
     }
 
+    // Schedule any effects that are waiting on the update pass of this view.
+    if (lView[EFFECTS_TO_SCHEDULE]) {
+      for (const notifyEffect of lView[EFFECTS_TO_SCHEDULE]) {
+        notifyEffect();
+      }
+
+      // Once they've been run, we can drop the array.
+      lView[EFFECTS_TO_SCHEDULE] = null;
+    }
+
     // Do not reset the dirty state when running in check no changes mode. We don't want components
     // to behave differently depending on whether check no changes is enabled or not. For example:
     // Marking an OnPush component as dirty from within the `ngAfterViewInit` hook in order to

packages/core/src/render3/interfaces/view.ts

@@ -57,6 +57,7 @@ export const QUERIES = 18;
 export const ID = 19;
 export const EMBEDDED_VIEW_INJECTOR = 20;
 export const ON_DESTROY_HOOKS = 21;
+export const EFFECTS_TO_SCHEDULE = 22;
 export const REACTIVE_TEMPLATE_CONSUMER = 23;
 export const REACTIVE_HOST_BINDING_CONSUMER = 24;
 
@@ -336,6 +337,11 @@ export interface LView<T = unknown> extends Array<any> {
    */
   readonly[EMBEDDED_VIEW_INJECTOR]: Injector|null;
 
+  /**
+   * Effect scheduling operations that need to run during this views's update pass.
+   */
+  [EFFECTS_TO_SCHEDULE]: Array<() => void>|null;
+
   /**
    * A collection of callbacks functions that are executed when a given LView is destroyed. Those
    * are user defined, LView-specific destroy callbacks that don't have any corresponding TView

packages/core/src/render3/reactivity/effect.ts

@@ -8,13 +8,16 @@
 
 import {createWatch, Watch, WatchCleanupRegisterFn} from '@angular/core/primitives/signals';
 
+import {ChangeDetectorRef} from '../../change_detection';
 import {assertInInjectionContext} from '../../di/contextual';
 import {InjectionToken} from '../../di/injection_token';
 import {Injector} from '../../di/injector';
 import {inject} from '../../di/injector_compatibility';
 import {ɵɵdefineInjectable} from '../../di/interface/defs';
 import {ErrorHandler} from '../../error_handler';
+import type {InternalViewRef} from '../view_ref';
 import {DestroyRef} from '../../linker/destroy_ref';
+import {FLAGS, LViewFlags, EFFECTS_TO_SCHEDULE} from '../interfaces/view';
 
 import {assertNotInReactiveContext} from './asserts';
 
@@ -170,7 +173,7 @@ export class ZoneAwareMicrotaskScheduler implements EffectScheduler {
  */
 class EffectHandle implements EffectRef, SchedulableEffect {
   unregisterOnDestroy: (() => void)|undefined;
-  protected watcher: Watch;
+  readonly watcher: Watch;
 
   constructor(
       private scheduler: EffectScheduler,
@@ -198,10 +201,6 @@ class EffectHandle implements EffectRef, SchedulableEffect {
     this.scheduler.scheduleEffect(this);
   }
 
-  notify(): void {
-    this.watcher.notify();
-  }
-
   destroy(): void {
     this.watcher.destroy();
     this.unregisterOnDestroy?.();
@@ -272,8 +271,25 @@ export function effect(
       (typeof Zone === 'undefined') ? null : Zone.current, destroyRef, errorHandler,
       options?.allowSignalWrites ?? false);
 
-  // Effects start dirty.
-  handle.notify();
+  // Effects need to be marked dirty manually to trigger their initial run. The timing of this
+  // marking matters, because the effects may read signals that track component inputs, which are
+  // only available after those components have had their first update pass.
+  //
+  // We inject `ChangeDetectorRef` optionally, to determine whether this effect is being created in
+  // the context of a component or not. If it is, then we check whether the component has already
+  // run its update pass, and defer the effect's initial scheduling until the update pass if it
+  // hasn't already run.
+  const cdr =
+      injector.get(ChangeDetectorRef, null, {optional: true}) as InternalViewRef<unknown>| null;
+  if (!cdr || !(cdr._lView[FLAGS] & LViewFlags.FirstLViewPass)) {
+    // This effect is either not running in a view injector, or the view has already
+    // undergone its first change detection pass, which is necessary for any required inputs to be
+    // set.
+    handle.watcher.notify();
+  } else {
+    // Delay the initialization of the effect until the view is fully initialized.
+    (cdr._lView[EFFECTS_TO_SCHEDULE] ??= []).push(handle.watcher.notify);
+  }
 
   return handle;
 }

packages/core/test/acceptance/after_render_hook_spec.ts

@@ -489,9 +489,9 @@ describe('after render hooks', () => {
               },
             ]
           });
-          TestBed.createComponent(TestCmp);
+          const fixture = TestBed.createComponent(TestCmp);
 
-          expect(() => TestBed.flushEffects())
+          expect(() => fixture.detectChanges())
               .toThrowError(/afterRender\(\) cannot be called from within a reactive context/);
         });
       });

packages/core/test/render3/reactivity_spec.ts

@@ -7,7 +7,7 @@
  */
 
 import {AsyncPipe} from '@angular/common';
-import {AfterViewInit, Component, computed, ContentChildren, createComponent, createEnvironmentInjector, destroyPlatform, effect, EnvironmentInjector, ErrorHandler, inject, Injector, Input, NgZone, OnChanges, QueryList, signal, SimpleChanges, ViewChild} from '@angular/core';
+import {AfterViewInit, Component, computed, ContentChildren, createComponent, createEnvironmentInjector, destroyPlatform, effect, EnvironmentInjector, ErrorHandler, inject, Injectable, Injector, Input, NgZone, OnChanges, QueryList, signal, SimpleChanges, ViewChild, ViewContainerRef} from '@angular/core';
 import {toObservable} from '@angular/core/rxjs-interop';
 import {TestBed} from '@angular/core/testing';
 import {bootstrapApplication} from '@angular/platform-browser';
@@ -124,11 +124,6 @@ describe('effects', () => {
     const fixture = TestBed.createComponent(Cmp);
     fixture.detectChanges();
 
-    // Effects don't run during change detection.
-    expect(didRun).toBeFalse();
-
-    TestBed.flushEffects();
-
     expect(didRun).toBeTrue();
   });
 
@@ -349,6 +344,174 @@ describe('effects', () => {
     expect(fixture.nativeElement.textContent).toBe('0');
   });
 
+  describe('effects created in components should first run after ngOnInit', () => {
+    it('when created during bootstrapping', () => {
+      let log: string[] = [];
+      @Component({
+        standalone: true,
+        selector: 'test-cmp',
+        template: '',
+      })
+      class TestCmp {
+        constructor() {
+          effect(() => log.push('effect'));
+        }
+
+        ngOnInit(): void {
+          log.push('init');
+        }
+      }
+
+      const fixture = TestBed.createComponent(TestCmp);
+      TestBed.flushEffects();
+      expect(log).toEqual([]);
+      fixture.detectChanges();
+      expect(log).toEqual(['init', 'effect']);
+    });
+
+    it('when created during change detection', () => {
+      let log: string[] = [];
+
+      @Component({
+        standalone: true,
+        selector: 'test-cmp',
+        template: '',
+      })
+      class TestCmp {
+        ngOnInitRan = false;
+        constructor() {
+          effect(() => log.push('effect'));
+        }
+
+        ngOnInit(): void {
+          log.push('init');
+        }
+      }
+
+      @Component({
+        standalone: true,
+        selector: 'driver-cmp',
+        imports: [TestCmp],
+        template: `
+          @if (cond) {
+            <test-cmp />
+          }
+        `,
+      })
+      class DriverCmp {
+        cond = false;
+      }
+
+      const fixture = TestBed.createComponent(DriverCmp);
+      fixture.detectChanges();
+      expect(log).toEqual([]);
+
+      // Toggle the @if, which should create and run the effect.
+      fixture.componentInstance.cond = true;
+      fixture.detectChanges();
+      expect(log).toEqual(['init', 'effect']);
+    });
+
+    it('when created dynamically', () => {
+      let log: string[] = [];
+      @Component({
+        standalone: true,
+        selector: 'test-cmp',
+        template: '',
+      })
+      class TestCmp {
+        ngOnInitRan = false;
+        constructor() {
+          effect(() => log.push('effect'));
+        }
+
+        ngOnInit(): void {
+          log.push('init');
+        }
+      }
+
+      @Component({
+        standalone: true,
+        selector: 'driver-cmp',
+        template: '',
+      })
+      class DriverCmp {
+        vcr = inject(ViewContainerRef);
+      }
+
+      const fixture = TestBed.createComponent(DriverCmp);
+      fixture.detectChanges();
+
+      fixture.componentInstance.vcr.createComponent(TestCmp);
+
+      // Verify that simply creating the component didn't schedule the effect.
+      TestBed.flushEffects();
+      expect(log).toEqual([]);
+
+      // Running change detection should schedule and run the effect.
+      fixture.detectChanges();
+      expect(log).toEqual(['init', 'effect']);
+    });
+
+    it('when created in a service provided in a component', () => {
+      let log: string[] = [];
+
+      @Injectable()
+      class EffectService {
+        constructor() {
+          effect(() => log.push('effect'));
+        }
+      }
+
+      @Component({
+        standalone: true,
+        selector: 'test-cmp',
+        template: '',
+        providers: [EffectService],
+      })
+      class TestCmp {
+        svc = inject(EffectService);
+
+        ngOnInit(): void {
+          log.push('init');
+        }
+      }
+
+      const fixture = TestBed.createComponent(TestCmp);
+      TestBed.flushEffects();
+      expect(log).toEqual([]);
+      fixture.detectChanges();
+      expect(log).toEqual(['init', 'effect']);
+    });
+
+    it('if multiple effects are created', () => {
+      let log: string[] = [];
+      @Component({
+        standalone: true,
+        selector: 'test-cmp',
+        template: '',
+      })
+      class TestCmp {
+        constructor() {
+          effect(() => log.push('effect a'));
+          effect(() => log.push('effect b'));
+          effect(() => log.push('effect c'));
+        }
+
+        ngOnInit(): void {
+          log.push('init');
+        }
+      }
+
+      const fixture = TestBed.createComponent(TestCmp);
+      fixture.detectChanges();
+      expect(log[0]).toBe('init');
+      expect(log).toContain('effect a');
+      expect(log).toContain('effect b');
+      expect(log).toContain('effect c');
+    });
+  });
+
   describe('should disallow creating an effect context', () => {
     it('inside template effect', () => {
       @Component({
@@ -400,9 +563,9 @@ describe('effects', () => {
           },
         ]
       });
-      TestBed.createComponent(Cmp);
+      const fixture = TestBed.createComponent(Cmp);
 
-      expect(() => TestBed.flushEffects())
+      expect(() => fixture.detectChanges())
           .toThrowError(/effect\(\) cannot be called from within a reactive context./);
     });
   });

packages/core/test/test_bed_effect_spec.ts

@@ -34,13 +34,19 @@ describe('effects in TestBed', () => {
     TestBed.createComponent(Cmp).detectChanges();
 
     expect(log).toEqual([
+      // The component gets constructed, which creates the effect. Since the effect is created in a
+      // component, it doesn't get scheduled until the component is first change detected.
       'Ctor',
-      'Effect',
+
+      // Next, the first change detection (update pass) happens.
       'DoCheck',
+
+      // Then the effect runs.
+      'Effect',
     ]);
   });
 
-  it('created in ngOnInit should not run with detectChanges()', () => {
+  it('created in ngOnInit should run with detectChanges()', () => {
     const log: string[] = [];
     @Component({
       selector: 'test-cmp',
@@ -68,12 +74,15 @@ describe('effects in TestBed', () => {
     TestBed.createComponent(Cmp).detectChanges();
 
     expect(log).toEqual([
-      // B: component bootstrapped
+      // The component gets constructed.
       'Ctor',
-      // ngDoCheck runs before ngOnInit
+
+      // Next, the first change detection (update pass) happens, which creates the effect and
+      // schedules it for execution.
       'DoCheck',
-    ]);
 
-    // effect should not have executed.
+      // Then the effect runs.
+      'Effect',
+    ]);
   });
 });

packages/core/testing/src/component_fixture.ts

@@ -135,6 +135,9 @@ export class ComponentFixture<T> {
       // Running without zone. Just do the change detection.
       this._tick(checkNoChanges);
     }
+    // Run any effects that were created/dirtied during change detection. Such effects might become
+    // dirty in response to input signals changing.
+    this.effectRunner?.flush();
   }
 
   /**