feat(signals): warn when reactive method runs with source injector (#4742)

Closes #4726

Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>

Author: 3 people

modules/signals/rxjs-interop/spec/rx-method.spec.ts

@@ -2,11 +2,12 @@ import {
   createEnvironmentInjector,
   EnvironmentInjector,
   Injectable,
+  Injector,
   runInInjectionContext,
   signal,
 } from '@angular/core';
 import { TestBed } from '@angular/core/testing';
-import { BehaviorSubject, pipe, Subject, tap } from 'rxjs';
+import { BehaviorSubject, of, pipe, Subject, tap } from 'rxjs';
 import { rxMethod } from '../src';
 import { createLocalService } from '../../spec/helpers';
 
@@ -347,4 +348,59 @@ describe('rxMethod', () => {
       expect(globalService.globalObservableChangeCounter).toBe(2);
     });
   });
+
+  describe('warning on source injector', () => {
+    const warnSpy = vitest.spyOn(console, 'warn');
+
+    beforeEach(() => {
+      warnSpy.mockReset();
+    });
+
+    const createAdder = (callback: (value: number) => void) =>
+      TestBed.runInInjectionContext(() => rxMethod<number>(tap(callback)));
+
+    it('does not warn on non-reactive value and source injector', () => {
+      let a = 1;
+      const adder = createAdder((value) => (a += value));
+      adder(1);
+
+      expect(warnSpy).not.toHaveBeenCalled();
+    });
+
+    for (const [reactiveValue, name] of [
+      [signal(1), 'Signal'],
+      [of(1), 'Observable'],
+    ] as const) {
+      describe(`${name}`, () => {
+        it('warns when source injector is used', () => {
+          let a = 1;
+          const adder = createAdder((value) => (a += value));
+          adder(reactiveValue);
+
+          expect(warnSpy).toHaveBeenCalled();
+          const warning = (warnSpy.mock.lastCall || []).join(' ');
+          expect(warning).toMatch(
+            /reactive method was called outside the injection context with a signal or observable/
+          );
+        });
+
+        it('does not warn on manual injector', () => {
+          let a = 1;
+          const adder = createAdder((value) => (a += value));
+          const injector = TestBed.inject(Injector);
+          adder(reactiveValue, { injector });
+
+          expect(warnSpy).not.toHaveBeenCalled();
+        });
+
+        it('does not warn if called within injection context', () => {
+          let a = 1;
+          const adder = createAdder((value) => (a += value));
+          TestBed.runInInjectionContext(() => adder(reactiveValue));
+
+          expect(warnSpy).not.toHaveBeenCalled();
+        });
+      });
+    }
+  });
 });

modules/signals/rxjs-interop/src/rx-method.ts

@@ -10,6 +10,8 @@ import {
 } from '@angular/core';
 import { isObservable, noop, Observable, Subject } from 'rxjs';
 
+declare const ngDevMode: unknown;
+
 type RxMethodRef = {
   destroy: () => void;
 };
@@ -42,8 +44,25 @@ export function rxMethod<Input>(
       return { destroy: noop };
     }
 
+    const callerInjector = getCallerInjector();
+    if (
+      typeof ngDevMode !== 'undefined' &&
+      ngDevMode &&
+      config?.injector === undefined &&
+      callerInjector === undefined
+    ) {
+      console.warn(
+        '@ngrx/signals/rxjs-interop: The reactive method was called outside',
+        'the injection context with a signal or observable. This may lead to',
+        'a memory leak. Make sure to call it within the injection context',
+        '(e.g. in a constructor or field initializer) or pass an injector',
+        'explicitly via the config parameter.\n\nFor more information, see:',
+        'https://ngrx.io/guide/signals/rxjs-integration#reactive-methods-and-injector-hierarchies'
+      );
+    }
+
     const instanceInjector =
-      config?.injector ?? getCallerInjector() ?? sourceInjector;
+      config?.injector ?? callerInjector ?? sourceInjector;
 
     if (isSignal(input)) {
       const watcher = effect(
@@ -78,10 +97,10 @@ function isStatic<T>(value: T | Signal<T> | Observable<T>): value is T {
   return !isSignal(value) && !isObservable(value);
 }
 
-function getCallerInjector(): Injector | null {
+function getCallerInjector(): Injector | undefined {
   try {
     return inject(Injector);
   } catch {
-    return null;
+    return undefined;
   }
 }

modules/signals/spec/signal-method.spec.ts

@@ -3,6 +3,7 @@ import { TestBed } from '@angular/core/testing';
 import {
   createEnvironmentInjector,
   EnvironmentInjector,
+  Injector,
   runInInjectionContext,
   signal,
 } from '@angular/core';
@@ -214,4 +215,50 @@ describe('signalMethod', () => {
     TestBed.flushEffects();
     expect(a).toBe(104);
   });
+
+  describe('warns on source injector', () => {
+    const warnSpy = vitest.spyOn(console, 'warn');
+    const n = signal(1);
+
+    beforeEach(() => {
+      warnSpy.mockReset();
+    });
+
+    it('warns when source injector is used for a signal', () => {
+      let a = 1;
+      const adder = createAdder((value) => (a += value));
+      adder(n);
+
+      expect(warnSpy).toHaveBeenCalled();
+      const warning = (warnSpy.mock.lastCall || []).join(' ');
+      expect(warning).toMatch(
+        /function returned by signalMethod was called outside the injection context with a signal/
+      );
+    });
+
+    it('does not warn on non-reactive value and source injector', () => {
+      let a = 1;
+      const adder = createAdder((value) => (a += value));
+      adder(1);
+
+      expect(warnSpy).not.toHaveBeenCalled();
+    });
+
+    it('does not warn on manual injector', () => {
+      let a = 1;
+      const adder = createAdder((value) => (a += value));
+      const injector = TestBed.inject(Injector);
+      adder(n, { injector });
+
+      expect(warnSpy).not.toHaveBeenCalled();
+    });
+
+    it('does not warn if called within injection context', () => {
+      let a = 1;
+      const adder = createAdder((value) => (a += value));
+      TestBed.runInInjectionContext(() => adder(n));
+
+      expect(warnSpy).not.toHaveBeenCalled();
+    });
+  });
 });

modules/signals/src/signal-method.ts

@@ -10,6 +10,8 @@ import {
   untracked,
 } from '@angular/core';
 
+declare const ngDevMode: unknown;
+
 type SignalMethod<Input> = ((
   input: Input | Signal<Input>,
   config?: { injector?: Injector }
@@ -32,8 +34,25 @@ export function signalMethod<Input>(
     config?: { injector?: Injector }
   ): EffectRef => {
     if (isSignal(input)) {
+      const callerInjector = getCallerInjector();
+      if (
+        typeof ngDevMode !== 'undefined' &&
+        ngDevMode &&
+        config?.injector === undefined &&
+        callerInjector === undefined
+      ) {
+        console.warn(
+          '@ngrx/signals: The function returned by signalMethod was called',
+          'outside the injection context with a signal. This may lead to',
+          'a memory leak. Make sure to call it within the injection context',
+          '(e.g. in a constructor or field initializer) or pass an injector',
+          'explicitly via the config parameter.\n\nFor more information, see:',
+          'https://ngrx.io/guide/signals/signal-method#automatic-cleanup'
+        );
+      }
+
       const instanceInjector =
-        config?.injector ?? getCallerInjector() ?? sourceInjector;
+        config?.injector ?? callerInjector ?? sourceInjector;
 
       const watcher = effect(
         () => {
@@ -64,10 +83,10 @@ export function signalMethod<Input>(
   return signalMethodFn;
 }
 
-function getCallerInjector(): Injector | null {
+function getCallerInjector(): Injector | undefined {
   try {
     return inject(Injector);
   } catch {
-    return null;
+    return undefined;
   }
 }

projects/ngrx.io/content/guide/signals/rxjs-integration.md

@@ -208,7 +208,7 @@ export class BooksComponent implements OnInit {
 The cleanup behavior of reactive methods differs when they're created and called across different injector hierarchies.
 
 If the reactive method is called within the descendant injection context, the call will be automatically cleaned up when the descendant injector is destroyed.
-However, when the call is made outside of the descendant injection context, it's necessary to explicitly provide the descendant injector reference to ensure proper cleanup.
+However, when the call is made outside of the descendant injection context, it's necessary to explicitly provide the descendant injector reference to ensure proper cleanup. Otherwise, the cleanup occurs when the ascendant injector where the reactive method is initialized gets destroyed.
 
 ```ts
 import { Component, inject, Injectable, Injector, OnInit } from '@angular/core';
@@ -241,7 +241,7 @@ export class NumbersComponent implements OnInit {
 
 <div class="alert is-important">
 
-If the injector is not provided when calling the reactive method outside of current injection context, the cleanup occurs when reactive method is destroyed.
+If the injector is not provided when calling the reactive method with a signal or observable outside the injection context, a warning message about a potential memory leak is displayed in development mode.
 
 </div>
 
@@ -322,4 +322,4 @@ export class NumbersComponent implements OnInit {
     logNumber(10);
   }
 }
-```
+```

projects/ngrx.io/content/guide/signals/signal-method.md

@@ -91,6 +91,12 @@ export class NumbersComponent implements OnInit {
 
 Here, the `effect` outlives the component, which would produce a memory leak.
 
+<div class="alert is-important">
+
+If an injector is not provided when a method generated by `signalMethod` is called with a signal outside the injection context, a warning message about a potential memory leak is displayed in development mode.
+
+</div>
+
 ## Manual Cleanup
 
 When a `signalMethod` is created in an ancestor injection context, it's necessary to explicitly provide the caller injector to ensure proper cleanup: