From f9120d79cb88a9f14c4baa6981f71a5afbd984e1 Mon Sep 17 00:00:00 2001
From: Kristiyan Kostadinov <crisbeto@abv.bg>
Date: Thu, 28 Dec 2023 11:19:32 +0200
Subject: [PATCH] fix(core): allow effect to be used inside an ErrorHandler
 (#53713)

`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
---
 .../core/src/render3/reactivity/effect.ts     | 12 +++---
 packages/core/test/render3/reactivity_spec.ts | 40 +++++++++++++++++++
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/packages/core/src/render3/reactivity/effect.ts b/packages/core/src/render3/reactivity/effect.ts
index ac0472efdeed0..6845a63350df5 100644
--- 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
index 7908d5b0e40f7..a051cc92e2461 100644
--- 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;