From 7cb20cd156de7d81aa6a5793b4b9b64451c5dfe8 Mon Sep 17 00:00:00 2001
From: Rainer Hahnekamp <rainer.hahnekamp@gmail.com>
Date: Thu, 2 Jan 2025 15:27:18 +0100
Subject: [PATCH 1/2] fix(signals): fix issue with `onDestroy` removing
 watchers prematurely
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Each `signalMethod` instance uses a watcher assigned to it. The watcher is
destroyed when the `signalMethod` itself gets destroyed. However, watchers were
removed prematurely due to `onDestroy` behavior in `effect`.
• `onDestroy` doesn’t run when an effect is destroyed but after each execution.
• This caused watchers to be removed after the first execution unexpectedly.

Problematic Code: `watchers.splice(watchers.indexOf(watcher), 1);`

The removal mistakenly affected other watchers, keeping instances running,
even after the signalMethod was destroyed.

Co-authored-by: @davdev82
---
 modules/signals/spec/signal-method.spec.ts |  7 ++-----
 modules/signals/src/signal-method.ts       | 11 +++++++++--
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/modules/signals/spec/signal-method.spec.ts b/modules/signals/spec/signal-method.spec.ts
index ab554ff78a..ad32dbf304 100644
--- a/modules/signals/spec/signal-method.spec.ts
+++ b/modules/signals/spec/signal-method.spec.ts
@@ -72,18 +72,15 @@ describe('signalMethod', () => {
       const adder = createAdder((value) => (a += value));
       adder(summand1);
       adder(summand2);
-
-      summand1.set(2);
-      summand2.set(3);
       TestBed.flushEffects();
-      expect(a).toBe(6);
+      expect(a).toBe(4);
 
       adder.destroy();
 
       summand1.set(2);
       summand2.set(3);
       TestBed.flushEffects();
-      expect(a).toBe(6);
+      expect(a).toBe(4);
     });
 
     it('does not cause issues if destroyed signalMethodFn contains destroyed effectRefs', () => {
diff --git a/modules/signals/src/signal-method.ts b/modules/signals/src/signal-method.ts
index f9322e34c2..5a04c7e64d 100644
--- a/modules/signals/src/signal-method.ts
+++ b/modules/signals/src/signal-method.ts
@@ -1,5 +1,6 @@
 import {
   assertInInjectionContext,
+  DestroyRef,
   effect,
   EffectRef,
   inject,
@@ -35,15 +36,21 @@ export function signalMethod<Input>(
         config?.injector ?? getCallerInjector() ?? sourceInjector;
 
       const watcher = effect(
-        (onCleanup) => {
+        () => {
           const value = input();
           untracked(() => processingFn(value));
-          onCleanup(() => watchers.splice(watchers.indexOf(watcher), 1));
         },
         { injector: instanceInjector }
       );
       watchers.push(watcher);
 
+      instanceInjector.get(DestroyRef).onDestroy(() => {
+        const ix = watchers.indexOf(watcher);
+        if (ix !== -1) {
+          watchers.splice(ix, 1);
+        }
+      });
+
       return watcher;
     } else {
       processingFn(input);

From fd188216d7feb90277e35546f0b2d87429f1077d Mon Sep 17 00:00:00 2001
From: Rainer Hahnekamp <rainer.hahnekamp@gmail.com>
Date: Mon, 6 Jan 2025 22:09:13 +0100
Subject: [PATCH 2/2] fix(signals): add test for manual `destroy` of instance

---
 modules/signals/spec/signal-method.spec.ts | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/modules/signals/spec/signal-method.spec.ts b/modules/signals/spec/signal-method.spec.ts
index ad32dbf304..aad3fef897 100644
--- a/modules/signals/spec/signal-method.spec.ts
+++ b/modules/signals/spec/signal-method.spec.ts
@@ -195,4 +195,23 @@ describe('signalMethod', () => {
       expect(a).toBe(5);
     });
   });
+
+  it('stops specific tracking when calling destroy manually on an instance', () => {
+    let a = 1;
+    const summand1 = signal(1);
+    const summand2 = signal(2);
+    const adder = createAdder((value) => (a += value));
+    adder(summand1);
+    const s2 = adder(summand2);
+
+    TestBed.flushEffects();
+    s2.destroy();
+    expect(a).toBe(4);
+
+    summand1.set(100);
+    summand2.set(3000);
+
+    TestBed.flushEffects();
+    expect(a).toBe(104);
+  });
 });