Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion modules/signals/rxjs-interop/spec/rx-method.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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();
});
});
}
});
});
25 changes: 22 additions & 3 deletions modules/signals/rxjs-interop/src/rx-method.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import {
} from '@angular/core';
import { isObservable, noop, Observable, Subject } from 'rxjs';

declare const ngDevMode: unknown;

type RxMethodRef = {
destroy: () => void;
};
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
}
}
47 changes: 47 additions & 0 deletions modules/signals/spec/signal-method.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { TestBed } from '@angular/core/testing';
import {
createEnvironmentInjector,
EnvironmentInjector,
Injector,
runInInjectionContext,
signal,
} from '@angular/core';
Expand Down Expand Up @@ -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();
});
});
});
25 changes: 22 additions & 3 deletions modules/signals/src/signal-method.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import {
untracked,
} from '@angular/core';

declare const ngDevMode: unknown;

type SignalMethod<Input> = ((
input: Input | Signal<Input>,
config?: { injector?: Injector }
Expand All @@ -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(
() => {
Expand Down Expand Up @@ -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;
}
}
6 changes: 3 additions & 3 deletions projects/ngrx.io/content/guide/signals/rxjs-integration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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>

Expand Down Expand Up @@ -322,4 +322,4 @@ export class NumbersComponent implements OnInit {
logNumber(10);
}
}
```
```
6 changes: 6 additions & 0 deletions projects/ngrx.io/content/guide/signals/signal-method.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down