Skip to content
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

Feature Request: Add DI between services #19

Closed
hwnprsd opened this issue Jul 13, 2020 · 11 comments
Closed

Feature Request: Add DI between services #19

hwnprsd opened this issue Jul 13, 2020 · 11 comments
Labels
enhancement New feature or request Published This new feature has been published.

Comments

@hwnprsd
Copy link

hwnprsd commented Jul 13, 2020

In my current project, I have a StorageService, DownloadService and an APIService.
More often than not, whatever is downloaded needs to be stored in the database and the API needs to be called to get the bucket URL for downloading the file.

Would it make sense to be able to access services using getService inside a MomentumService?
Or should I stick to injecting them through the bootStrap of a non-lazy controller?

@xamantra
Copy link
Owner

This is possible and would be a very helpful feature actually. I will work on this soon.

@xamantra xamantra added enhancement New feature or request TODO needs to be worked on labels Jul 13, 2020
@xamantra xamantra pinned this issue Jul 14, 2020
@exts
Copy link

exts commented Jul 14, 2020

@xamantra Since we're on the topic, any chance of also allowing callbacks to setup a service inside the momentum using existing services eg.:

Momentum(
  services: [
    Service(MyService, () {
      var apiService = getService<ApiService>();
      return MyService(apiService);
    }),
    FactoryService(MyService, () { ... }), // same concept except it creates a new instance every time it's called
  ]
);

@xamantra
Copy link
Owner

@exts, can you explain more about this? like, why would you want to have this and some use cases?

@exts
Copy link

exts commented Jul 14, 2020

For the factory service, sometimes you don't want to keep an instance of the same object (I'm assuming it's keeping a copy of the instance when its first called, I haven't tested or checked the source). For the other example, it just respects DIP, plus sometimes you might have a service that can be used on its own and another service that may rely on it. This would just allow you to pass it as a dependency when setting up the application. This would also solve ops issue as well which is cleaner and would make the services easier to test as well w/out needing to setup an momentum object.

@xamantra
Copy link
Owner

This would just allow you to pass it as a dependency when setting up the application.

I'm already done writing the function for getService<T> inside momentum service so this isn't needed anymore. Just needed to do some polishing. And this kind of pattern isn't flexible:

      var apiService = getService<ApiService>();
      return MyService(apiService);

sometimes you don't want to keep an instance of the same object

I see. An example of this would be like an ApiService with a parameter enableLogs, sometimes you don't want to enable logs at a certain area in your app. Example:

services: [
  ApiService(), // false by default.
  ApiService(enableLogs: true),
],

The question is, how can we grab the API service that has logs enabled and the one with logs disabled based on your FactoryService? I can't think of a pattern with your example code but I'm thinking of doing this:

// get the API service with logs enabled.
var apiServiceA = getService<ApiService>(where: (apiService) => apiService.logsEnabled);

// get the API service with logs disabled.
var apiServiceB = getService<ApiService>(where: (apiService) => !apiService.logsEnabled);

// if "where" parameter isn't provided, it returns the first instance from the "services" list.
// And it doesn't break existing projects too.
var apiService = getService<ApiService>();

So I think we don't need the FactoryService anymore.

@xamantra xamantra added In Progress Issues that are currently being worked on. and removed TODO needs to be worked on labels Jul 14, 2020
@exts
Copy link

exts commented Jul 14, 2020

On one end I like it, but I feel like wouldn't it be quicker to have some form of alias. What happens if your where clause gets unnecessarily lengthy?

var apiServiceExample = getService<ApiService>(alias: "logs");

///....

Momentum(
  services: [
    Service(ApiService, alias: "logs", () => ApiService(enableLogs: true)),
  ]
);

This is just me throwing out different ideas before you commit to one solution.

e: This also made me think about the many services and analytics tools out there that some apps use w/ similar interfaces which map to different apis.

@xamantra
Copy link
Owner

First, I would like to cleanup this code:

Service(ApiService, alias: "logs", () => ApiService(enableLogs: true)),

// You don't need the first runtime type parameter "ApiService".

Service(alias: "logs", () => ApiService(enableLogs: true)),

// You don't need a lambda to generate the service.

Service(alias: "logs", ApiService(enableLogs: true)),

I would also want to make the alias parameter to be dynamic so that we can provide any types of keys here for flexibility. Like using enums for example.

Momentum(
  services: [
    DatabaseService(),
    DownloadService(),
    InjectService(alias: ApiAlias.withoutLogs, ApiService()),
    InjectService(alias: ApiAlias.withLogs, ApiService(enableLogs: true)),
  ],
);

// accessing down the tree

var apiService = getService<ApiService>(alias: ApiAlias.withoutLogs);

// using context (non-breaking change).
var apiService = getService<ApiService>(context, alias: ApiAlias.withoutLogs);

// will return this service: "InjectService(alias: ApiAlias.withoutLogs, ApiService())," (the first one from the list)
var apiService = getService<ApiService>();

The code is a bit longer but it's much safer than using strings.

I'll forget about the where clause because I think this is better.

@exts
Copy link

exts commented Jul 14, 2020

yeah I like that much better.

xamantra pushed a commit that referenced this issue Jul 16, 2020
- Added new docs
@xamantra xamantra added RTP Ready to publish and removed In Progress Issues that are currently being worked on. labels Jul 16, 2020
@xamantra
Copy link
Owner

I published a new version on pub.dev: v1.2.8

Docs References:

@exts
Copy link

exts commented Jul 16, 2020

I'll test out the changes tonight to make sure everything works.

@xamantra xamantra added Published This new feature has been published. and removed RTP Ready to publish labels Jul 16, 2020
@xamantra
Copy link
Owner

@exts and @d3fkon . If you guys found an issue relating to this new feature. File a separate issue. I will be closing this one.

Repository owner locked as resolved and limited conversation to collaborators Jul 17, 2020
@xamantra xamantra unpinned this issue Jul 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request Published This new feature has been published.
Projects
None yet
Development

No branches or pull requests

3 participants