Skip to content

Commit

Permalink
fix(signals): use Injector of rxMethod instance caller
Browse files Browse the repository at this point in the history
Ensure that an instance of `rxMethod` uses the `Injector` of the caller where
the instance was invoked, rather than the injector of the node where `rxMethod`
was created.

This change addresses a potential memory leak where a `SignalStore` method,
generated by `rxMethod` and provided at the root level, continues to track a
signal via its `effect` using the `RootInjector`. In cases where the component
calling the method is destroyed, the `Signal` remains tracked, leading to
unintended behavior and memory retention.

With this commit, `rxMethod` now prioritizes the injector of the instance
caller, falling back to the creator's injector only if no injection context
exists. Additionally, a custom injector can be provided by the caller and will
take precedence when specified.

For `Observable`, since it has a different completion mechanism, the instance
caller is still responsible for handling unsubscription or completion.
  • Loading branch information
rainerhahnekamp committed Sep 22, 2024
1 parent 1f7f740 commit 7286585
Show file tree
Hide file tree
Showing 2 changed files with 155 additions and 8 deletions.
127 changes: 126 additions & 1 deletion modules/signals/rxjs-interop/spec/rx-method.spec.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
import {
Component,
createEnvironmentInjector,
EnvironmentInjector,
inject,
Injectable,
Injector,
OnInit,
signal,
} from '@angular/core';
import { TestBed } from '@angular/core/testing';
import { BehaviorSubject, pipe, Subject, tap } from 'rxjs';
import { BehaviorSubject, finalize, pipe, Subject, tap } from 'rxjs';
import { rxMethod } from '../src';
import { createLocalService } from '../../spec/helpers';
import { provideRouter } from '@angular/router';
import { provideLocationMocks } from '@angular/common/testing';
import { RouterTestingHarness } from '@angular/router/testing';

describe('rxMethod', () => {
it('runs with a value', () => {
Expand Down Expand Up @@ -231,4 +238,122 @@ describe('rxMethod', () => {
TestBed.flushEffects();
expect(counter()).toBe(4);
});

it('completes on manual destroy with Signals', () => {
TestBed.runInInjectionContext(() => {
let completed = false;
const counter = signal(1);
const fn = rxMethod<number>(finalize(() => (completed = true)));
TestBed.flushEffects();
fn(counter);
fn.unsubscribe();
expect(completed).toBe(true);
});
});

describe('Signals and effect injector', () => {
@Injectable({ providedIn: 'root' })
class GlobalService {
rxMethodStatus = 'none';
log = rxMethod<string>(
pipe(
tap({
next: () => (this.rxMethodStatus = 'started'),
finalize: () => (this.rxMethodStatus = 'destroyed'),
})
)
);
}

@Component({
selector: `app-storeless`,
template: ``,
standalone: true,
})
class WithoutStoreComponent {}

function setup(WithStoreComponent: new () => unknown): GlobalService {
TestBed.configureTestingModule({
providers: [
provideRouter([
{ path: 'with-store', component: WithStoreComponent },
{
path: 'without-store',
component: WithoutStoreComponent,
},
]),
provideLocationMocks(),
],
});

return TestBed.inject(GlobalService);
}

it('should destroy with component injector when rxMethod is in root and RxMethod in component', async () => {
@Component({
selector: 'app-with-store',
template: ``,
standalone: true,
})
class WithStoreComponent {
store = inject(GlobalService);

constructor() {
this.store.log(signal('test'));
}
}

const globalService = setup(WithStoreComponent);

const harness = await RouterTestingHarness.create('/with-store');
expect(globalService.rxMethodStatus).toBe('started');
await harness.navigateByUrl('/without-store');
expect(globalService.rxMethodStatus).toBe('destroyed');
});

it("should fallback to rxMethod's injector when RxMethod's call is outside of injection context", async () => {
@Component({
selector: `app-store`,
template: ``,
standalone: true,
})
class WithStoreComponent implements OnInit {
store = inject(GlobalService);

ngOnInit() {
this.store.log(signal('test'));
}
}

const globalService = setup(WithStoreComponent);

const harness = await RouterTestingHarness.create('/with-store');
expect(globalService.rxMethodStatus).toBe('started');
await harness.navigateByUrl('/without-store');
expect(globalService.rxMethodStatus).toBe('started');
});

it('should provide the injector for RxMethod on call', async () => {
@Component({
selector: `app-store`,
template: ``,
standalone: true,
})
class WithStoreComponent implements OnInit {
store = inject(GlobalService);
injector = inject(Injector);

ngOnInit() {
this.store.log(signal('test'), this.injector);
}
}

const globalService = setup(WithStoreComponent);

const harness = await RouterTestingHarness.create('/with-store');
expect(globalService.rxMethodStatus).toBe('started');
await harness.navigateByUrl('/without-store');
expect(globalService.rxMethodStatus).toBe('destroyed');
});
});
});
36 changes: 29 additions & 7 deletions modules/signals/rxjs-interop/src/rx-method.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import {
import { isObservable, noop, Observable, Subject, Unsubscribable } from 'rxjs';

type RxMethod<Input> = ((
input: Input | Signal<Input> | Observable<Input>
input: Input | Signal<Input> | Observable<Input>,
injector?: Injector
) => Unsubscribable) &
Unsubscribable;

Expand All @@ -24,22 +25,32 @@ export function rxMethod<Input>(
}

const injector = config?.injector ?? inject(Injector);
const destroyRef = injector.get(DestroyRef);
const source$ = new Subject<Input>();

const sourceSub = generator(source$).subscribe();
destroyRef.onDestroy(() => sourceSub.unsubscribe());

const rxMethodFn = (input: Input | Signal<Input> | Observable<Input>) => {
const rxMethodFn = (
input: Input | Signal<Input> | Observable<Input>,
customInjector?: Injector
) => {
if (isSignal(input)) {
const callerInjector = getCallerInjectorIfAvailable();
const instanceInjector = customInjector ?? callerInjector ?? injector;

const watcher = effect(
() => {
const value = input();
untracked(() => source$.next(value));
},
{ injector }
{ injector: instanceInjector }
);
const instanceSub = { unsubscribe: () => watcher.destroy() };

instanceInjector.get(DestroyRef).onDestroy(() => {
sourceSub.unsubscribe();
});

const instanceSub = {
unsubscribe: () => watcher.destroy(),
};
sourceSub.add(instanceSub);

return instanceSub;
Expand All @@ -49,6 +60,9 @@ export function rxMethod<Input>(
const instanceSub = input.subscribe((value) => source$.next(value));
sourceSub.add(instanceSub);

const destroyRef = injector.get(DestroyRef);
destroyRef.onDestroy(() => sourceSub.unsubscribe());

return instanceSub;
}

Expand All @@ -59,3 +73,11 @@ export function rxMethod<Input>(

return rxMethodFn;
}

function getCallerInjectorIfAvailable(): Injector | null {
try {
return inject(Injector);
} catch (e) {
return null;
}
}

0 comments on commit 7286585

Please sign in to comment.