fix(core): allow effect to be used inside an ErrorHandler (#53713)

crisbeto · atscott · commit f9120d79cb88 · 2024-01-03T10:17:30.000-08:00
`effect` was expecting an `ErrorHandler` in its constructor which can lead to a circular DI error if an effect is used inside a custom `ErrorHandler`. These changes inject the `ErrorHandler` only when reporting errors. Fixes #52680. PR Close #53713
diff --git a/packages/core/src/render3/reactivity/effect.ts b/packages/core/src/render3/reactivity/effect.ts
@@ -182,8 +182,8 @@ class EffectHandle implements EffectRef, SchedulableEffect {
   constructor(
       private scheduler: EffectScheduler,
       private effectFn: (onCleanup: EffectCleanupRegisterFn) => void,
-      public creationZone: Zone|null, destroyRef: DestroyRef|null,
-      private errorHandler: ErrorHandler|null, allowSignalWrites: boolean) {
+      public creationZone: Zone|null, destroyRef: DestroyRef|null, private injector: Injector,
+      allowSignalWrites: boolean) {
     this.watcher = createWatch(
         (onCleanup) => this.runEffect(onCleanup), () => this.schedule(), allowSignalWrites);
     this.unregisterOnDestroy = destroyRef?.onDestroy(() => this.destroy());
@@ -193,7 +193,10 @@ class EffectHandle implements EffectRef, SchedulableEffect {
     try {
       this.effectFn(onCleanup);
     } catch (err) {
-      this.errorHandler?.handleError(err);
+      // Inject the `ErrorHandler` here in order to avoid circular DI error
+      // if the effect is used inside of a custom `ErrorHandler`.
+      const errorHandler = this.injector.get(ErrorHandler, null, {optional: true});
+      errorHandler?.handleError(err);
     }
   }
 
@@ -273,12 +276,11 @@ export function effect(
 
   !options?.injector && assertInInjectionContext(effect);
   const injector = options?.injector ?? inject(Injector);
-  const errorHandler = injector.get(ErrorHandler, null, {optional: true});
   const destroyRef = options?.manualCleanup !== true ? injector.get(DestroyRef) : null;
 
   const handle = new EffectHandle(
       injector.get(APP_EFFECT_SCHEDULER), effectFn,
-      (typeof Zone === 'undefined') ? null : Zone.current, destroyRef, errorHandler,
+      (typeof Zone === 'undefined') ? null : Zone.current, destroyRef, injector,
       options?.allowSignalWrites ?? false);
 
   // Effects need to be marked dirty manually to trigger their initial run. The timing of this
diff --git a/packages/core/test/render3/reactivity_spec.ts b/packages/core/test/render3/reactivity_spec.ts
@@ -66,6 +66,46 @@ describe('effects', () => {
     expect(lastError.message).toBe('fail!');
   });
 
+  it('should be usable inside an ErrorHandler', async () => {
+    const shouldError = signal(false);
+    let lastError: any = null;
+
+    class FakeErrorHandler extends ErrorHandler {
+      constructor() {
+        super();
+        effect(() => {
+          if (shouldError()) {
+            throw new Error('fail!');
+          }
+        });
+      }
+
+      override handleError(error: any): void {
+        lastError = error;
+      }
+    }
+
+    @Component({
+      standalone: true,
+      template: '',
+      providers: [{provide: ErrorHandler, useClass: FakeErrorHandler}]
+    })
+    class App {
+      errorHandler = inject(ErrorHandler);
+    }
+
+    const fixture = TestBed.createComponent(App);
+    fixture.detectChanges();
+
+    expect(fixture.componentInstance.errorHandler).toBeInstanceOf(FakeErrorHandler);
+    expect(lastError).toBe(null);
+
+    shouldError.set(true);
+    fixture.detectChanges();
+
+    expect(lastError?.message).toBe('fail!');
+  });
+
   it('should run effect cleanup function on destroy', async () => {
     let counterLog: number[] = [];
     let cleanupCount = 0;
