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.

Sign up for GitHub

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

Jump to bottom

[Question] GetItAll from one type? #75

Open
rafaelcorbellini-egsys opened this issue May 5, 2020 · 63 comments
Open

[Question] GetItAll from one type? #75

rafaelcorbellini-egsys opened this issue May 5, 2020 · 63 comments
Labels
enhancement New feature or request

Comments

@rafaelcorbellini-egsys
Copy link

Is there any way to get everyone of a kind?

example when logging out of the application I want to get all objects registered as DAO and perform a function of them.
@rafaelcorbellini-egsys rafaelcorbellini-egsys changed the title GetItAll from one type? [Question] GetItAll from one type? May 5, 2020
@escamoteur
Copy link
Collaborator

You mean you registered multiple instances of the same type? and you want to call the same method on all of them?

@rafaelcorbellini-egsys
Copy link
Author

I want get all instance of one type.

Example i have N InterfaceDAO (UserDao, TodoDAO... ) and register all of them.

have some way to resolve all (Iterable) registered ?

@escamoteur
Copy link
Collaborator

ah, understood. I will think about it.

@mahdiyami2 mahdiyami2 mentioned this issue Jun 28, 2020
Closed
@mareklat
Copy link

@escamoteur Have you analyzed this topic? Are there any real chances to implement this?

@escamoteur
Copy link
Collaborator

Could you elaborated why this would be useful?

@mareklat
Copy link

Of course. Most of all, it helps to write modular and clean code.

For example:
I have an Interceptor interface in my http client. In the object responsible for communication with the server, I inject a list of objects implementing the Interceptor interface. Then, when I want to add a new Interceptor, just only create a new implementation.

A similar solution is present in other DI libraries, e.g. Dagger, Spring Boot, etc.

@escamoteur
Copy link
Collaborator

One question would be should it only get all of exact the same type or also derived types?

@mareklat
Copy link

I think they should it only get all of exact the same type. We can use type passed when there are registering factory, for example

getit.factory<Interceptor>(() => LoggingInterceptor());

Currently, when I register more than one object with the same type and when allowReassignment==false , the library throws an exception. Instead, we could register a collection of interfaces

getit.factory<List<Interceptor>>(() => [
   get<LoggingInterceptor>(),
   get<AnotherInterceptor>(),
   ...
]);

Maybe new flag will be necessary to allow binding into collection.

@escamoteur
Copy link
Collaborator

ok, makes sense, but I need to find time for it. Still not finished to convert all my pacakges to null safety

@mareklat
Copy link

Fantastic :) I haven't explored the get_it internal yet, but in my free time I'll try to help

@mareklat mareklat mentioned this issue Feb 25, 2021
Closed
@mareklat
Copy link

@escamoteur Hey, have you found the time to focus on this idea? Can we reopen this topic? I believe that this is a crucial and much needed feature

@escamoteur escamoteur reopened this Jun 12, 2021
@escamoteur
Copy link
Collaborator

OK,

your PR used a special intoSet parameter. But do we need this if we can use names instances?
You want to be ablt to do a .getAll<Type>() right?
If we limit it to the exact type I would probably only have to make a smal change to the internal get_it data structure. But my feeling is we should be able to request all of one type plus derived types to really allo full flexibility

@mareklat
Copy link

Yes, I agree that the ability to call derived types should remain.

I have a few ideas, but most of it comes to idea where firstly we register instance in the same way as it is now, and optionally bind it to list of parent types.

Normally we can register instance like this:
getIt.registerFactory(() =>UserDTO())

But if we want to also register instance to collection of supertypes we can call like this:

getIt.registerFactory(() =>UserDTO()).inToList<DTO>();

and in some point in code:

getIt.get<List<DTO>>()

To allow this behavior, each registration method must return an object. An object that allows you to call the inToSet method. An example of this object:

class Multibinding<T extends Object> {

  final GetIt _getIt;

  Multibinding(this._getIt);

  void inToList<S>() {
    if (!(T is S)) {
      throw "$T is not subtype of $S";
    }

    if (_getIt.isRegistered<List<S>>()) {
      List<S> list = _getIt.get();
      _getIt.unregister<List<S>>();
      _getIt.registerFactory<List<S>>(() {
        T instance = _getIt.get();
        var newList = list.toList();
        newList.add(instance as S);
        return List.unmodifiable(newList);
      });
    } else {
      _getIt.registerFactory<List<S>>(() {
        T instance = _getIt.get();
        var newList = <S>[];
        newList.add(instance as S);
        return List.unmodifiable(newList);
      });
    }
  }

}

I think this solution allows you to implement multibinding concept with a minimum amount of work. What do you think?

@escamoteur
Copy link
Collaborator

not sure if we need that.

if we add an

List<MyType> = getIt.getAll<MyType>();

that returns you all registered objects with that type and its derived types.

@mareklat
Copy link

Hmm.. this will be much better to use.
In this approach when we call getAll<SuperType>(), we need scan all factories and compare like T is SuperType, collect them to list and return.
This will be efficient?

@escamoteur
Copy link
Collaborator

Yeah this might not be very performant. But I try to imagine how often will you have to do such a call? It's probably nothing you do on every frame.
it's O(n) which means even if you have registered 1000 objects it shouldn't take long

@escamoteur
Copy link
Collaborator

I would say lets implement it like that and see what the feedback is

@escamoteur
Copy link
Collaborator

would you try a PR?

@mareklat
Copy link

You're right, maybe it's not such a bad idea. Ok, I will prepare a PR and we will see :)

@escamoteur
Copy link
Collaborator

please don't forget adding the new function to the readme

@ernesto
Copy link

ernesto commented Aug 9, 2021

@mareklat I am working with a similar stuff here, your PR will be very appreciated :D
About O(n) problem, I suppose it can be resolved with a getAll<SuperType, DerivadType?>() ?

@vladimirfx
Copy link

vladimirfx commented Oct 16, 2021 •
edited

IMO there is two separate API calls:

  • getAll<ExactType>()
  • getAllWithSubtypes<SuperType>()

Our practice with Dagger and Guice (Android and backend) shown that first call is sufficient in 90%+ cases. Actually I found 4 cases out of few hundred in all accessible projects where with subtypes lookup is used. For SOLID-complied code that case is very uncommon if possible at all.

So I propose implement '10 for 90' case with just getAll<ExactType>() and see on feedback to implement withSubtypes case.

About O(n) complexity - for most practical graphs (<1000 bindings) is not measurable impact on app performance.

@escamoteur
Copy link
Collaborator

Hi, sorry for not looking into this earlier, but I had some mental health problems the last half year.

No PR has appeared yet. let me see if I find the time to add it

@escamoteur escamoteur added the enhancement New feature or request label Feb 2, 2022
@iandis
Copy link

iandis commented Jun 14, 2022

I think a use case like clearing/resetting all the DAOs/ViewModel objects when a user log out can be done by using something like the Pub-Sub pattern instead.

@hughesjs
Copy link
Contributor

hughesjs commented Feb 12, 2024 •
edited

The later one probably because c# supports method overloading. Maybe making it a requirement to provide an instance name if you want to register multiple implementations and get all of them would make implementing this really easy and fast Am 12. Feb. 2024, 14:36 +0100 schrieb James H @.>:
…
I think the way this is handled in MS DI is that if you want a specific implementation when there are multiple registered you need to use a named instance, or register a factory which takes the list and does what you need with it. In all honesty, in cases where I've used this, I've never had a case where I've wanted to inject both a list of all implementations and a specific implementation. So I'm not 100% sure if what I'm saying is correct. Interestingly, there's also no concept of .GetAll(), it's just the behaviour of .Get<IEnumerable() (the equivalent of iterable). — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: @.>

I don't think you need to overload anything, both of those method calls have the same signature, one type arg and nothing else, there's probably just an if typeof(T).IsAssignableTo<IEnumerable<>> in there.

As for your second bit, do you mean if you want to register multiple implementations and get a specific one you need a name? I don't see why you'd need a name if you only ever wanted to be able to get the list.

Another option would be to just document that if you register multiple and don't provide instance names, you get the first/last on registered when trying to get a single one.

@escamoteur
Copy link
Collaborator

escamoteur commented Feb 12, 2024 via email

@hughesjs
Copy link
Contributor

Failing that, I don't think there's any qualms in having it as a separate .GetAll()

@escamoteur
Copy link
Collaborator

escamoteur commented Feb 12, 2024 via email

@escamoteur
Copy link
Collaborator

escamoteur commented Feb 12, 2024 via email

@hughesjs
Copy link
Contributor

hughesjs commented Feb 13, 2024 •
edited

I would expect it to only return things that were explicitly registered with that interface:

So:

.register<BaseType>(DerivedA());
.register<BaseType>(DerivedB());
.register<DerivedC>(DerivedC());
.getAll<BaseType>();

Would return:

[
DerivedA(),
DerivedB()
]

@hughesjs
Copy link
Contributor

An old colleague of mine has just shared this with me which I believe offers the functionality we're talking about. Might be helpful, might not.

https://github.com/LeeSanderson/SimpleDartServiceProvider

@escamoteur
Copy link
Collaborator

escamoteur commented Feb 13, 2024 via email

@hughesjs
Copy link
Contributor

hughesjs commented Feb 19, 2024 •
edited

I'm not sure about that one. I think the principal of least surprise would suggest either of these would be acceptable:

  • Make it behave similarly to get<T> so it gets everything from the current scope and down
  • Have two methods getAllFromAllScopes<T> and getAllFromCurrentScope<T>

I've not really used this feature though

@escamoteur
Copy link
Collaborator

Hi,
I'm implemented that feature but haven't tested it yet. See #354
It would be awesome if one of you could write some tests for it and and a section in the readme.

@escamoteur
Copy link
Collaborator

Also if one of you could review my latest changes in case you spot any problems. Existing tests pass all.

@escamoteur
Copy link
Collaborator

The current implementation only returns the current Scope, I all an allScopes parameter as soon as you can verify that this works so far

@hughesjs
Copy link
Contributor

I'll take a look if I get a chance, might not be til the weekend though

@escamoteur
Copy link
Collaborator

escamoteur commented Feb 20, 2024 via email

@tpucci
Copy link
Contributor

tpucci commented Feb 24, 2024

Hi, I'm implemented that feature but haven't tested it yet. See #354 It would be awesome if one of you could write some tests for it and and a section in the readme.

Hey!

I've tried the feature. There is a little bug and the fix is easy.

Otherwise, it seems good!

🙂

Should I make a PR ?

@escamoteur
Copy link
Collaborator

escamoteur commented Feb 24, 2024 via email

@tpucci tpucci mentioned this issue Feb 24, 2024
Merged
@tpucci
Copy link
Contributor

tpucci commented Feb 24, 2024

Here you go: #355
Cheers! 😃

@tpucci
Copy link
Contributor

tpucci commented Mar 21, 2024

Can we merge this @escamoteur ? 🙂 #355

@escamoteur
Copy link
Collaborator

escamoteur commented Mar 21, 2024 via email

@escamoteur
Copy link
Collaborator

if anyone of you have time to add some docs on this new function (we now also have the async version) #361 to the readme, I would be really thankful as I don't have a lot of time at the moment

@hughesjs
Copy link
Contributor

hughesjs commented Apr 17, 2024 •
edited

if anyone of you have time to add some docs on this new function (we now also have the async version) #361 to the readme, I would be really thankful as I don't have a lot of time at the moment

Yeah, I can probably document this (or at least start on documenting it) this afternoon.

@escamoteur
Copy link
Collaborator

That would be awesome. currently I didn't have time to add the alallScopes parameter. I try to do that the next days

@hughesjs
Copy link
Contributor

Is this all merged into main? Just so I know where to branch from.

@escamoteur
Copy link
Collaborator

yep

@hughesjs
Copy link
Contributor

How's this? #363

@hughesjs hughesjs mentioned this issue Apr 22, 2024
Merged
@escamoteur
Copy link
Collaborator

Hi friends, may I ask you for one more help on this? I just added the fromAllScopes parameter to getAll and getAllAsync plus fixing some other stuff which took me almost all day.
Could one of you add tests for this latest changes, I have to get back to my project.

@hughesjs
Copy link
Contributor

Sorry, I've not really used the scopes functionality of GetIt, so I wouldn't be the best person to write this

@escamoteur
Copy link
Collaborator

escamoteur commented May 14, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests