Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Allow constructor injection of SignalStore #4282

Closed
1 of 2 tasks
bradley-dotnet opened this issue Mar 25, 2024 · 3 comments
Closed
1 of 2 tasks

RFC: Allow constructor injection of SignalStore #4282

bradley-dotnet opened this issue Mar 25, 2024 · 3 comments

Comments

@bradley-dotnet
Copy link

bradley-dotnet commented Mar 25, 2024

Which @ngrx/* package(s) are relevant/related to the feature request?

signals

Information

Problem

The class that results from the signalStore method has a parameterless constructor.

The current way to add dependencies is by adding them to the withMethods feature factory function. Since this is called directly from the code creating the store, there's no opportunity for test code to override the use of the inject function. For tests avoiding the use of TestBed in order to ensure that root-level concrete dependencies are never accidentally used in a test, this is a big problem.

Proposal

Add a withDependencies method to the base set of signal store methods. This methods would result in one or more parameters (depending on the actual implementation details needed) being added to the generated class constructor, as well as the fields from the injection being available to all future with... calls.

The exact parameters for withDependencies will be based on what technical approaches for building it are actually feasible. The lack of runtime-type reflection in Typescript could restrict the options for that API. My initial thought would be accepting a Type[] that can then be spread into the generated constructor.

Also possible would be adding a special argument to the signalStore method similar to the existing one allowing for providedIn: 'root'.

While it could be done in a future effort, adding a similar withDestroyRef that prevents the destroy hook from using the inject function would also be very useful.

Describe any alternatives/workarounds you're currently using

  1. Use a regluar service that owns a SignalState. Since that class can use classic constructor injection passing in mocks from a test is simple. Has the problem of rxMethod needing to be run from an injection context unless an Injector is provided.
  2. Create a factory method that returns the result of signalStore. This method can be parameter injected and then use a closure to pass the parameters to the various methods within the store. The problem with this approach is that a manual injection token must be created since functions can not be directly provided/injected. It requires a lot of awkward boilerplate and I would not expect junior developers to maintain such a system.

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@va-stefanek
Copy link
Contributor

@timdeschryver should not that issue has tag Signals instead of Store?

@va-stefanek
Copy link
Contributor

va-stefanek commented Jul 24, 2024

@markostanimirovic any update with that one? Maybe there can be additional function e.g:

withDependencies({
dep: DepClass
}), 

This way people can define deps that can be utilize in any withMethod?

@markostanimirovic
Copy link
Member

The only benefit I see is the ability to test SignalStore without TestBed.

On the other hand, we also have to think about the negative consequences of this change:

  • Additional complexity in SignalStore implementation.
  • Breaking changes in multiple SignalStore models.

Additionally, without TestBed, we cannot test any store that uses rxMethod or effect. It's also not possible to test the behavior of the onDestroy hook.

So is it worth it? I don't think so.

@ngrx ngrx locked and limited conversation to collaborators Jul 24, 2024
@markostanimirovic markostanimirovic converted this issue into discussion #4459 Jul 24, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

4 participants