feat(signals): warn when reactive method runs with source injector#4742
Conversation
✅ Deploy Preview for ngrx-io canceled.Built without sensitive environment variables
|
7713962 to
3f8835a
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a warning when reactive methods (signalMethod and rxMethod) are invoked outside an injection context to help prevent memory leaks. The changes include updates to documentation, modifications to the core implementation to display a warning, and new tests to verify the warning behavior.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/ngrx.io/content/guide/signals/signal-method.md | Updated documentation alert message for signalMethod |
| projects/ngrx.io/content/guide/signals/rxjs-integration.md | Revised documentation text for reactive method warnings |
| modules/signals/src/signal-method.ts | Added warning on missing injector, updated getCallerInjector return |
| modules/signals/spec/signal-method.spec.ts | Added tests for warning behavior in signalMethod |
| modules/signals/rxjs-interop/src/rx-method.ts | Added warning on missing injector, updated getCallerInjector return |
| modules/signals/rxjs-interop/spec/rx-method.spec.ts | Added tests for warning behavior in rxMethod |
Comments suppressed due to low confidence (4)
projects/ngrx.io/content/guide/signals/signal-method.md:94
- [nitpick] Consider clarifying the alert message by referencing that an explicit injector can be provided via the config parameter, to align with the implementation details.
<div class="alert is-important">
projects/ngrx.io/content/guide/signals/rxjs-integration.md:243
- [nitpick] For clarity, consider explicitly naming the reactive method (e.g., rxMethod) in the warning description so that the documentation clearly communicates which methods are affected.
-If the injector is not provided when calling the reactive method outside of current injection context, the cleanup occurs when reactive method is destroyed.
modules/signals/src/signal-method.ts:36
- [nitpick] The multiline warning message in the console.warn call contains extra indentation that may lead to unintended whitespace in the output; consider formatting the string to remove excessive leading whitespace.
if (config?.injector === undefined && callerInjector === undefined) {
modules/signals/rxjs-interop/src/rx-method.ts:45
- [nitpick] Similar to the signalMethod implementation, ensure that the multiline warning message in the subsequent console.warn call is formatted to avoid unintended leading whitespace in the console output.
const callerInjector = getCallerInjector();
timdeschryver
left a comment
There was a problem hiding this comment.
LGTM, I've added a comment to make it clear that the message comes from us.
Do we also want to disable this in production?
I was also thinking if this should be something to be able to disable, but I can't think of any good use case for doing so.
I would keep it turned on. Generally speaking, I think it might be better to remove the possibility of using the source injector in the long run. It seems to me to be too risky... |
447c2d4 to
cad51a4
Compare
…n context Emit a warning when a reactive method created via `rxMethod` or `signalMethod` is invoked outside of an injection context without a manually provided injector. This helps detect potential memory leaks caused by mismatched injection contexts — for instance, when the method is defined in a service provided in root but invoked in a component’s `ngOnInit`.
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
cad51a4 to
fa72689
Compare
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
It gives a warning when
rxMethodorsignalMethod's functions are called outside an injection context and therefore bare the risk of memory leaks.Closes #4726
What is the new behavior?
Does this PR introduce a breaking change?